diff --git a/src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.Shared.xml b/src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.Shared.xml index 1ebf944c46ad14..2467906aaaa4dc 100644 --- a/src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.Shared.xml +++ b/src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.Shared.xml @@ -12,5 +12,8 @@ + + + diff --git a/src/libraries/System.Private.CoreLib/src/System/AppContext.cs b/src/libraries/System.Private.CoreLib/src/System/AppContext.cs index 8efe52a223ab9c..e446c4277cd15d 100644 --- a/src/libraries/System.Private.CoreLib/src/System/AppContext.cs +++ b/src/libraries/System.Private.CoreLib/src/System/AppContext.cs @@ -6,7 +6,6 @@ using System.Diagnostics; using System.IO; using System.Reflection; -using System.Runtime.CompilerServices; using System.Runtime.ExceptionServices; using System.Runtime.Loader; using System.Runtime.Versioning; @@ -143,8 +142,7 @@ internal static unsafe void Setup(char** pNames, char** pValues, int count) s_dataStore.Add(new string(pNames[i]), new string(pValues[i])); } - // burn these values in to the SecureAppContext's cctor - RuntimeHelpers.RunClassConstructor(typeof(SecureAppContext).TypeHandle); + SecureAppContext.Initialize(); } private static string GetBaseDirectoryCore() diff --git a/src/libraries/System.Private.CoreLib/src/System/Resources/ResourceReader.Core.cs b/src/libraries/System.Private.CoreLib/src/System/Resources/ResourceReader.Core.cs index d28976070f96bc..f10afb8116c454 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Resources/ResourceReader.Core.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Resources/ResourceReader.Core.cs @@ -6,6 +6,7 @@ using System.Diagnostics; using System.IO; using System.Reflection; +using System.Runtime.Serialization; using System.Text; using System.Threading; @@ -37,7 +38,7 @@ internal ResourceReader(Stream stream, Dictionary resCa _ums = stream as UnmanagedMemoryStream; - _permitDeserialization = permitDeserialization; + _permitDeserialization = permitDeserialization && SerializationInfo.BinaryFormatterEnabled; ReadResources(); } @@ -67,6 +68,12 @@ private object DeserializeObject(int typeIndex) private void InitializeBinaryFormatter() { + if (!SerializationInfo.BinaryFormatterEnabled) + { + // allows the linker to trim away all the reflection goop below + throw new NotSupportedException(SR.NotSupported_ResourceObjectSerialization); + } + LazyInitializer.EnsureInitialized(ref s_binaryFormatterType, () => Type.GetType("System.Runtime.Serialization.Formatters.Binary.BinaryFormatter, System.Runtime.Serialization.Formatters, Version=0.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a", throwOnError: true)!); diff --git a/src/libraries/System.Private.CoreLib/src/System/SecureAppContext.cs b/src/libraries/System.Private.CoreLib/src/System/SecureAppContext.cs index dec4c0e9ac0071..ee3f8eec1858ed 100644 --- a/src/libraries/System.Private.CoreLib/src/System/SecureAppContext.cs +++ b/src/libraries/System.Private.CoreLib/src/System/SecureAppContext.cs @@ -1,6 +1,8 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Diagnostics; + namespace System { /// @@ -8,29 +10,28 @@ namespace System /// /// /// This class provides a place to store the values of security-sensitive switches as they - /// were when the application started. Attackers cannot use standard "open reflection" - /// gadgets to modify these fields since the reflection stack forbids altering the contents - /// of static initonly fields. This provides an extra layer of defense for applications - /// which rely on these switches as part of an overall attack surface reduction strategy. + /// were when the application started. It guards against dependency code inadvertently + /// calling AppContext.SetSwitch and subverting app-level policy. /// - /// This is not meant to be a perfect defense. A caller can always use unsafe code to modify - /// these static fields. However, we assume such a caller is already running code within the - /// process. Arbitrary memory writes can also alter these fields. Both of these scenarios are - /// outside the scope of our threat model. + /// This is not meant to be a perfect defense. A determined caller can always use private + /// reflection to modify the contents of these switches. But that doesn't fall under the + /// realm of "inadvertent" so is outside the scope of our threat model. /// internal static class SecureAppContext { - // Important: this field should be annotated 'static readonly' - private static readonly Switches s_switches = InitSwitches(); +#if DEBUG + private static bool s_isInitialized; +#endif - private static Switches InitSwitches() - { - return new Switches() - { - BinaryFormatterEnabled = GetSwitchValue("System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization"), - SerializationGuardEnabled = GetSwitchValue("Switch.System.Runtime.Serialization.SerializationGuard"), - }; - } + /// + /// Returns a value stating whether BinaryFormatter serialization is allowed. + /// + internal static bool BinaryFormatterEnabled { get; private set; } + + /// + /// Returns a value stating whether Serialization Guard is enabled. + /// + internal static bool SerializationGuardEnabled { get; private set; } private static bool GetSwitchValue(string switchName) { @@ -45,20 +46,15 @@ private static bool GetSwitchValue(string switchName) return LocalAppContextSwitches.GetCachedSwitchValue(switchName, ref cachedValue); } - /// - /// Returns a value stating whether BinaryFormatter serialization is allowed. - /// - internal static bool BinaryFormatterEnabled => s_switches.BinaryFormatterEnabled; - - /// - /// Returns a value stating whether Serialization Guard is enabled. - /// - internal static bool SerializationGuardEnabled => s_switches.SerializationGuardEnabled; - - private struct Switches + internal static void Initialize() { - internal bool BinaryFormatterEnabled { get; init; } - internal bool SerializationGuardEnabled { get; init; } +#if DEBUG + Debug.Assert(!s_isInitialized, "Initialize shouldn't be called multiple times."); + s_isInitialized = true; +#endif + + BinaryFormatterEnabled = GetSwitchValue("System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization"); + SerializationGuardEnabled = GetSwitchValue("Switch.System.Runtime.Serialization.SerializationGuard"); } } } diff --git a/src/libraries/System.Runtime.Serialization.Formatters/src/Resources/Strings.resx b/src/libraries/System.Runtime.Serialization.Formatters/src/Resources/Strings.resx index 0dffbf178a8e5d..db744f8c42ee82 100644 --- a/src/libraries/System.Runtime.Serialization.Formatters/src/Resources/Strings.resx +++ b/src/libraries/System.Runtime.Serialization.Formatters/src/Resources/Strings.resx @@ -250,6 +250,6 @@ Unable to read beyond the end of the stream. - BinaryFormatter serialization and deserialization is disallowed within this application. See https://aka.ms/binaryformatter for more information. + BinaryFormatter serialization and deserialization are disabled within this application. See https://aka.ms/binaryformatter for more information. diff --git a/src/libraries/System.Runtime.Serialization.Formatters/tests/DisableBitTests.cs b/src/libraries/System.Runtime.Serialization.Formatters/tests/DisableBitTests.cs index 7f47f8d28e67f6..c5bb8658252e89 100644 --- a/src/libraries/System.Runtime.Serialization.Formatters/tests/DisableBitTests.cs +++ b/src/libraries/System.Runtime.Serialization.Formatters/tests/DisableBitTests.cs @@ -11,12 +11,29 @@ namespace System.Runtime.Serialization.Formatters.Tests public static class DisableBitTests { // these tests only make sense on platforms with both SecureAppContext and RemoteExecutor support - public static bool ShouldRunTests => !PlatformDetection.IsNetFramework && RemoteExecutor.IsSupported; + public static bool ShouldRunFullAppContextEnablementChecks => !PlatformDetection.IsNetFramework && RemoteExecutor.IsSupported; private const string EnableBinaryFormatterSwitchName = "System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization"; private const string MoreInfoUrl = "https://aka.ms/binaryformatter"; - [ConditionalFact(nameof(ShouldRunTests))] + [Fact] + [PlatformSpecific(TestPlatforms.Browser)] + public static void DisabledAlwaysInBrowser() + { + // First, test serialization + + MemoryStream ms = new MemoryStream(); + BinaryFormatter bf = new BinaryFormatter(); + var ex = Assert.Throws(() => bf.Serialize(ms, "A string to serialize.")); + Assert.Contains(MoreInfoUrl, ex.Message, StringComparison.Ordinal); // error message should link to the more info URL + + // Then test deserialization + + ex = Assert.Throws(() => bf.Deserialize(ms)); + Assert.Contains(MoreInfoUrl, ex.Message, StringComparison.Ordinal); // error message should link to the more info URL + } + + [ConditionalFact(nameof(ShouldRunFullAppContextEnablementChecks))] public static void DisabledThroughAppContext() { RemoteExecutor.Invoke(() => @@ -37,7 +54,7 @@ public static void DisabledThroughAppContext() }).Dispose(); } - [ConditionalFact(nameof(ShouldRunTests))] + [ConditionalFact(nameof(ShouldRunFullAppContextEnablementChecks))] public static void DisabledThroughSecureAppContext_CannotOverride() { RemoteInvokeOptions options = new RemoteInvokeOptions(); diff --git a/src/libraries/System.Runtime/tests/System/AppContext/SecureAppContext.cs b/src/libraries/System.Runtime/tests/System/AppContext/SecureAppContext.cs index c3549a9aaf30e4..add6275aeb21e6 100644 --- a/src/libraries/System.Runtime/tests/System/AppContext/SecureAppContext.cs +++ b/src/libraries/System.Runtime/tests/System/AppContext/SecureAppContext.cs @@ -10,34 +10,6 @@ namespace System.Tests { public partial class SecureAppContextTests { - // these tests only make sense on platforms where reflection is expected to forbid overwriting initonly fields - public static bool RunForbidSettingInitonlyTests => PlatformDetection.IsNetCore && RemoteExecutor.IsSupported; - - [ConditionalFact(nameof(RunForbidSettingInitonlyTests))] - public void CannotUseReflectionToChangeValues() - { - RemoteExecutor.Invoke(() => - { - Type secureAppContextType = typeof(AppContext).Assembly.GetType("System.SecureAppContext", throwOnError: true); - - foreach (FieldInfo field in secureAppContextType.GetFields(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Static)) - { - try - { - Assert.True(field.FieldType.IsValueType, "Field is a reference type; instance members may be subject to mutation."); - Assert.True(field.IsInitOnly, "Field is mutable."); - - object originalValue = field.GetValue(null); - Assert.Throws(() => field.SetValue(null, originalValue)); - } - catch (Exception ex) - { - throw new Exception($"Failure when testing field {field.Name}.", ex); - } - } - }).Dispose(); - } - [ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] [InlineData("Switch.System.Runtime.Serialization.SerializationGuard", "SerializationGuardEnabled", true)] [InlineData("System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization", "BinaryFormatterEnabled", true)]