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

Replace binary formatter #8532

Merged
merged 87 commits into from
Jul 19, 2024
Merged

Conversation

rchauhan18
Copy link
Contributor

@rchauhan18 rchauhan18 commented Dec 13, 2023

Description

The Binary Formatter was initially developed prior to the comprehensive understanding of deserialization vulnerabilities as a significant threat. As a result, the code doesn’t follow the modern practices. Due to the security vulnerability Binary formatter is considered obsolete. In .NET 9.0, the entire Binary formatter infrastructure will be removed from the product.

Binary Reader and Writer can be used to read and write the primitive data types into binary data and does not have any security vulnerability. We are creating a safe handle ‘BinaryFormatWriter’ which handles all primitives, Enums, strings, arrays and lists of primitives, and other items before falling into the process of serialization/deserialization. For serialization/deserialization we will use Binary writer. This approach will allow us to disable Binary formatter for the primitive and some other cases.
To convert the object to binary data we call TryWriteFrameworkObject with a memory stream and the object.
BinaryFormattedObject class is added to parse a binary formatted object from a stream. It used Binary reader to read from a stream.
This solution is similar as in WinForms
PR: dotnet/winforms#9088
CC
@JeremyKuhne @lonitra

image

Customer Impact

In .NET 9.0, the entire Binary formatter infrastructure will be removed from the product, and it will become obsolete.
It has additional benefits of making applications more friendly to linker trimming.

Regression

N/A

Testing

Internal testing done.

Risk

Low
There might be a few instances where this solution won't be effective. Currently, this PR includes all the primitive types and few more data types. The plan is to handle additional cases as feedback comes in.

Microsoft Reviewers: Open in CodeFlow

@ghost ghost assigned rchauhan18 Dec 13, 2023
@ghost ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Dec 13, 2023
@ghost ghost requested review from dipeshmsft and singhashish-wpf December 13, 2023 09:27
@ghost ghost added the draft label Dec 13, 2023
@JeremyKuhne
Copy link
Member

To be clear, BinaryFormatter APIs will never be removed. Setting a switch in .NET 9 to enable the BinaryFormatter will likely become required for WPF and WinForms (as it is already for all other .NET projects in .NET 8). Enabling the functionality will possibly require additional steps in the future, but the surface area should always be there.

@wjk
Copy link
Contributor

wjk commented Dec 13, 2023

To be clear, BinaryFormatter APIs will never be removed.

Then it is my mistake for misunderstanding the “Customer Impact” section of the OP. Sorry to bother you.

@michmela44
Copy link

To be clear, BinaryFormatter APIs will never be removed.

While the APIs may never be removed, according to BinaryFormatter infrastructure removed from .NET (.NET 9), "The internal BinaryFormatter implementation will no longer exist in .NET in any form and cannot be reenabled through any compat switch."

@JeremyKuhne
Copy link
Member

To be clear, BinaryFormatter APIs will never be removed.

While the APIs may never be removed, according to BinaryFormatter infrastructure removed from .NET (.NET 9), "The internal BinaryFormatter implementation will no longer exist in .NET in any form and cannot be reenabled through any compat switch."

The hope is to make you jump through more hoops by putting the actual implementation in another package, but there is no way the API will be removed or there will be no way to re-enable for quite some time. It is fundamental to many existing applications, particularly WinForms.

To be clear, one should avoid the BinaryFormatter if at all possible, but as a platform we will not completely abandon existing users of this functionality.

JeremyKuhne
JeremyKuhne previously approved these changes Dec 14, 2023
Copy link
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. You should consider ensuring you have test coverage with and without the BinaryFormatter being enabled. See https://github.com/dotnet/winforms/blob/main/src/Common/tests/TestUtilities/BinaryFormatterScope.cs for how we do this in WinForms.

@rchauhan18 rchauhan18 removed the draft label Dec 27, 2023
@rchauhan18 rchauhan18 marked this pull request as ready for review December 27, 2023 10:53
@rchauhan18 rchauhan18 requested a review from a team as a code owner December 27, 2023 10:53
@rchauhan18 rchauhan18 requested a review from a team as a code owner July 11, 2024 16:48
@rchauhan18 rchauhan18 changed the base branch from main to release/9.0-preview6 July 11, 2024 16:50
@rchauhan18 rchauhan18 changed the base branch from release/9.0-preview6 to main July 11, 2024 16:50
@rchauhan18
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rchauhan18
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines failed to run 1 pipeline(s).

JeremyKuhne
JeremyKuhne previously approved these changes Jul 12, 2024
Copy link
Member

@JeremyKuhne JeremyKuhne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, you'll want to make sure to manually test that handled types work without the BinaryFormatter.

@rchauhan18
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@rchauhan18 rchauhan18 merged commit f29acba into dotnet:main Jul 19, 2024
8 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.