Introduce opt-in BinaryFormatter killbit #38963

GrabYourPitchforks · 2020-07-08T22:39:39Z

This is the first stage of the long-term plan to begin winding down BinaryFormatter within the framework. Contributes to #29976. That issue is a bit outdated, and we're cleaning up the new long-term plan before publishing it. But wanted to at least get this in since there's agreement on the team that it's valuable.

This PR introduces the following changes:

Introduce an opt-in way to killbit BinaryFormatter via the .csproj or your application's config
Hooks the killbit up through the linker trimming feature, so precompiled apps don't contain any of the BinaryFormatter infrastructure
Disables BinaryFormatter completely in wasm projects, as it wasn't intended to work in those scenarios
Carves out some AppContext switches as "sensitive" such that they cannot be overwritten using standard reflection
Refactors some of the Serialization Guard logic out into a separate file, but no functional changes aside from plumbing through the "sensitive" switch infrastructure mentioned above

There are no public API changes as part of this PR. I've added the "needs doc" label anyway since this introduces new behaviors that should be noted in the API docs.

Opting in to the killbit via config

For applications which don't anticipate using any BinaryFormatter code paths, the BinaryFormatter feature can be disabled wholesale via app config or a project-wide setting. This can only be done at the application level, not at the library level. This can be used to reduce the overall output size of the compiled application, or it can be done as part of an overall threat surface reduction. The threat surface reduction stems from the elimination of normally-untaken code paths that call into the BinaryFormatter infrastructure and that an adversary might discover how to activate, against the app developer's intentions.

At this time we don't anticipate app developers opting in to the killbit in WinForms apps or other project types where the runtime itself makes use of BinaryFormatter. Those scenarios will likely fail in unexpected ways.

Via runtimeConfig.template.json:

{
    "configProperties": {
        "System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization": false
    }
}

Or via your application's .csproj:

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net5.0</TargetFramework>
    <!-- using the line below -->
    <EnableUnsafeBinaryFormatterSerialization>false</EnableUnsafeBinaryFormatterSerialization>
  </PropertyGroup>
</Project>

(Note: the csproj setting requires changes in the runtime/sdk repo, which aren't yet submitted.)

Impact to WASM projects

The killbit is always active in WASM (Blazor client) projects, with no ability to opt out. These project types should switch to a safe, linker-friendly serialization stack like System.Text.Json.

- Also disallow BinaryFormatter from within wasm projects

src/mono/netcore/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.wasm.xml

GrabYourPitchforks · 2020-07-08T22:41:28Z

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

@@ -141,6 +142,9 @@ 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);


@jkotas @MichalStrehovsky Can you comment on whether this is the right way to run the cctor once AppContext initialization is complete? In a nutshell: I'm trying to capture what the values were immediately upon app start, independent of any future modifications to AppContext itself. See the code comments on SecureAppContext for more info.

I do not think we should be doing measures like this.

Also, a better way to trigger a static constructor is to just call a static method on the type.

I do not think we should be doing measures like this.

If somebody managed to hack this via reflection, reflection calls will run the static constructor first and then fail the attempt to set it.

a better way to trigger a static constructor is to just call a static method on the type.

The static constructor needs to be explicit for this to work.

I don't know much about the BinaryFormatter exploits, but if somebody already owns the process to the extent that they can flip the switches within the normal "unsecure" AppContext (they can invoke arbitrary static methods and provide parameters), I would think their level of interest in BinaryFormatter would be pretty low because there's plenty other surface to attack at that point (WebClient.DownloadFile sounds much more convenient than AppContext.SetSwitch followed by complex BinaryFormatter goo).

jkotas · 2020-07-08T23:36:05Z

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

+    /// process. Arbitrary memory writes can also alter these fields. Both of these scenarios are
+    /// outside the scope of our threat model.
+    /// </remarks>
+    internal static class SecureAppContext


I would make this local to System.Runtime.Serialization.Formatters. I do not think it needs to live in CoreLib.

I disagree, this has the basis of becoming a wider used feature for disabling things, and isn't serialization specific.

If you want this to be a wider used feature for disabling things, this needs to be designed a proper public API/surface. The current shape is not suitable as public API.

At least some part of this needs to live in System.Private.CoreLib, as it's required to be able to trim away the reflection logic in ResourceReader.Core.cs.

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

jkotas · 2020-07-09T00:27:28Z

The tests failing on Mono should be disabled against #37899

jkotas · 2020-07-09T03:39:40Z

Impact to WASM projects

The tests getting enabled in #38948 will need to be disabled again.

cc @akoeplinger

1. Added: (http://web.mit.edu/jemorris/humor/500-miles) 2. Added: JustWatch - The Streaming Guide (https://www.justwatch.com/) 3. Added: Introduce opt-in BinaryFormatter killbit by GrabYourPitchforks · Pull Request #38963 · dotnet/runtime (dotnet/runtime#38963)

src/libraries/System.Runtime.Serialization.Formatters/tests/DisableBitTests.cs

safern · 2020-07-09T21:34:17Z

The tests getting enabled in #38948 will need to be disabled again.

I think we should just skip the whole test assembly on Mono for Browser

EDIT: I thought there was only BinaryFormatter tests but that was a wrong assumption, we should just skip tests that use BinaryFormatter.

MichalStrehovsky · 2020-07-10T08:41:31Z

Carves out some AppContext switches as "sensitive" such that they cannot be overwritten using standard reflection

Depends on what you call "standard reflection". Is this standard reflection?

class FortKnox
{
    private static readonly bool s_unbreakableLock;

    static FortKnox()
    {
        s_unbreakableLock = InsecureJunk.s_insecureFlag;
    }

    public static void Enter()
    {
        Console.WriteLine(s_unbreakableLock ? "Go away" : "Open sesame");
    }
}

class InsecureJunk
{
    internal static bool s_insecureFlag;

    static void Main()
    {
        s_insecureFlag = true;

        FortKnox.Enter();

        s_insecureFlag = false;

        var del = (Action)Delegate.CreateDelegate(typeof(Action), typeof(FortKnox), ".cctor");
        del();

        FortKnox.Enter();
    }
}

I must say I don't know what the SecureAppContext is trying to achieve and what the "Secure" in it tries to imply. It feels equally "secure" as a SecureString.

I would like us to eventually have an API on AppContext that lets us set switches that cannot be changed using public APIs after they're initially set: it would also address the "Stability" section of the Feature switch design. But I don't see the point in going to readonly bools to provide a false sense of this being in some way "secure". I think it would be preferable for this pull request to go in that direction (add internal-only API on AppContext that lets us set immutable switches, with the "normal" immutability guarantees that don't try to give false sense of security (i.e. just put the switches in a separate dictionary or add a flag and if someone private reflects on the dictionary and changes it, so be it).

EDIT: simplified the delegate creation quite a bit

jkotas · 2020-07-10T13:21:04Z

I think it would be preferable for this pull request to go in that direction (add internal-only API on AppContext that lets us set immutable switches

I do not think this should be part of the pull request. If we want a feature like this, it should be designed and approved as a proper public API. I am actually not sure whether it provides enough value. Today, if you set AppContext value mid-flight after somebody cached it, the new value will be simply ignored. With this feature, you would get an exception telling you that the value is ignored.

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

src/libraries/System.Runtime/tests/System/AppContext/SecureAppContext.cs

marek-safar · 2020-07-15T08:09:04Z

...s/System.Runtime.Serialization.Formatters/src/System.Runtime.Serialization.Formatters.csproj

@@ -2,8 +2,10 @@
  <PropertyGroup>
    <AssemblyName>System.Runtime.Serialization.Formatters</AssemblyName>
    <RootNamespace>System.Runtime.Serialization.Formatters</RootNamespace>
-    <TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks>
+    <TargetFrameworks>$(NetCoreAppCurrent);$(NetCoreAppCurrent)-Browser</TargetFrameworks>


I'm not sure this is worth it, IL linker will do the same job with code you already wrote

The split is to throw different exception for the unconditionally disabled variant: #38963 (comment)

I am not sure whether the different exception is worth the complexity for the unconditionally disabled variant. But if we do want the unconditionally disabled variant to be different, we can go all the way and make the whole type disabled: #38963 (comment)

marek-safar · 2020-07-15T08:11:28Z

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.Resources.ResourceReader">
+      <method signature="System.Boolean InitializeBinaryFormatter()" body="stub" value="false" feature="System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization" featurevalue="false" />


It'd be great to add trimming test for this and update documentation at https://github.com/dotnet/runtime/blob/master/docs/workflow/trimming/feature-switches.md#available-feature-switches

@marek-safar You're referring to the "kitchen sink" test project in the sdk repo, right?

I believe Marek is referring to a test like this: https://github.com/dotnet/runtime/tree/master/src/libraries/System.Runtime/tests/TrimmingTests

Yes, what Jan said

jkotas · 2020-07-15T11:13:06Z

src/libraries/System.Private.CoreLib/src/System/Resources/ResourceReader.Core.cs

        {
+            if (!LocalAppContextSwitches.BinaryFormatterEnabled)


Do we really need this? Can this depend on BinaryFormater throwing the exception instead?

If you would like to provide an exception with a better error message, this can catch NotSupportedException thrown by the binary formatter.

Perhaps letting the PNSE / NSE bubble up would be ideal after all. It clearly indicates "I know what you were trying to do, but the current app configuration disallowed it."

We do want the linker to trim all of this away when BinaryFormatter is disabled, though. That's also part of the discussion at #39090. Maybe the best course of action should be to have this method throw an exception with the exact same error message that BinaryFormatter would have thrown.

I think there are two options:

Throw the same exception as what binary formatter throws here

Catch the NotSupportedException from the BinaryFormatter and replace it with a different exception that has better message specific to ResourceReader.

Either way is fine with me.

jkotas · 2020-07-15T11:37:55Z

LGTM otherwise. Thank you!

eerhardt · 2020-07-15T16:21:00Z

@eerhardt @MichalStrehovsky Does the linker understand cross-module substitutions and trimming? That is, if we make a substitution such that some System.Private.CoreLib.dll function always returns false, will the linker be able to const-propagate and trim away System.Runtime.Serialization.FormatterServices.dll call sites that are conditioned on that return value?

Yes. For example, when the EventSource.IsSupported feature is false, we have a substitution that says the EventSource::IsEnabled() method returns false:

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

Lines 3 to 5 in 4cc3750

    
           <type fullname="System.Diagnostics.Tracing.EventSource" feature="System.Diagnostics.Tracing.EventSource.IsSupported" featurevalue="false"> 
        
             <method signature="System.Boolean get_IsSupported()" body="stub" value="false" /> 
        
             <method signature="System.Boolean IsEnabled()" body="stub" value="false" />

And assemblies outside of CoreLib that use EventSource get the const propagated to them and the branches are trimmed as expected.

eerhardt · 2020-07-15T17:03:39Z

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.Resources.ResourceReader">
+      <method signature="System.Boolean InitializeBinaryFormatter()" body="stub" value="false" feature="System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization" featurevalue="false" />


This shouldn't be on ResourceReader, the substitution should be on System.LocalAppContextSwitches::get_BinaryFormatterEnabled(). That way all other code that is conditioned on LocalAppContextSwitches.BinaryFormatterEnabled gets trimmed as well.

We should not need LocalAppContextSwitches::get_BinaryFormatterEnabled in CoreLib at all. That's why I have suggested the substitution to be on InitializeBinaryFormatter

Ah, Jan, I think I see what you mean. One sec while I send a new iteration.

GrabYourPitchforks · 2020-07-15T17:09:45Z

~~I'll back out the PNSE portion since it's causing us to contort the code too much.~~

Edit: Nevermind. Turns out this is the easiest way to make the wasm implementation always throw. Otherwise we have to contort the build to introduce a wasm-specific linker file.

src/mono/netcore/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.wasm.xml

src/libraries/System.Runtime.Serialization.Formatters/src/ILLink/ILLink.Substitutions.xml

GrabYourPitchforks · 2020-07-15T17:59:11Z

@jkotas, I think bcda8cb addresses your feedback. If the linker substitutes "return false" for the Initialize method body, we can't rely on BinaryFormatter.Deserialize to throw an exception (since we can't even call it), so we'll throw an exception with the same text.

If the linker does not substitute "return false", we'll call into BF. And if it throws an exception, so be it.

jkotas · 2020-07-15T18:02:45Z

Yes, this is exactly what I meant. Thanks!

jkotas

I would be ok be having the trimming test in a follow up PR, together with the work to make reflection pattern in ResourceReader understood by the linker. It is tracked by #32862

eerhardt · 2020-07-15T19:35:30Z

src/libraries/System.Private.CoreLib/src/System/Resources/ResourceReader.Core.cs

@@ -50,7 +51,14 @@ private object DeserializeObject(int typeIndex)

            if (_binaryFormatter == null)
            {
-                InitializeBinaryFormatter();
+                if (!InitializeBinaryFormatter())


I'm not seeing this code pattern handled in an optimal way by the linker. Here's the resulting trimmed code:

private object DeserializeObject(int typeIndex) { if (!this._permitDeserialization) { throw new NotSupportedException(SR.NotSupported_ResourceObjectSerialization); } if (this._binaryFormatter == null) { this.InitializeBinaryFormatter(); throw new NotSupportedException(SR.BinaryFormatter_SerializationDisallowed); } Type type = this.FindType(typeIndex); object sDeserializeMethod = ResourceReader.s_deserializeMethod(this._binaryFormatter, this._store.BaseStream); if (sDeserializeMethod.GetType() != type) { throw new BadImageFormatException(SR.Format(SR.BadImageFormat_ResType_SerBlobMismatch, type.FullName, sDeserializeMethod.GetType().FullName)); } return sDeserializeMethod; }

Since the switch is cut off inside of the if (_binaryFormatter == null), the linker won't trim the other code in this method. Being able to trim the rest of the code in this method is nice because other things can be trimmed as well, like s_deserializeMethod. It might not be a huge deal here since the dead code is minimal. But in general we should try to make the linker check optimal so the rest of the method can be trimmed.

@eerhardt Would something like this work? I can send it as a separate PR.

object binaryFormatter = GetBinaryFormatter(); // hardcoded to return null on wasm if (binaryFormatter is null) { throw new PNSE(); } Type type = this.FindType(typeIndex); // remainder of method private object BinaryFormatter() { if (_binaryFormatter is null) { // initialize } Debug.Assert(_binaryFormatter != null); return _binaryFormatter; }

I do not think the linker can stub methods that return object: https://github.com/mono/linker/blob/811c69dfb6e325a344b713890408b137b39a888b/src/linker/Linker.Steps/BodySubstituterStep.cs#L172

I think the easiest way to fix this is to move if (this._binaryFormatter == null) inside InitializeBinaryFormatter. Also, I think it would be fine to defer this to #32862 . The InitializeBinaryFormatter method will need to be rewritten to make it work well with reflection linking and analysis.

Note that #32862 is not planned for 5.0.

I am also fine with deferring this right now. I don't see a lot of code being preserved here since the FindType method is also called elsewhere. So the size savings won't be impactful.

GrabYourPitchforks · 2020-07-16T05:23:06Z

CI failures are known issue: #39404.

GrabYourPitchforks added 4 commits July 2, 2020 18:47

Introduce BinaryFormatter killbit

171a0c0

- Also disallow BinaryFormatter from within wasm projects

Add tests for SecureAppContext

d72c8b2

Add killbit tests

64c3770

Fix license header on newly-added files

a6d2482

GrabYourPitchforks added area-Serialization breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. new-api-needs-documentation labels Jul 8, 2020

GrabYourPitchforks requested review from blowdart, bartonjs and joperezr July 8, 2020 22:39

GrabYourPitchforks requested review from akoeplinger, EgorBo, marek-safar, steveisok and vargaz as code owners July 8, 2020 22:39

GrabYourPitchforks added this to the 5.0.0 milestone Jul 8, 2020

GrabYourPitchforks commented Jul 8, 2020

View reviewed changes

src/mono/netcore/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.wasm.xml Outdated Show resolved Hide resolved

GrabYourPitchforks commented Jul 8, 2020

View reviewed changes

jkotas reviewed Jul 8, 2020

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/SecureAppContext.cs Outdated Show resolved Hide resolved

stephentoub mentioned this pull request Jul 9, 2020

WASM Re-enable Threading.Channels serialization tests #39016

Merged

safern reviewed Jul 9, 2020

View reviewed changes

src/libraries/System.Runtime.Serialization.Formatters/tests/DisableBitTests.cs Outdated Show resolved Hide resolved

bartonjs reviewed Jul 10, 2020

View reviewed changes

src/libraries/System.Runtime.Serialization.Formatters/src/Resources/Strings.resx Outdated Show resolved Hide resolved

bartonjs reviewed Jul 10, 2020

View reviewed changes

src/libraries/System.Runtime/tests/System/AppContext/SecureAppContext.cs Outdated Show resolved Hide resolved

GrabYourPitchforks added 6 commits July 14, 2020 22:27

Split implementation

6b599b2

PNSE implementations

9137e43

Split PNSE into separate file

ac0a46a

Updates

82807b1

Merge master into bf_obs; remove new SerializationInfo public APIs

19b252a

Fix formatting

5e493d6

marek-safar reviewed Jul 15, 2020

View reviewed changes

jkotas reviewed Jul 15, 2020

View reviewed changes

eerhardt reviewed Jul 15, 2020

View reviewed changes

src/mono/netcore/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.wasm.xml Outdated Show resolved Hide resolved

eerhardt reviewed Jul 15, 2020

View reviewed changes

src/libraries/System.Runtime.Serialization.Formatters/src/ILLink/ILLink.Substitutions.xml Show resolved Hide resolved

GrabYourPitchforks added 2 commits July 15, 2020 10:37

Update exception text and illtrim files

af0f08e

Update linker substitutions

bcda8cb

jkotas approved these changes Jul 15, 2020

View reviewed changes

eerhardt reviewed Jul 15, 2020

View reviewed changes

Skip some failing unit tests in wasm

92cd09c

GrabYourPitchforks merged commit 0554117 into dotnet:master Jul 16, 2020

safern mentioned this pull request Jul 16, 2020

Disable System.Runtime tests that use binary formatter on wasm #39487

Merged

This was referenced Jul 17, 2020

[Wasm] Enable System.Data.Common tests #39463

Merged

[wasm] Enable System.Resources.Extensions.Tests and System.Resources.ResourceManager.Tests test suites #39513

Merged

kunalspathak mentioned this pull request Jul 24, 2020

[Perf -35%] System.Memory.Span<Char>.IndexOfValue #39722

Closed

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020

GrabYourPitchforks deleted the bf_wasm branch March 22, 2021 21:13

jkotas added the binaryformatter-migration Issues related to the removal of BinaryFormatter and migrations away from it label May 29, 2024

Introduce opt-in BinaryFormatter killbit #38963

Introduce opt-in BinaryFormatter killbit #38963

Conversation

GrabYourPitchforks commented Jul 8, 2020

Opting in to the killbit via config

Impact to WASM projects

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Jul 9, 2020

jkotas commented Jul 9, 2020

safern commented Jul 9, 2020 • edited Loading

MichalStrehovsky commented Jul 10, 2020 • edited Loading

jkotas commented Jul 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Jul 15, 2020

eerhardt commented Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GrabYourPitchforks commented Jul 15, 2020 • edited Loading

GrabYourPitchforks commented Jul 15, 2020 • edited Loading

jkotas commented Jul 15, 2020

jkotas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GrabYourPitchforks commented Jul 16, 2020

jkotas Jul 8, 2020 •

edited

Loading

safern commented Jul 9, 2020 •

edited

Loading

MichalStrehovsky commented Jul 10, 2020 •

edited

Loading

eerhardt commented Jul 15, 2020 •

edited

Loading

GrabYourPitchforks commented Jul 15, 2020 •

edited

Loading

GrabYourPitchforks commented Jul 15, 2020 •

edited

Loading