BinaryFormatter.Deserialize swallows the SecurityException after #35491 fix.

[DmitryGaravsky]

Issue description:
After the source code changes merged in the context of dotnet/corefx#40215 the BinaryFormatter.Deserialize method wraps the SecurityException in the SerializationException:

    try
    {
        var parser = new BinaryParser(serializationStream, reader);
        return reader.Deserialize(parser, check);
    }
    catch (SerializationException)
    {
        throw;
    }
    catch (Exception e) // issue here
    {
        throw new SerializationException(SR.Serialization_CorruptedStream, e);
    }

Expected Behavior:
According to the documentation the BinaryFormatter.Deserialize method must not swallow the SecurityException.
To resolve the current issue, you can re-throw the SecurityException:

    catch (SerializationException)
    {
        throw;
    }
    catch (SecurityException ) // fix
    {
        throw;
    }

Affected .Net Version:
.Net Core 5+ (there is no issue in .Net Core 3.1 LTS and below).

The impact: Breaking Change.
This change breaks Securing deserialization with custom SerializationBinder approach.

[danmoseley]

It does appear from the docs that SecurityException is special here.

[GrabYourPitchforks]

The SecurityException type is very specific to Code Access Security and shouldn't be thrown anywhere within a coreclr-based application. A serialization binder certainly shouldn't be throwing such an exception since it's not involved with Code Access Security.

What's the particular scenario broken here?

Edit: This is akin to the ArgumentNullException scenario. Even though it's documented as an exception that can be thrown, the expectation is that it will only ever be thrown at the immediate call site before deserialization begins since it's essentially a glorified precondition check. We don't otherwise special-case ArgumentNullException instances that occur as part of calling into the deserializer. They'll be wrapped within a normal SerializationException same as everything else.

[DmitryGaravsky]

Hi guys, thank you for participating.
Let me describe a particular scenario in detail.

As you know, the SerializationBinder can be used to safely deserialize some data from an untrusted source. The binder gives us an opportunity to inspect what types are loaded in our application.

So we can have the following environment:

[Serializable]
public class UserSettings { /*...*/ }

This is how the handling of user settings looks like (this is a sample):

try {
    // deserializing from some untrusted source
    var userSettings = SafeBinaryFormatter<UserSettings>.Deserialize(untrustedBytes);
}
catch(SerializationException) {
    // bypass the error,  log an issue and continue with unchanged settings
}
catch(System.Security.SecurityException) {
    // raise panic and stop the execution
}

The SafeBinaryFormatter class is defined as follows:

sealed class SafeBinaryFormatter<T> {
    SafeBinaryFormatter() { }
    readonly static BinaryFormatter safeFormatter = new BinaryFormatter() { Binder = new SafeBinder() };
    public static T Deserialize(byte[] bytes) {
        using(MemoryStream ms = new MemoryStream(bytes))
            return (T)safeFormatter.Deserialize(ms);
    }
    sealed class SafeBinder : SerializationBinder {
        public override Type BindToType(string assemblyName, string typeName) {
            if(typeof(T).FullName == typeName && typeof(T).Assembly.FullName == assemblyName)
                return typeof(T);
            throw new NonTrustedTypeDeserializationException(typeName);
        }
    }
    sealed class NonTrustedTypeDeserializationException : System.Security.SecurityException {
        const string messageFormat = "The {0} type is not trusted and therefore is not deserialized due to security reasons.";
        internal NonTrustedTypeDeserializationException(string typeName)
            : base(string.Format(messageFormat, typeName)) {
        }
    }
}

With the change I mentioned in the context of this issue, the code above is totally broken. Due to them, the real execution flow is changed. Our unit tests are warned about this immediately (we maintain unit tests for all the released and preview environments like .net core 5 in our codebase). Thus it is an important breaking change for us.

At the moment, we have to decide whether to add some deep handling of SerializationException on our side or this is a real BC in .net codebase that will be fixed in the release.
Thank you for your help and understanding.

[GrabYourPitchforks]

As you know, the SerializationBinder can be used to safely deserialize some data from an untrusted source.

This is not an accurate statement.

The BinaryFormatter.Deserialize method must not be called with untrusted data. Our docs are clear on this: "Calling this method with untrusted data is a security risk. Call this method only with trusted data." There's no escape clause which says that using a SerializationBinder allows calling this method with untrusted data. If you do come across documentation stating such please let me know because it's something that needs immediate correction!

Meanwhile I'll mark this as a breaking change so that it bubbles up to the right folks. Thanks for describing your scenario!

[DmitryGaravsky]

If you do come across documentation stating such please let me know because it's something that needs immediate correction!

There is nothing incorrect in the documentation and we'll be the happiest developers in the world if our programs work only in a trusted environment. But in the real world, we have to deal with various data from different sources (databases/communication protocols/file formats/external APIs). The solutions like above appear only because we clearly understand all the risks and do our best to guarantee the required security level for our customers. That is why our customers pay for our work ;-)

Meanwhile I'll mark this as a breaking change so that it bubbles up to the right folks.

Happy to hear and will stay tuned, thank you!

[GrabYourPitchforks]

This will be captured in the breaking change document (see dotnet/docs#19604). Thank you for reporting the issue!