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

Remove BinaryFormatter usage from SettingsPropertyValue #39295

Closed
GrabYourPitchforks opened this issue Jul 14, 2020 · 2 comments · Fixed by #50531
Closed

Remove BinaryFormatter usage from SettingsPropertyValue #39295

GrabYourPitchforks opened this issue Jul 14, 2020 · 2 comments · Fixed by #50531
Assignees
Milestone

Comments

@GrabYourPitchforks
Copy link
Member

Ref:

using (MemoryStream ms = new MemoryStream((byte[])SerializedValue))
{
value = (new BinaryFormatter()).Deserialize(ms);
}

using (MemoryStream ms = new MemoryStream(buffer))
{
return (new BinaryFormatter()).Deserialize(ms);
}

using (MemoryStream ms = new MemoryStream())
{
BinaryFormatter bf = new BinaryFormatter();
bf.Serialize(ms, _value);
return ms.ToArray();
}

This issue tracks the removal and replacement of this code per the BinaryFormatter obsoletion plan.

For context: Reading from local configuration is generally perceived to be a "safe" operation. However, we have seen cases where the config APIs are used in multi-tenant environments, reading values from arbitrary XML files. The same thing hit ResourceReader a few years back. A popular web service allowed non-admins to upload .resx files, and when the web service tried parsing it it allowed RCE within the context of the service.

@ghost
Copy link

ghost commented Jul 14, 2020

Tagging subscribers to this area: @safern
Notify danmosemsft if you want to be subscribed.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Jul 14, 2020
@safern safern removed the untriaged New issue has not been triaged by the area owner label Jul 14, 2020
@ghost ghost added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Feb 10, 2021
@jeffhandley jeffhandley modified the milestones: Future, 6.0.0 Feb 10, 2021
@joperezr joperezr removed the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Feb 10, 2021
@pgovind
Copy link

pgovind commented Feb 10, 2021

I think this is a fairly easy library to turn off binary formatter support. Here's my proposal:

There already exists a SettingsSerializeAs.Xml enum. So, let's mark SettingsSerializeAs.Binary as Obsolete for .NET 6. Further, I want to throw an exception when SettingsSerializeAs.Binary is used unless EnableUnsafeBinaryFormatterSerialization is set in the csproj.

@GrabYourPitchforks, @ericstj, area-owners, thoughts?

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 1, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 13, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

6 participants