Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider allowing Exception instances to be serialized without requiring BinaryFormatter #43482

Open
GrabYourPitchforks opened this issue Sep 12, 2020 · 58 comments
Assignees
Labels
area-System.Runtime binaryformatter-migration Issues related to the removal of BinaryFormatter and migrations away from it documentation Documentation bug or enhancement, does not impact product or test code
Milestone

Comments

@GrabYourPitchforks
Copy link
Member

Some remoting technologies use BinaryFormatter to serialize Exception instances across security boundaries, which puts them out of SDL compliance and potentially exposes consumers to security vulnerabilities. The current recommended way to serialize exception information safely is to call ToString on the exception instance, then transmit the resulting string across the wire. However, this does not create useful object models for consuming applications, as they can't interact with a simple string like they can a rich exception instance (accessing properties, using try / catch, etc.).

In an ideal world, an exception serialization tech would have the following characteristics:

  • The proper Exception-derived type will be instantiated after deserialization.
  • The human-readable message and stack trace are preserved.
  • The inner exceptions are preserved.
  • Vital information about the exception is preserved. This is normally information passed to the Exception ctor and exposed via properties, such as the argument name provided to an ArgumentOutOfRangeException.

To maintain SDL compliance and work with our linker technology, we'd need to enforce a few extra behaviors:

  • The payload cannot be the sole arbiter of type information. The deserializer needs to provide an allow list of legal Exception-derived types, and the payload cannot attempt to instantiate types outside that allowed list
  • The deserializer tech must go through normal instance validation and type safety checks, normally performed by the exception's ctor. This disallows using the existing SerializationInfo / StreamingContext infrastructure.
  • Cyclic or deeply-nested object graphs must not be constructed. This also disallows using the existing SerializationInfo / StreamingContext infrastructure.

It's possible that the deserialization tech would need to include special-case handling of each allowed Exception-derived type in order to fulfill these requirements. Perhaps this could be simplified by understanding canonical patterns like .ctor(string message, Exception innerException). But we'll cross that bridge when we come to it.

@svick
Copy link
Contributor

svick commented Sep 12, 2020

For those who don't know what some TLAs mean, like me, I think SDL refers to Microsoft Security Development Lifecycle.

@yaakov-h
Copy link
Member

The proper Exception-derived type will be instantiated after deserialization.

What would happen in the case where the deserializing assembly/service/app/whatever does not know about or have access to the appropriate Exception subclass?

@GrabYourPitchforks
Copy link
Member Author

GrabYourPitchforks commented Sep 12, 2020

What would happen in the case where the deserializing assembly/service/app/whatever does not know about or have access to the appropriate Exception subclass?

The case of "not having access" shouldn't be possible with this design. The deserializer needs to have ahead-of-time knowledge about all possible Exception classes that might be instantiated. (This characteristic is true of any secure polymorphic deserialization technology; it's not unique to Exception and derived types.)

This means that deserialization scenarios fall into only two buckets: (1) the deserializer is aware of the requested Exception-derived type and knows how to instantiate it; or (2) the deserializer is not aware of the requested Exception-derived type. There's no middle ground where the deserializer says "well, I found a Type that corresponds to what the payload is requesting, but I don't know how to instantate it."

In the case of the second bucket above, I imagine the deserializer would instantiate a placeholder Exception-derived type and include the original error message and the original stack trace.

@AustinWise
Copy link
Contributor

It would be nice if the new exposed something like .NET Framework's Exception.PreForRemoting method. That way exceptions could be rethrown while preserving the server-side stack trace.

@yaakov-h
Copy link
Member

That still relies on the Remoting infrastructure doing the serialization, and has been somewhat generalized with ExceptionDispatchInfo since .NET 4.5.

Perhaps being able to serialize a ExceptionDispatchInfo would solve this problem?

@AustinWise
Copy link
Contributor

@yaakov-h Thats a good point. EDI gets you most of the way there. All that PrepForRemoting adds is the extra markers in the stack trace that show a remoting boundary was crossed.

@eiriktsarpalis
Copy link
Member

Speaking as a third party serializer author, my main issue with the existing design is that the Exception(Serialization, StreamingContext) constructor provides the only viable mechanism for unmarshalling an exception with all of its runtime metadata intact (Stacktrace, Data, etc.). Consequently, any serializer that needs to implement faithful exception deserialization needs to also support ISerializable. I would personally appreciate a serializer-independent mechanism that would let users augment a newly created exception instance with all necessary metadata, something like an ExceptionDispatchInfoBuilder.

The deserializer tech must go through normal instance validation and type safety checks, normally performed by the exception's ctor. This disallows using the existing SerializationInfo / StreamingContext infrastructure.

Assuming the highly experimental abstract static interface methods ever gets adopted, one could conceive of a type safe and linker-friendly successor to SerializationInfo/StreamingContext constructors. This is largely how languages with type classes implement type-safe serialization.

@GrabYourPitchforks
Copy link
Member Author

A 100% faithful reconstruction of the original Exception instance is not necessarily required. For example, perhaps it's good enough for our serializer to say that it doesn't deal with serializing the Data dictionary. (If we were to try to handle that, we'd become a serializer for arbitrary data, and the .NET ecosystem does not need yet another arbitrary data serializer.) This also means that we'd lose Watson bucket info. But we'd definitely want to keep useful information like the human-readable stack trace around. Restoring that data might involve the creation of new API surface within the runtime.

@olivier-spinelli
Copy link

Fully aggree with the fact that an actual exception is not required. For years we used a simple stupid projection into a "Data" object (factory method is here; https://github.com/Invenietis/CK-Core/blob/master/CK.Core/CKExceptionData.cs#L143). This data is "as serializable as possible" and its only data.
And the information captured appeared to be "good enough" for (as I said) numerous years.

@eiriktsarpalis
Copy link
Member

A 100% faithful reconstruction of the original Exception instance is not necessarily required. For example, perhaps it's good enough for our serializer to say that it doesn't deal with serializing the Data dictionary. (If we were to try to handle that, we'd become a serializer for arbitrary data, and the .NET ecosystem does not need yet another arbitrary data serializer.)

Agreed, although that should be a design decision preferably made by the serializer implementation, rather than something forced by the underlying exception reconstruction mechanism.

@AArnott
Copy link
Contributor

AArnott commented Sep 19, 2020

I am surprised that the requirements in the description do not include a provision for dropping sensitive information from the exception so it's safe for remote parties with an Information Disclosure risk.

@GrabYourPitchforks
Copy link
Member Author

@AArnott I don't think that's a viable requirement. In general it's a bad idea to send any object that contains privileged information through a serializer, as there's too high a risk that the sensitive information might be transmitted. This also runs the risk that for any given Exception object, sensitive information may or may not be disclosed depending on the exact Exception type, which isn't always under the dev's control. This makes the system very difficult to reason about from a security perspective.

Any threat model of this serializer would absolutely include information disclosure as a threat. But I suspect the answer to that will be "this serializer is not intended for use in environments where the recipient is not trustworthy."

@terrajobst terrajobst transferred this issue from dotnet/designs Oct 16, 2020
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Oct 16, 2020
@HongGit HongGit added untriaged New issue has not been triaged by the area owner area-System.Runtime and removed untriaged New issue has not been triaged by the area owner area-Serialization labels Oct 22, 2020
@HongGit
Copy link
Contributor

HongGit commented Oct 22, 2020

Move to runtime, this is not a serialization issue.

@rynowak
Copy link
Member

rynowak commented Nov 9, 2020

We just had a discussion about this in the context of dapr/dotnet-sdk#414

Actor frameworks (or other RPC frameworks) tend to use exceptions of different types to signal different business concerns. There might be a different exception type for a transaction that's rejected for low balance or rejected because the item is out of stock, etc.

Ultimately application code wants to call some functionality on another server and then interact with the result in a strongly-typed way, likely with multiple catch blocks.

For this use case the majority of exceptions would be user-defined. Ideally a solution would support serializing additional user-defined properties as well (can be opt-in).

@jkotas
Copy link
Member

jkotas commented Aug 13, 2024

RuntimeHelpers.GetUninitializedObject creates uninitialized object. It does not run any constructors and it can create invalid instances. Any forward-looking scheme needs to run a proper (public) constructor of the given exception type.

@eiriktsarpalis
Copy link
Member

Agreed, a composition model using constructors is the only feasible scheme for rehydrating exceptions. A minimalist solution might involve us declaring that serializable exceptions are the ones that expose a constructor accepting a message and an inner exception (which many types already have): stack traces can then be set out of band using SetRemoteStackTrace.

@yaakov-h
Copy link
Member

What about derived Exception properties, like HttpRequestException.StatusCode or SqlException.Errors?

@eiriktsarpalis
Copy link
Member

What about derived Exception properties, like HttpRequestException.StatusCode or SqlException.Errors?

We'd want to avoid using something akin to SerializationInfo which can contain arbitrary objects. We could either decide that derived exception data does not get serialized or perhaps only allow exporting of string values (akin to Dictionary<string, string>), forcing formatting/parsing responsibility to the exception author.

@jkotas
Copy link
Member

jkotas commented Aug 14, 2024

We could either decide that derived exception data does not get serialized or perhaps only allow exporting of string values

This is highly opinionated solution that is unlikely to work well in many cases. It can be a sample, but I do not think we would want to ship it as a package from dotnet/runtime.

@AArnott
Copy link
Contributor

AArnott commented Aug 14, 2024

it is tied to now-deprecated BinaryFormatter APIs and

I disagree with this. I have code that serializes and deserializes arbitrary Exception types using the ISerializable interface they implement. I would even argue I do it securely. I don't use BinaryFormatter at all.

I suspect we can go a long way by restricting or removing support for (2). If we can assume that the most important data present in an exception are its type, the message, the inner exception and the stack trace, then a safe design that only guarantees those values being copied could be possible. Naively, it could work like a drop-in replacement for the ISerializable pattern, roughly:

This is almost what I do (in StreamJsonRpc). Except I didn't have to replace ISerializable. I used it as-is. It works great. I'm happy to provide details.

We'd want to avoid using something akin to SerializationInfo which can contain arbitrary objects. We could either decide that derived exception data does not get serialized or perhaps only allow exporting of string values (akin to Dictionary<string, string>), forcing formatting/parsing responsibility to the exception author.

What we do in StreamJsonRpc is just that, IIRC. Any complex object from the Data dictionary is simply serialized as a dictionary of primitives, and therefore they get deserialized as a dictionary of primitives. Perfectly safe that way, while retaining the data.

The only arbitrary object we even instantiate in this system are Exception-derived types. We made the decision to assume that Exception types are generally safe to instantiate (e.g. assume that none of them have harmful finalizers or other behavior). If that turns out to be false, we have a disallow-list that can block it. We also allow for exceptions that don't conform to the ISerializable patterns. In either case (blocked or non-conformant) or if we simply cannot find the exception type in the deserializer, we simply deserialize as System.Exception instead of its derived type.

@osexpert
Copy link

What about derived Exception properties, like HttpRequestException.StatusCode or SqlException.Errors?

SqlException has already adjusted themself to the new world with non-BF serializers like StreamJsonRpc:
https://github.com/dotnet/SqlClient/blob/main/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlException.cs
They do not store errors anymore and fake an Errors-collection (if none):
Line 71: si.AddValue("Errors", null); // Not specifying type to enable serialization of null value of non-serializable type
Line 91: public SqlErrorCollection Errors => _errors ?? new SqlErrorCollection();

IMO this is trying to fix a problem that the community has already discovered/accepted/worked around (SqlException, StreamJsonRpc, others)

Maybe all that is needed is updating the documentation/guidelines to how it is used "De facto".

@eiriktsarpalis
Copy link
Member

it is tied to now-deprecated BinaryFormatter APIs and

I disagree with this. I have code that serializes and deserializes arbitrary Exception types using the ISerializable interface they implement.

I mean this in the sense that SerializationInfo and related APIs are part of the BinaryFormatter namespace and have been marked obsolete. This code might run today

image

but it emits 3 separate warnings which gives a strong signal to users that they should be moving away from it. The problem is, there is currently no general-purpose alternative.

What we do in StreamJsonRpc is just that, IIRC. Any complex object from the Data dictionary is simply serialized as a dictionary of primitives, and therefore they get deserialized as a dictionary of primitives. Perfectly safe that way, while retaining the data.

Which should work as long as the exception type doesn't attempt to downcast to a specific type. It sounds like it might also have problems in the context of trimmed apps or AOT since the serializer can't know the type of the underlying objects at compile time. I think your approach is a step in the right direction, however we need to close the loop so that exception authors also design their types around such APIs that make serialization secure by construction. Only runtime libraries are able to effect this type of change.

ISerializable/SerializationInfo is insecure by design, and think we can do better than simply expecting library/framework authors to avoid the pits of failures present in deprecated APIs.

We made the decision to assume that Exception types are generally safe to instantiate

I recall @GrabYourPitchforks mentioning that the shared framework does contain exception types whose SerializationInfo constructor is vulnerable to remote code execution attacks from malicious payloads. Anything calling SerializationInfo.GetInfo with typeof(object) as an argument should do it -- a cursory search through the codebase suggests that there are a lot of these.

@AArnott
Copy link
Contributor

AArnott commented Aug 14, 2024

I mean this in the sense that SerializationInfo and related APIs are part of the BinaryFormatter namespace

No it isn't. System.Runtime.Serialization.Formatters.Binary.BinaryFormatter and System.Runtime.Serialization.SerializationInfo are not in the same namespace.

and have been marked obsolete.

IMO that's a flaw in how we obsoleted APIs. Folks were over-eager to obsolete all APIs used with BinaryFormatter instead of obsoleting only APIs directly and solely used with BinaryFormatter.
We should fix this by removing the excess obsolete attributes on APIs that are not themselves fundamentally insecure.

Which should work as long as the exception type doesn't attempt to downcast to a specific type.

That's true. Although in all our use with StreamJsonRpc (which is substantial, but tiny on a .NET scale), we've never encountered such a failure.
And actually I should refine my words: if the Exception type itself has a property typed as SomeType, then we'll go ahead and deserialize that. We only reduce complex objects to dictionaries of primitives for objects for the likes of the Data dictionary where the declared type is object. So in practice this problem would only manifest if an Exception-derived type declared an FooBase property and gave it a FooDerived value, and upon deserialization expected FooDerived and downcast it in its deserializing constructor. That seems pretty weird to do though, IMO.

it might also have problems in the context of trimmed apps or AOT since the serializer can't know the type of the underlying objects at compile time

True. Although this could be resolved using something like [KnownTypeAttribute] or a list of expected exception types in the app so that the serializer can pregenerate code to deserialize such types.

ISerializable/SerializationInfo is insecure by design, and think we can do better than simply expecting library/framework authors to avoid the pits of failures present in deprecated APIs.

Can you elaborate on this? Why is it insecure by design? In fact ISerializable is the only pattern I've ever seen for serializers where the intended use is explicitly given to the serializing/deserializing code so it can consider security matters.
If StreamJsonRpc can implement a serializer/deserializer that leverages these APIs securely (as I've described), and we're not really using any of the APIs in a hacky way, it doesn't seem like an insecure API design IMO.

Anything calling SerializationInfo.GetInfo with typeof(object) as an argument should do it -- a cursory search through the codebase suggests that there are a lot of these.

Agreed. That's why StreamJsonRpc doesn't deserialize such objects except as harmless dictionaries of primitives.
The only way I can think of for an injection attack to succeed is by way of primitive values that the executing code then read and interpret in a dangerous way. For example, if the Data dictionary contained a "Commands" key with a format c: string value, and the Exception class blindly executed it, then yes, we'd have a problem.

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Aug 14, 2024

I mean this in the sense that SerializationInfo and related APIs are part of the BinaryFormatter namespace

No it isn't. System.Runtime.Serialization.Formatters.Binary.BinaryFormatter and System.Runtime.Serialization.SerializationInfo are not in the same namespace.

I mean sure, it technically sits in a parent namespace and the same can be said of SerializableAttribute, however both types are intrinsically tied to the flawed design of BinaryFormatter even if they can and have been consumed by other serializers.

ISerializable/SerializationInfo is insecure by design, and think we can do better than simply expecting library/framework authors to avoid the pits of failures present in deprecated APIs.

Can you elaborate on this? Why is it insecure by design?

Because if used as designed SerializationInfo permits storing arbitrary object graphs, including other exceptions and polymorphic values. Additionally, the types of values held by SerializationInfo are specified by the instance itself rather than the caller. A general-purpose serializer/framework looking to support all possible serializable exceptions must necessarily implement unchecked polymorphic (de)serialization, a.k.a. emitting runtime types in the payload, a.k.a. opening themselves up to remote code execution from malicious payloads.

Anything calling SerializationInfo.GetInfo with typeof(object) as an argument should do it -- a cursory search through the codebase suggests that there are a lot of these.

Agreed. That's why StreamJsonRpc doesn't deserialize such objects except as harmless dictionaries of primitives.

Right, but like I said this cannot offer complete support that works in every platform. Maybe if you're an application author that has tight control over what exception types can and cannot be marshalled this works fine, but the calculus changes if you're a library or framework author.

I think we're pretty much on the same page with respect to what is a safe way to serialize exceptions, I'm suggesting we take this one step forward and let exception type authors use the same safe-by-construction APIs in a way that guarantees they are always round-tripable.

@jkotas
Copy link
Member

jkotas commented Aug 14, 2024

ISerializable/SerializationInfo is insecure by design

ISerializable-based serialization implementations have never been hardened against malicious data. Hardening against invalid data was not part of ISerializable-based contract. Many BF exploits took advantage of that.

@jkotas
Copy link
Member

jkotas commented Aug 14, 2024

the calculus changes if you're a library or framework author.

If you are library or framework author, you can provide custom converters that target your serializations scheme for built-in set of exceptions and allow people register their own converters on top of that.

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Aug 14, 2024

the calculus changes if you're a library or framework author.

If you are library or framework author, you can provide custom converters that target your serializations scheme for built-in set of exceptions and allow people register their own converters on top of that.

True, but that just means more ad-hoc and mutually inconsistent solutions. In practice the path of least resistance is falling back to ISerializable, as is the case with orleans (and I presume Akka.NET). Maybe we could just decide that we're fine by this, in which case we can close the issue (and potentially un-deprecate SerializationInfo like @AArnott suggested).

@jkotas
Copy link
Member

jkotas commented Aug 14, 2024

In practice the path of least resistance is falling back to ISerializable

How are you going to ensure that this is secure? As I have said above, ISerializable-based serialization implementations have never been hardened against malicious data.

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Aug 14, 2024

In practice the path of least resistance is falling back to ISerializable

How are you going to ensure that this is secure? As I have said above, ISerializable-based serialization implementations have never been hardened against malicious data.

We can't. I'm saying this is the de facto standard way for exception serialization in the ecosystem and that it might be in our interest to provide a secure alternative.

@AArnott
Copy link
Contributor

AArnott commented Aug 15, 2024

ISerializable-based serialization implementations have never been hardened against malicious data.

I'm curious if there's a single concrete example of an ISerializable Exception that is vulnerable or would likely make an application vulnerable to malicious data given the safeguards StreamJsonRpc put in place. In particular:

  1. Arbitrary objects deserialized limited to Exception-derived types.
  2. Programs typically only use an Exception's type, and sometimes some error code, control execution flow.
  3. Programs almost always ignore the Data collection.

If we have an example of an exploitable vulnerability in these conditions, I'd sympathize perhaps with simply not trusting Exceptions pre-existing ISerializable implementations. But if we can't find one, I'm afraid we're throwing the baby out with the bathwater.

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Aug 15, 2024

ISerializable-based serialization implementations have never been hardened against malicious data.

I'm curious if there's a single concrete example of an ISerializable Exception that is vulnerable or would likely make an application vulnerable to malicious data

ISerializable forces a trade-off between being vulnerable and being feature-complete. Consider the following example that isn't entirely unheard of:

class MyCoolException : Exception, ISerializable
{
    private readonly SomeOpaqueType _data;
    public MyCoolException(string message) : base(message) 
    {
        _data = new SomeOpaqueType(message);
    }

    public MyCoolException(SerializationInfo info, StreamingContext context) : base(info, context) 
    {
        _data = (SomeOpaqueType)info.GetValue("Data", typeof(SomeOpaqueType));
    }

    void ISerializable.GetObjectData(SerializationInfo info, StreamingContext context)
    {
        info.AddValue("Data", _data);
    }

    record SomeOpaqueType(string value);
}

Because the deserialization constructor expects the entry for "Data" to be of a specific type, it will fail unless the SerializationInfo provider has already determined that "Data" should have been deserialized as that type. Since it is not possible to know ahead of time what types a particular deserialization constructor might require, this information needs to be incorporated in the payload itself, and could involve any type present in the current assembly load context.

This is precisely what creates the conditions for remote code execution, as an imaginative attacker can craft payloads using readily available types from the shared framework that end up spawning calc.exe as a side-effect. ysoserial.net contains a lot of examples showing how this can be achieved and even comes with a malicious payload generator for a host of vulnerable serializers.

@KalleOlaviNiemitalo
Copy link

Because the deserialization constructor expects the entry for "Data" to be of a specific type, it will fail unless the SerializationInfo provider has already determined that "Data" should have been deserialized as that type.

Can the provider implement a System.Runtime.Serialization.IFormatterConverter whose object Convert(object value, Type type) method converts from some other representation to SomeOpaqueType?

@eiriktsarpalis
Copy link
Member

I think that might be possible, yes. Although I think it would still be challenging to support in AOT.

@AArnott
Copy link
Contributor

AArnott commented Aug 15, 2024

Because the deserialization constructor expects the entry for "Data" to be of a specific type, it will fail unless the SerializationInfo provider has already determined that "Data" should have been deserialized as that type. Since it is not possible to know ahead of time what types a particular deserialization constructor might require, this information needs to be incorporated in the payload itself

I don't understand why this must be the case.
Consider this part of your code:

info.GetValue("Data", typeof(SomeOpaqueType));

That right there provides the type that is expected to be deserialized. That isn't data-driven -- that's right there in compiled code. StreamJsonRpc can use that to deserialize "Data", allowing the cast to succeed.

Can the provider implement a System.Runtime.Serialization.IFormatterConverter whose object Convert(object value, Type type) method converts from some other representation to SomeOpaqueType?

Yes. That is exactly what StreamJsonRpc does. Essentially we defer deserialization of certain fragments until we know what type was expected -- not by the data but by the trusted code itself.

As for AOT, yes this requires reflection (or at least some hints as to which types should be prepared for deserialization). AOT represents a very small portion of all .NET code, so while it's great that it's support is growing, I'm not sure we should throw out a system that gives us support to serialize nearly all exceptions for something that will only support a small subset of exceptions after changes are made to them in order to support AOT which is also a tiny subset.

If we can agree that this ISerializable approach is adequate for exception serialization, we can move on to figuring out the best way to make this shine in AOT environments too.

@jkotas
Copy link
Member

jkotas commented Aug 15, 2024

If we can agree that this ISerializable approach is adequate for exception serialization,

I do not think that ISerializable approach can be the base for safe exception serialization. You cannot make an assumption that BF deserialization constructor of every exception type out there is hardened for untrusted input. The problem exists even when the payload is limited to primitive types. You can have:

int data = (int)info.GetValue("Data", typeof(int))!;
if (data == 42)
{
     hang or crash
}

Of course, the code is not going to be written like this, the actual logic would be more complicated, but the net effect can be the same.

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Aug 15, 2024

I'm not sure we should throw out a system that gives us support to serialize nearly all exceptions for something that will only support a small subset of exceptions after changes are made to them in order to support AOT which is also a tiny subset.

I don't think anybody proposed we remove ISerializable, it wouldn't be feasible given all the code that depends on it. The issue fundamentally is that ISerializable is insecure by design. Yes, the common risks can be mitigated by jumping through a lot of hoops but it's not something that you get in the path of least resistance, which is why I think we are correct to warn when those APIs are being used.

If we can agree that this ISerializable approach is adequate for exception serialization, we can move on to figuring out the best way to make this shine in AOT environments too.

Would it be possible to build some of the StreamJsonRpc mitigations into SerializationInfo directly? (either via a new constructor or some derived type?).

The problem exists even when the payload is limited to primitive types. You can have:

@jkotas sure, but doesn't this type of risk also exist in modern serializers? I can easily see the same type of problem occurring in a user-defined JsonConverter.

@AArnott
Copy link
Contributor

AArnott commented Aug 15, 2024

Of course, the code is not going to be written like this, the actual logic would be more complicated, but the net effect can be the same.

Fair enough. But that isn't a BF-specific vulnerability at that point. It sounds like you're making the case that "all existing deserializers may have some bug somewhere that could be exploited, so we should rewrite all of them" but I see no reason to believe that any rewrite would be anything other than as-likely to have security bugs like what you suggest here.

I don't think anybody proposed we remove ISerializable

What I'm concerned about is not that the interface may be removed, but rather that exception types defined in .NET will stop implementing it, or start throwing from their implementations. And that future exception types defined will not support it. And that the guidance is for 3rd parties to stop implementing such interfaces and deserializing constructors. That will undermine the goodness that we have with StreamJsonRpc.

The issue fundamentally is that ISerializable is insecure by design. Yes, the common risks can be mitigated by jumping through a lot of hoops but it's not something that you get in the path of least resistance,

I still am not seeing this. I don't see that StreamJsonRpc jumped through a lot of hoops. We just implemented it cautiously. And it was a relatively small amount of code.
If .NET's guidance was simply: implement your deserializing constructors carefully, considering the Context where necessary to assess trust level, I think we'd tap into a lot of value that is that nearly every Exception already does it, and does it right.

@jkotas
Copy link
Member

jkotas commented Aug 15, 2024

@jkotas sure, but doesn't this type of risk also exist in modern serializers?

Custom converters for modern deserializers are expected to be written against modern security requirements. It includes things like explicit allow-lists of types that have been hardened for untrusted input. If you see a type that implements ISerialiable, it is impossible to tell whether the ISerializable implementation has been hardened against untrusted input.

If .NET's guidance was simply: implement your deserializing constructors carefully, considering the Context where necessary to assess trust level, I think we'd tap into a lot of value that is that nearly every Exception already does it, and does it right.

This guidance has been non-existent for ISerializable-based contracts. I do not think that it is acceptable to apply it retroactively.

goodness that we have with StreamJsonRpc.

I am not convinced that the StreamJsonRpc scheme is secure. The scheme is probably acceptable for closed system like VS cross-process communication where actively malicious input is less of a concern. I do not think it would be acceptable for general use for arbitrary end points that have to be hardened against actively malicious input.

@eiriktsarpalis
Copy link
Member

The issue fundamentally is that ISerializable is insecure by design. Yes, the common risks can be mitigated by jumping through a lot of hoops but it's not something that you get in the path of least resistance,

I still am not seeing this. I don't see that StreamJsonRpc jumped through a lot of hoops. We just implemented it cautiously.

The point I'm trying to make is that unless you are a serialization expert that is keenly aware of the security ramifications, you will most likely end up implementing things using the readily available, insecure way (a.k.a. passing in FormatterConverter that does not support non-trivial delayed type conversions). A search using grep.app shows many occurrences of this, including major frameworks like orleans or akka.net. It is also what is recommended by many SO answers and ChatGPT.

The scheme is probably acceptable for closed system like VS cross-process communication where actively malicious input is less of a concern.

I think it stands to reason that we only recommend exception deserialization for applications not expected to handle untrusted input. This is true for the aforementioned orleans and akka.net that essentially implement remoting across distributed compute clusters.

@jkotas
Copy link
Member

jkotas commented Aug 16, 2024

applications not expected to handle untrusted input.

The BF security guide explains the risks of such assumptions: https://learn.microsoft.com/en-us/dotnet/standard/serialization/binaryformatter-security-guide#the-risks-of-assuming-data-to-be-trustworthy .

@eiriktsarpalis
Copy link
Member

Sure, I'm just pointing out that these systems are designed to accept requests that execute remote code and that should factor into their threat modelling.

@jeffhandley
Copy link
Member

The documentation we create for this needs to also incorporate the questions and scenarios from #101159, which I just closed as a duplicate of this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Runtime binaryformatter-migration Issues related to the removal of BinaryFormatter and migrations away from it documentation Documentation bug or enhancement, does not impact product or test code
Projects
None yet
Development

No branches or pull requests