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

Fix S3925 FP: BinaryFormatter serialization is deprecated, should be handled differently for >= .NET 8 #8377

Closed
gregory-paidis-sonarsource opened this issue Nov 17, 2023 · 7 comments
Labels
Area: C# C# rules related issues. Area: C#12 C#12 related issues Type: Improvement Making existing code better.

Comments

@gregory-paidis-sonarsource
Copy link
Contributor

gregory-paidis-sonarsource commented Nov 17, 2023

This is a remainder from preparing for the .NET8 release.
Microsoft has been gradually dropping support for the BinaryFormatter, so the best practises referenced in this rule are different post-NET8.

Initially raised on this community post by Corniel.

See more discussion here (internal click)

S3925 Needs specification.

Relevant information:

Expected behaviour of the rule:

  • Only raise issues when the user explicitly opts-in for serialization of the concrete type by either (see also RSpec)
    • Applying the [Serializable] attribute
    • Having ISerializable in the base type list (it is explicitly mentioned in the current types base type list)
    • Declaring a serialization constructor
  • If the user opts-in for serialization, the old rules apply without modification. The new .Net warnings need to be dealt with by the user as described by Microsoft.
@gregory-paidis-sonarsource gregory-paidis-sonarsource added Type: Improvement Making existing code better. Area: C# C# rules related issues. labels Nov 17, 2023
@martin-strecker-sonarsource
Copy link
Contributor

Other rules affected:

  • S3925 - "ISerializable" should be implemented correctly
  • S2552 - "Serializable" classes should provide a serialization constructor ✅ Rule not implemented
  • S3927 - Serialization event handlers should be implemented correctly ✅ Only triggered on explicit formatting methods
  • S5773 - Types allowed to be deserialized should be restricted ✅ Only triggered on explicit formatting methods
  • S4027 - Exceptions should provide standard constructors Fix S4027 FP: BinaryFormatter. Serialization constructors are obsolete and should not be required #8560
  • S3926 - Deserialization methods should be provided for "OptionalField" members ✅ Only triggered on explicit formatting methods
  • S5135 - Deserialization should not be vulnerable to injection attacks ✅ Only triggered on explicit formatting methods
  • S5766 - Deserializing objects without performing data validation is security-sensitive
  • S1144 - Unused private types or members should be removed
  • S4212 - Serialization constructors should be secured
  • Fulltext RSpec search for "serializable"

A list of types/members annotated as deprecated can be found here:
https://github.com/dotnet/designs/blob/main/accepted/2020/better-obsoletion/binaryformatter-obsoletion.md#new-obsoletions-in-net-8

@andrei-epure-sonarsource andrei-epure-sonarsource added the Area: C#12 C#12 related issues label Nov 17, 2023
@Corniel
Copy link
Contributor

Corniel commented Nov 29, 2023

@andrei-epure-sonarsource officially, this is not C#12 but .NET 8 related.

@DominikAmon
Copy link

Especially when writing custom exceptions that derive from System.Exception.
As System.Exception is still implements System.Runtime.Serialization.ISerializable SonarQube "forces" derived classes to implement the Serialization as well. This is where Roslyn kicks in with SYSLIB0051 - This API supports obsolete formatter-based serialization.

@martin-strecker-sonarsource martin-strecker-sonarsource changed the title S3925: BinaryFormatter serialization is deprecated, should be handled differently for >= .NET 8 Fix S3925: FP BinaryFormatter serialization is deprecated, should be handled differently for >= .NET 8 Jan 17, 2024
@martin-strecker-sonarsource
Copy link
Contributor

The current implementation of S3925 requires the user to opt-in for serialization on the concrete type:

public class MyException : Exception // Compliant: no opt-in for custom serialization
{ }

public class CustomLookup : Dictionary<string, object> // Compliant: no opt-in for custom serialization

If the user opts-in, he needs to follow the old guidelines. This rule already behaves as described in the issue description and the RSpec already mentions the opt-in behavior. There is nothing to change for this rule at the moment.

@cristian-ambrosini-sonarsource cristian-ambrosini-sonarsource changed the title Fix S3925: FP BinaryFormatter serialization is deprecated, should be handled differently for >= .NET 8 Fix S3925 FP: BinaryFormatter serialization is deprecated, should be handled differently for >= .NET 8 Jan 17, 2024
@pavel-mikula-sonarsource
Copy link
Contributor

S4212 has been deprecated, so we can omit that one

@martin-strecker-sonarsource
Copy link
Contributor

@cristian-ambrosini-sonarsource I think we looked at everything here and just forgot to close the issue, right?

@cristian-ambrosini-sonarsource
Copy link
Contributor

Yes, we can close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: C# C# rules related issues. Area: C#12 C#12 related issues Type: Improvement Making existing code better.
Projects
None yet
Development

No branches or pull requests

7 participants