Add warning for using BinaryFormatter in GenerateResource on .NET 8 #8524
Conversation
It will be removed in .NET 9; doing so should be discouraged. Note that this does nothing by default, but we can change that in the SDK.
Forgind
changed the title
| // Warn of BinaryFormatter exposure (on by default in .NET 8+) | ||
| if (logWarningForBinaryFormatter) | ||
| { | ||
| log.LogWarningWithCodeFromResources("GenerateResource.BinaryFormatterUse", name, resxFilename, mimetype); |
There was a problem hiding this comment.
I think typename is of more interest than mimetype here. The user likely didn't set the mimetype but let a tool specify that by serializing an object.
On the flip side, it's conceivable that a given object is serializable both via binaryformatter and typeconverter so a different representation of the same object wouldn't throw. Hmm. @merriemcgaw do you have a preference (or a suggestion for someone who might)?
There was a problem hiding this comment.
I do not personally, but I wonder if @JeremyKuhne and/or @ericstj might have thoughts. My first thought would be to agree typename is more interesting but you bring up an interesting case.
There was a problem hiding this comment.
Since it's for developers why not include both?
There was a problem hiding this comment.
MimeType here can be one of 3 values, 2 of which haven't been used since like .NET 1.0 days so its probably not all that useful except to help the user know why the resource was being flagged. Probably could be left out to make a less scary warning and just mentioned in the documentation for the diagnostic ID. Agreed that typename is much more useful.
There was a problem hiding this comment.
@ericstj is right -- I forgot those were legacy
| // Warn of BinaryFormatter exposure (on by default in .NET 8+) | ||
| if (logWarningForBinaryFormatter) | ||
| { | ||
| log.LogWarningWithCodeFromResources("GenerateResource.BinaryFormatterUse", name, resxFilename, mimetype); |
There was a problem hiding this comment.
I'd initially intended it to just never be null, but then I broke my own invariant when tests came along and made a narrower and more confusing invariant. I'll change this.
| // Warn of BinaryFormatter exposure (SDK should turn this on by default in .NET 8+) | ||
| if (logWarningForBinaryFormatter) | ||
| { | ||
| log?.LogWarningWithCodeFromResources("GenerateResource.BinaryFormatterUse", name, resxFilename, typename); |
There was a problem hiding this comment.
Put resxFilename in location instead of in message
Try to find line number of resource using bf
Not fully tested, but looks promising
| @@ -1161,6 +1161,11 @@ | |||
| <value>MSB3824: In order to build with .NET Core, resource inputs must be in .txt or .resx format.</value> | |||
| <comment>{StrBegin="MSB3824: "}</comment> | |||
| </data> | |||
| <data name="GenerateResource.BinaryFormatterUse"> | |||
| <value>MSB3825: Resource "{0}" of type "{2}" is deserialized via BinaryFormatter at runtime. BinaryFormatter is deprecated due to possible security risks and will be removed with .NET 9. If you wish to continue using it, set property "GenerateResourceWarnOnBinaryFormatterUse" to false. | |||
| More information: https://learn.microsoft.com/dotnet/standard/serialization/binaryformatter-security-guide</value> | |||
There was a problem hiding this comment.
Isn't there a helplink field on the warning? can we set that too?
There was a problem hiding this comment.
This sounds familiar, but I'm having trouble finding what I'd expected.
Searching for "helplink" didn't return anything helpful.
Searching for "help" in the TaskLoggingHelper revealed that there's a HelpKeywordPrefix (generally just set to MSBuild) that's added to the messageResourceName, so no action there, but I don't think that's what you meant.
Searching for "help" in the same Strings.resx revealed nothing.
Searching the whole repo for <help also didn't work.
Any suggestions?
There was a problem hiding this comment.
We noticed that HelpLink is available, but there isn't a helper method for it yet. I suggested I'd make a follow-up PR to fix that and clean up all the other instances using it.
There was a problem hiding this comment.
#5493 (comment) <-- we don't actually have an easy way to do this right now. So we can wait.
Fear not! I've been working on that all afternoon 🫡 |
| @@ -1161,6 +1161,11 @@ | |||
| <value>MSB3824: In order to build with .NET Core, resource inputs must be in .txt or .resx format.</value> | |||
| <comment>{StrBegin="MSB3824: "}</comment> | |||
| </data> | |||
| <data name="GenerateResource.BinaryFormatterUse"> | |||
| <value>MSB3825: Resource "{0}" of type "{2}" is deserialized via BinaryFormatter at runtime. BinaryFormatter is deprecated due to possible security risks and will be removed with .NET 9. If you wish to continue using it, set property "GenerateResourceWarnOnBinaryFormatterUse" to false. | |||
| More information: https://learn.microsoft.com/dotnet/standard/serialization/binaryformatter-security-guide</value> | |||
There was a problem hiding this comment.
#5493 (comment) <-- we don't actually have an easy way to do this right now. So we can wait.
Forgind
added
merge-when-branch-open
It will be removed in .NET 9; doing so should be discouraged.
Note that this does nothing by default, but we can change that in the SDK.
Fixes #8453
Context
BinaryFormatter is deprecated and will be removed in .NET 9. In addition to the possibility of using a modern MSBuild with an older framework, there are apparently ways you can exempt your project, so we are not currently removing it entirely, and this warning (which is off by default) can be disabled even if it is enabled in the SDK.
Changes Made
I deleted
using System.Runtime.Serialization.Formatters.Binary;in GenerateResource, then put a warning before the one usage of BinaryFormatter. That isn't necessarily the best way to figure out where it's used, as it would be helpful to know early, so feel free to comment to that effect.Then I disabled it via a property and will make a separate PR to enable it in the 8.0 SDK.
Testing
Notes