Migrated to MessagePack instead of BinaryFormatter #1202

grazy27 · 2025-01-26T13:23:49Z

Overview

This PR replaces BinaryFormatter serialization with the MessagePack library.
I was unable to contribute to #1166, so I’ve opened a new one.

Replacing BinaryFormatter addresses a range of issues, including:

.NET 9 compatibility: Upgrading to .NET 9 requires a separate NuGet package, but this package is incompatible with .NET Framework 4.8, which is also targeted by the project.
Dotnet.Interactive incompatibility: Using Dotnet.Interactive result in a smth like BinarySerializationDisabled exception. The only workaround currently is adding:
```
AppContext.SetSwitch("System.Runtime.Serialization.EnableUnsafeBinaryFormatterSerialization", true);
```
to the beginning of the notebook. It doesn't work each time, it's hard to troubleshoot Dotnet.Interactive, since these exceptions constantly arise and prevent UDFs from execution.
Deprecation: BinaryFormatter has been abandoned and is officially deprecated.

Compatibility Considerations

To ensure better compatibility with existing .NET types and reduce coupling with MessagePack, I opted to retain the Serializable and NonSerialized attributes.

One limitation of MessagePack is its handling of types with constructors containing parameters. If parameter names do not match the corresponding property or field names (e.g., a property _value and a constructor argument value), the parameters will not be automatically matched.

In such cases a default, parameterless constructor is required. If no default constructor is found, MessagePack throws a descriptive exception.

This PR fixes #1131.

Security Considerations

Using MessagePack improves serialization security by addressing common vulnerabilities associated with BinaryFormatter. Key points include:

Safe Deserialization:
- MessagePack does not support arbitrary executable code. It can only serialize and deserialize objects and their properties. Delegates are not supported by design.
- Data exchange occurs exclusively between the Driver and Worker, ensuring that the input being deserialized is the same as what was serialized by the application.
Typeless Mode:
- Typeless mode is only used for deserializing custom objects. For primitives, an optimized serialization format is used, which adds a single byte for type information rather than serializing the entire System.Type.
Restricted Type Resolution:
- The binary payload includes only type descriptions, not definitions.
- This ensures that only types known to the application domain or located in the deserializing side’s probing paths can be deserialized, preventing malicious derived types from executing custom logic.
Mitigation of RCE (Remote Code Execution) Risks:
- MessagePack Enhanced Security Mode is enabled to filter object types and disallow known vulnerable ones.
- A custom filter is implemented to restrict deserialization to:
  - Simple types (primitives, collections, etc.)
  - Types explicitly marked with the [Serializable] attribute.

For reference, here’s an overview paper on potential RCE attack vectors with typeless MessagePack deserialization:
Generating Deserialization Payloads for MessagePack-C# Typeless Mode

…tion logic into a separate class

grazy27 · 2025-01-26T13:25:25Z

src/csharp/Microsoft.Spark.E2ETest.ExternalLibrary/ExternalClass.cs

@@ -9,11 +9,11 @@ namespace Microsoft.Spark.E2ETest.ExternalLibrary
    [Serializable]
    public class ExternalClass
    {
-        private string _s;
+        private string s;


Otherwise messagePack cannot instantiate an ExternalClass instance, as it doesn't know what to use for argument.
We can either create a default parameter-less constructor, or rename field to match ctor argument name.

wudanzy · 2025-02-13T02:55:14Z

/AzurePipelines run

azure-pipelines · 2025-02-13T02:55:41Z

Azure Pipelines successfully started running 1 pipeline(s).

wudanzy · 2025-02-13T02:59:02Z

Just come back from Chinese New Year.

Hi @grazy27, I see #1166 says do not merge this PR. #1166 (comment)

Could you please summarize what was the blocker before and if that has been resolved in this PR?

SparkSnail · 2025-02-14T06:52:42Z

src/csharp/Microsoft.Spark/Microsoft.Spark.csproj

@@ -30,6 +29,7 @@

  <ItemGroup>
    <PackageReference Include="Apache.Arrow" Version="14.0.2" />
+    <PackageReference Include="MessagePack" Version="3.0.214-rc.1" />


Why use the pre-release version?

Thanks for noticing, I didn't see that checkbox 'Include pre-release' was checked

Bumped up to the latest release one

Aha, it seems like we don't have latest msgPack version in the Arcade nuget sources, and we don't have standard nuget.org enabled in nuget.config. And that version must have been the latest available in those feeds.

I'll have a look how to handle it, either choose existing in those sources version or add nuget.org to package sources

Hello, @SparkSnail @wudanzy,

It turns out there's no release MessagePack 3 in feeds recommended by Arcade project.
It seems to be some internal Microsoft strategy, since there's no nuget.org feed in any of dotnet** projects. Example of removing commit

There's this pre-prelease 3.0.214 and release 2.5

There were breaking changes between 3.0 and 2.5, 2.5 has different API and requires additional efforts for making it work - and we'll have to spend more efforts to migrate back up when it appears.

I suggest using pre-prelease 3.0, as since it's already in the feed it must have passed some internal review. And if you know who can be reached out to include release 3.* version to the dotnet-public feed, your help would be appreciated

I see, if there's no security concerns, that's fine to me.

that's fine, we can upgrade it in the future.

grazy27 · 2025-02-15T14:28:47Z

Just come back from Chinese New Year.

Hi @grazy27, I see #1166 says do not merge this PR. #1166 (comment)

Could you please summarize what was the blocker before and if that has been resolved in this PR?

Hello, @wudanzy, welcome back!

The comment not to merge refers to a few reasons:

1. The PR is not fully completed and doesn't function properly

See my review comments inside.

2. A Microsoft dev pointed out that both `BinarySerializer` and `MessagePack.Typeless` are considered RCE-vulnerable and require additional approval from a manager.

For this one, I performed threat modeling and spent extra effort restricting and limiting serialization. Even though we control both serialization and deserialization sides, I ensured that it's not possible to serialize executable code or a custom class that the deserializing part doesn't recognize. Additionally, we can only serialize classes marked as Serializable, which limits known types. The MsgPack blacklist of dangerous classes is explicitly enabled in this PR.

Since Microsoft released a new NuGet package after pr was created, the author suggested skipping this PR entirely.

However, I later found that continuing with BinarySerializer caused too many other issues (see PR description). So, I fixed the existing problems, added additional security measures, changed required attributes from MessagePack to .NET standard ones and raised this PR.

SparkSnail · 2025-02-17T06:32:01Z

/AzurePipelines run

azure-pipelines · 2025-02-17T06:32:29Z

Azure Pipelines successfully started running 1 pipeline(s).

wudanzy

Hi @grazy27, thanks for picking this up and make it proceed!

wudanzy · 2025-02-18T01:41:35Z

src/csharp/Microsoft.Spark/Utils/BinarySerDe.cs

+    public override void ThrowIfDeserializingTypeIsDisallowed(Type type)
+    {
+        // Check against predefined blacklist
+        base.ThrowIfDeserializingTypeIsDisallowed(type);


How this is going to be used?

If there's no 'System.Serializable' attribute defined, we won't allow deserialization. Mimics behavior of binarySerializer

wudanzy · 2025-02-18T01:41:54Z

src/csharp/Microsoft.Spark/Utils/BinarySerDe.cs

+        base.ThrowIfDeserializingTypeIsDisallowed(type);
+
+        // Check if MessagePack can handle this type safely
+        var formatter = StandardResolver.Instance.GetFormatterDynamic(type);


Do we have a unit test to cover this?

Yep

We haven't tested internal MessagePack rules, but that's covered inside messagepack itself

wudanzy · 2025-02-18T08:31:38Z

/AzurePipelines run

azure-pipelines · 2025-02-18T08:32:08Z

Azure Pipelines successfully started running 1 pipeline(s).

Migrated binary serialization to use Messagepack, extracted serializa…

c9d60a6

…tion logic into a separate class

grazy27 commented Jan 26, 2025

View reviewed changes

wudanzy self-requested a review February 13, 2025 02:53

Merge branch 'main' into feature/message-pack-serialization

60e66c3

SparkSnail previously approved these changes Feb 14, 2025

View reviewed changes

SparkSnail reviewed Feb 14, 2025

View reviewed changes

SparkSnail self-requested a review February 14, 2025 06:52

grazy27 dismissed SparkSnail’s stale review via 86a662f February 15, 2025 15:05

wudanzy previously approved these changes Feb 18, 2025

View reviewed changes

grazy27 dismissed wudanzy’s stale review via 60e66c3 February 18, 2025 07:47

grazy27 force-pushed the feature/message-pack-serialization branch from 86a662f to 60e66c3 Compare February 18, 2025 07:47

SparkSnail approved these changes Feb 18, 2025

View reviewed changes

wudanzy approved these changes Feb 19, 2025

View reviewed changes

wudanzy merged commit 3f4cd7c into dotnet:main Feb 19, 2025
54 checks passed

wudanzy mentioned this pull request Feb 19, 2025

Replace BinaryFormatter with MessagePack serialization #1166

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrated to MessagePack instead of BinaryFormatter #1202

Migrated to MessagePack instead of BinaryFormatter #1202

grazy27 commented Jan 26, 2025

grazy27 Jan 26, 2025

wudanzy commented Feb 13, 2025

azure-pipelines bot commented Feb 13, 2025

wudanzy commented Feb 13, 2025

SparkSnail Feb 14, 2025

grazy27 Feb 15, 2025

grazy27 Feb 15, 2025

grazy27 Feb 17, 2025

grazy27 Feb 18, 2025

wudanzy Feb 18, 2025

SparkSnail Feb 18, 2025

grazy27 commented Feb 15, 2025 •

edited

Loading

SparkSnail commented Feb 17, 2025

azure-pipelines bot commented Feb 17, 2025

wudanzy left a comment

wudanzy Feb 18, 2025

grazy27 Feb 18, 2025

wudanzy Feb 18, 2025

grazy27 Feb 18, 2025 •

edited

Loading

wudanzy commented Feb 18, 2025

azure-pipelines bot commented Feb 18, 2025

Migrated to MessagePack instead of BinaryFormatter #1202

Migrated to MessagePack instead of BinaryFormatter #1202

Conversation

grazy27 commented Jan 26, 2025

Overview

Compatibility Considerations

Security Considerations

Choose a reason for hiding this comment

wudanzy commented Feb 13, 2025

azure-pipelines bot commented Feb 13, 2025

wudanzy commented Feb 13, 2025

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

grazy27 commented Feb 15, 2025 • edited Loading

1. The PR is not fully completed and doesn't function properly

2. A Microsoft dev pointed out that both BinarySerializer and MessagePack.Typeless are considered RCE-vulnerable and require additional approval from a manager.

SparkSnail commented Feb 17, 2025

azure-pipelines bot commented Feb 17, 2025

wudanzy 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

grazy27 Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

wudanzy commented Feb 18, 2025

azure-pipelines bot commented Feb 18, 2025

grazy27 commented Feb 15, 2025 •

edited

Loading

2. A Microsoft dev pointed out that both `BinarySerializer` and `MessagePack.Typeless` are considered RCE-vulnerable and require additional approval from a manager.

grazy27 Feb 18, 2025 •

edited

Loading