Remove BinaryFormatter from the shared framework in .NET 5

[GrabYourPitchforks]

We've known for a long while that BinaryFormatter is a dangerous type and that it's responsible for many of the security vulnerabilities real-world .NET applications have seen in recent years. In .NET Core 1.x, the type was removed from the product entirely, and it was brought back in .NET Core 2.0 under the guise of "app compat".

Though as of the time of this writing there are no known types in .NET Core which can be leveraged by BinaryFormatter to achieve RCE, it is impractical to make this guarantee going forward. As new types are introduced (or existing types from the full Framework are re-introduced) I have no doubt that one of them will contain such a vector that we weren't able to catch during development due to the complexity of this system.

Some applications use a custom SerializationBinder to mitigate attacks both on .NET Core and in full Framework. The engineering team's current understanding is that a properly-written SerializationBinder is sufficient to block RCE attacks. However, this is not a guarantee, and even so, a custom SerializationBinder cannot mitigate other attacks like DoS within the BinaryFormatter infrastructure itself.

All of this said, the conclusion is that BinaryFormatter is not an appropriate serialization format for application developers to be using in a cloud-connected world. It was a mistake for us to bring it back in .NET Core 2.0, as us re-introducing it into the shared framework gives the type an undeserved air of respectability, like we're nodding and implicitly approving its usage by modern applications.

To address this, and to eventually migrate applications off of BinaryFormatter, I propose the following steps. These steps will only affect customers who target .NET 5 or above for their applications.

1. Remove BinaryFormatter from the shared framework and into its own standalone package that must be manually retrieved and installed into the application.

2. Mark the BinaryFormatter type itself with [Obsolete], or include in the package a code analyzer which warns on the type's usage. Force developers to explicitly acknowledge that they're calling an extremely dangerous API and that they must be certain the data is coming from a trusted location.

3. Document the BinaryFormatter code base as legacy only. Clearly communicate that Microsoft is making no further investments in this code base, and all bug reports or feature requests will be immediately closed.

4. Create a separate "safe" (type-limiting, bounds-checking, etc.) serializer whose payload format is BinaryFormatter-compatible and which can be used to deserialize existing payloads in a safe fashion, even if those payloads originate from an untrusted source. The API surface will look different than the traditional BinaryFormatter.Deserialze call due to needing to configure the serializer on a per-call basis, but at minimum it gives applications a way to read existing payloads. This allows them to migrate their data without leaving them hanging. This serializer could be in the same package that BinaryFormatter is in.

These steps would serve to modernize the runtime and SDK by eliminating a type which has no place in today's development cycle, it serves as a forcing function to get application developers to fix potential vulnerabilities in their code, and it provides on an opt-in basis a way for applications to continue to work if they really must continue using the type going forward.

[blowdart]

[Symbai]

BinaryFormatter has the advantage that it's incredibly fast. Faster than any other formatter, by a lot. If you consider to remove it, please consider to provide a reasonable alternative, which is fast and secure. If you do, then I don't see any reason to not agree with that, regardless whether my personal opinion counts or not.

[NickCraver]

@Symbai I think a very important thing to keep in mind here is that BinarySerializer is not fast and secure. It is fast largely because it is not secure.

[GrabYourPitchforks]

@Symbai If you're looking for something where the payload format is human-readable, check out the work taking place in this repo in the System.Text.Json namespace. Or if you want pure performance over human readability, you may want to check out Google's ProtoBuf project, which has C# APIs.

In the proposal I have in this issue, I do suggest that we should create a BinaryFormatter-compatible "safe" deserializer, but performance wouldn't be a driving factor of such a serializer. (Though we wouldn't do anything to intentionally tank performance.) One of the other formats might be more appropriate for your needs given this.

[ChrisMcKee]

Pushing it into its own package seems a reasonable compromise.

[tebeco]

is it a good idea to refer to benaadams/System.Ben#107
for "fast and secure" binary formatter ?

Too soon ?

[samsosa]

Or if you want pure performance over human readability, you may want to check out Google's ProtoBuf project, which has C# APIs.

Than maybe replace the BinaryFormatter implementation under the hood with ProtoBuf?

[ViktorHofer]

I'm sure you are considering partners here which are relying on BinaryFormatter in .NET Core. Keep in mind to loop them in whenever you are starting this effort.

[mgravell]

@samsosa just to be explicit re:

Than maybe replace the BinaryFormatter implementation under the hood with ProtoBuf?

That isn't a realistic option. Protobuf is a powerful tool, but it has a different feature-set to BinaryFormatter, and there is no good way of making one pretend to be the other, in either direction. Just like XmlSerializer, Json.NET etc are all fundamentally serializers, but don't have he exact same feature set. If people want to use protobuf, that's fine, but they'd need to do that knowingly and deliberately.

[mconnew]

I would like to try an put a stop to the trend I'm seeing in multiple issues of ProtoBuf being the solution to all things serialization. It is not a good serialization format, and it was never designed to be one. It is designed to be a compact way to represent a limited set of features for sending an object graph to a gRPC endpoint. It has very severe limitations when trying to be used for general purpose serialization.

It doesn't have support for references so if an object appears in an object graph multiple times, it will be completely serialized multiple times. We've had customers get tripped up by this before when they overrode Equals or GetHashCode incorrectly and ended up with two separate objects on deserialization which caused application logic problems.

The lack of references means it can't handle cycles in your object model which will cause infinite recursion on serialization. It simply cannot handle that scenario at all.

There have been attempts to come up with a way to support references to allow for these two scenarios. The one that protobuf.net does needs explicit opt-in for the types/members which need this support. This requires understanding your entire object model and know where cycles might exist and when having duplicates will be a problem. This could be a LOT of work depending on how complex your data model is. It greatly complicates your on-wire representation and none of the benchmarks I've seen have been against an OM using these features. Any benchmarks on size and speed of serialization need to take this into account. My understanding is that the Google implementation of gRPC serialization doesn't support this feature and expects you to support this mechanism at your OM level and not at the serializer level.

As the response from the ProtoBuf team has been that they aren't going to build this capability into the protocol and go implement your own solution, you lose the interoperability that ProtoBuf is supposed to bring as there will always be competing ways to do this by different ProtoBuf libraries.

Protobuf is a bad format for use with an existing set of classes. It's best use is when designing a new OM with these limitation in mind. E.g. I'm creating a new gRPC endpoint and so define my data model using IDL and then convert that to classes. Going the reverse direction is going to be problematic for many people as Protobuf is not a general purpose serialization format.

Incidentally, I read a blog post comparing MessagePack, ProtoBuf, JSON.NET and DataContractSerializer comparing size of output and speed of serialization and deserialization where it showed DataContractSerializer to be the worst on both fronts. I cloned their code and added a variation for DataContractSerializer using a binary XmlDictionaryReader/Writer and the size was about 10% bigger than ProtoBuf, and the time to serialize and deserialize was about 10% slower than ProtoBuf. If you were to have multiple duplicate objects and you turned on the reference support in DCS, you could easily create a case where DCS is both faster and smaller than ProtoBuf.

[GrabYourPitchforks]

@mconnew Thank you for the excellent analysis! You're right - I should have been more careful with my wording in my earlier comment. I didn't intend to suggest that ProtoBuf is The One True Replacement(tm) going forward, just that it's one of many options available to developers depending on their scenarios.

I think part of the solution is that we need to convince the ecosystem that a general purpose serializer that can handle arbitrary object graphs is not a sustainable concept. Serializers like DataContractSerializer handle this to a limited extent, but they also place restrictions on the shape the graph must take and limits on what deserialization looks like. Even support for cyclic object graphs is inherently unsafe unless the application explicitly expects to deserialize such cycles, as DataContractSerializer allows on an opt-in basis.

Whether this is a documentation issue that gives best practices for creating DTOs which can be safely serialized, whether we recommend switching to a safe but restricted-capability serializer like DataContractSerializer, or whether we take some other course of action are all up in the air. (It's also possible that we do nothing, but clearly I'm opposed to that idea since I opened this issue. 😊)

Another option - though one that might be more difficult to swallow since it'll require significant work within the ecosystem - would be to create a set of educational guidelines that explains in detail the concepts behind serialization, trust boundaries, and how the two interact. This would make individual developers responsible for analyzing their specific scenarios and choosing appropriate data models and serialization formats which are appropriate for their needs, while also giving them the knowledge to make trade-offs when necessary. I'm not a fan of this solution by itself because it presents a steep learning curve for our audience and goes against .NET's general premise of creating usable frameworks. But perhaps these educational guidelines could be useful in conjunction with another more approachable solution.

[mconnew]

@GrabYourPitchforks, my comment was in response to my seeing multiple places where people were suggesting using ProtoBuf for serialization. It's NOT a serialization format and I just wanted to make sure I put my thoughts out there somewhere contextually relevant.
I believe that DataContractSerializer does about the best job it's possible to do with serializing. We could do with adding some helpers to enable the use of the binary XmlDicationaryReader/Writer as the dictionary needs to be transmitted OOB otherwise you pay the high cost of the Text output.

[mgravell]

@mconnew I don't want to distract from the conversation about BinaryFormatter, so I don't suggest "here", but I'd love to have an offline conversation with you about what does and doesn't qualify as a "serialization format" and why; it seems we have some different ideas there, and I'd love to hear a new perspective. I'm easy to get hold of if you're willing :)

[MarcoRossignoli]

but I'd love to have an offline conversation with you about what does and doesn't qualify as a "serialization format" and why;

IMO would be useful for all devs have an "open" discussion/listening on it...all devs have to serialize something sometimes in career and the hide issues and choices are not so trivial.
Maybe a linked "discussion" to this issue.
Not mandatory though.

[mgravell]

@MarcoRossignoli great idea; I've dropped a discussion here; I'd love your input there if you have the time, @mconnew