PR feedback

* Simplify SecureAppContext
* Update "is BF enabled?" linker checks
* Fix resource strings
* Add wasm-specific tests

src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.Shared.xml

@@ -12,5 +12,8 @@
     <type fullname="System.LocalAppContextSwitches">
       <method signature="System.Boolean get_EnableUnsafeUTF7Encoding()" body="stub" value="false" feature="System.Text.Encoding.EnableUnsafeUTF7Encoding" featurevalue="false" />
     </type>
+    <type fullname="System.Runtime.Serialization.SerializationInfo">
+      <method signature="System.Boolean get_BinaryFormatterEnabled()" body="stub" value="false" feature="System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization" featurevalue="false" />
+    </type>
   </assembly>
 </linker>

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()

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<string, ResourceLocator> 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)!);

src/libraries/System.Private.CoreLib/src/System/SecureAppContext.cs

@@ -1,36 +1,37 @@
 // 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
 {
     /// <summary>
     /// Represents AppContext switches that were in effect when the AppDomain was created.
     /// </summary>
     /// <remarks>
     /// 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.
     /// </remarks>
     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"),
-            };
-        }
+        /// <summary>
+        /// Returns a value stating whether BinaryFormatter serialization is allowed.
+        /// </summary>
+        internal static bool BinaryFormatterEnabled { get; private set; }
+
+        /// <summary>
+        /// Returns a value stating whether Serialization Guard is enabled.
+        /// </summary>
+        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);
         }
 
-        /// <summary>
-        /// Returns a value stating whether BinaryFormatter serialization is allowed.
-        /// </summary>
-        internal static bool BinaryFormatterEnabled => s_switches.BinaryFormatterEnabled;
-
-        /// <summary>
-        /// Returns a value stating whether Serialization Guard is enabled.
-        /// </summary>
-        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");
         }
     }
 }

src/libraries/System.Runtime.Serialization.Formatters/src/Resources/Strings.resx

@@ -250,6 +250,6 @@
     <value>Unable to read beyond the end of the stream.</value>
   </data>
   <data name="BinaryFormatter_SerializationDisallowed" xml:space="preserve">
-    <value>BinaryFormatter serialization and deserialization is disallowed within this application. See https://aka.ms/binaryformatter for more information.</value>
+    <value>BinaryFormatter serialization and deserialization are disabled within this application. See https://aka.ms/binaryformatter for more information.</value>
   </data>
 </root>

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<NotSupportedException>(() => 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<NotSupportedException>(() => 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();

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<FieldAccessException>(() => 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)]