Runtime: ResourceReader and ResourceWriter do not support binary formatted blobs in resources

[ViktorHofer]

from @danmosemsft

I think the work here is basically to bring back BinaryFormatter in ResourceReader. Since since this bug was opened, we ported it to .NET Core.
https://referencesource.microsoft.com/#mscorlib/system/resources/resourcereader.cs,107

[tarekgh]

I moved it to the future. I don't think this is needed for 2.1.

[tarekgh]

CC @krwq

[ericstj]

We need to consider this for vNext. I prototyped here: https://devdiv.visualstudio.com/DevDiv/Default/_git/DotNet-CoreCLR-Trusted/commit/c4439bc1f9dd4dbd827de5132334fef97ea958e2?refName=refs%2Fheads%2Fericstj%2Fadd.back.exeperiment

[jkotas]

@morganbr What do you think we need to do about the security aspect of this?

[ericstj]

We need to consider this for WPF/Winforms (or come up with an alternative).

[danmosemsft]

@morganbr What do you think we need to do about the security aspect of this?

I believe we have always said that build is not a security boundary, ie., you must trust the code you build, and it must trust the types in the resx. Is there another angle?

[danmosemsft]

Holding on this one for now.

[danmosemsft]

Next step here I believe was to make a call on whether to port as is or port with changes. @AlexGhiondea @ericstj I believe it was one of you that was engaged in that process.

[tarekgh]

CC @Tanya-Solyanik

[danmosemsft]

per @merriemcgaw they are evaluating whether they need this. On hold.

[filipnavara]

What's the current status?

There were three issues with using Bitmap/Icon/etc. resources in WinForms:

• Making the types serializable was implemented as PR dotnet/corefx#30817
• msbuild support in GenerateResource is tracked in https://github.com/Microsoft/msbuild/issues/2221; arguably it's not that important since Visual Studio internally triggers the NetFX msbuild even for CoreFX projects, so there's a workaround
• ResourceReader support tracked in this issue

Right now loading an Icon resource fails with the following error:

Unhandled Exception: System.NotSupportedException: Cannot read resources that depend on serialization.
   at System.Resources.ResourceReader._LoadObjectV2(Int32 pos, ResourceTypeCode& typeCode)
   at System.Resources.ResourceReader.LoadObjectV2(Int32 pos, ResourceTypeCode& typeCode)
   at System.Resources.ResourceReader.LoadObject(Int32 pos, ResourceTypeCode& typeCode)
   at System.Resources.RuntimeResourceSet.GetObject(String key, Boolean ignoreCase, Boolean isString)
   at System.Resources.RuntimeResourceSet.GetObject(String key, Boolean ignoreCase)
   at System.Resources.ResourceManager.GetObject(String name, CultureInfo culture, Boolean wrapUnmanagedMemStream)
   at System.Resources.ResourceManager.GetObject(String name, CultureInfo culture)
   at MailClient.Branding.eM_Client.Branding.get_application() in C:\Projects\svn\emclient\MailClient\Branding\eM Client\Branding.Designer.cs:line 78
   at MailClient.BrandingUtils.get_ApplicationIcon() in C:\Projects\svn\emclient\MailClient\BrandingUtils.cs:line 40

[tarekgh]

@Tanya-Solyanik any update from your side?

[danmosemsft]

This is work in ResourceReader, which I believe we (BCL) own, to bring back DeserializeObject etc.

[danmosemsft]

cc @adreddy507

[danmosemsft]

cc @dreddy-work rather

[filipnavara]

Btw, the implementation of ResourceReader in CoreClr and CoreRT are almost identical. The places where they actually diverged seem insignificant. I guess it should be moved to the shared code.

[filipnavara]

The ResourceReader is now moved to shared code that is consumed by both CoreRT and CoreClr. I made an experimental patch for bringing back the serialization support - https://github.com/dotnet/coreclr/compare/master...filipnavara:res-serialization?expand=1.

I'm not very happy about using reflection to access BinaryFormatter, so it would be nice if someone could propose an alternative solution.

[tarekgh]

@filipnavara I believe we'll need to move BinaryFormatter to the corelib to make this work cleanly. hold on till @dreddy-work get back to us and then we can figure out what work need to be done here. thanks for your initiatives.

[jkotas]

I believe we'll need to move BinaryFormatter to the corelib to make this work cleanl

The reflection use for this is fine. I do not think we need to move BinaryFormatter to CoreLib for this. We use reflection in other similar places that support legacy code paths.

[jkotas]

https://github.com/dotnet/coreclr/compare/master...filipnavara:res-serialization?expand=1

The call should be a direct call, not a DynamicInvoke. Like in https://github.com/dotnet/corefx/issues/26745#issuecomment-389215485

[morganbr]

@danmosemsft, has there been a decision on whether we actually need that functionality in that form?

[filipnavara]

The call should be a direct call, not a DynamicInvoke.

Of course :) It is a leftover from my first broken attempt at it.

Like in dotnet/corefx#26745 (comment)

Not really accessible for non-Microsofties like me ;-)

has there been a decision on whether we actually need that functionality in that form?

Unless you plan to significantly rework how the resx files are produced and consumed I don't see any other way. I have now made custom CoreClr build and incorporated some of the recent fixes for System.Drawing.Common and I was able to get our WinForms application running on CoreFX.

[mjrousos]

Regarding the question of whether we need this, I think that we do. I've just started helping customers move to .NET Core 3.0 with WinForms/WPF apps and hit this almost immediately.

Any customer migrating an app with bitmap or icon resources will run into the exception @filipnavara posted in his comment.

[ericstj]

Not really accessible for non-Microsofties like me ;-)

Here you go: https://github.com/ericstj/coreclr/commit/782f7dda1afd04d5b67b681ecd56aab154d483c0. No guarantees it still compiles 😉

[filipnavara]

@ericstj Thanks. It is essentially the same as the patch I have with small differences in error handling :)

[nguerrera]

cc @dasMulli

[ericstj]

I'll pick this up to put in a fix (possibly temporary) for preview1.

[stephentoub]

I've just started helping customers move to .NET Core 3.0 with WinForms/WPF apps and hit this almost immediately.

Just to double-down on this, it's super painful. Just yesterday I tried to port a old Windows Forms app I had, and immediately hit this, without a good workaround (where "good" is defined as not taking me hours to try to recreate everything in the .resx I'd previously created in the tooling).

[dasMulli]

Fyi this is stopping KeePass from migrating, which would be a perfect demo for porting to .NET Core 3.0.

[danmosemsft]

@ericstj do you believe we still need this after your change? Seems you may have already committed a reasonable long term solution. Maybe we can close this, or narrow the scope of it.

[ericstj]

I've only fixed the reading side. The writing side still doesn't emit serialized blobs:
https://github.com/dotnet/corefx/blob/6e55e0fa6711a15e1e02a6948532979a142c993b/src/System.Resources.Writer/src/System/Resources/ResourceWriter.cs#L646

I believe we need WinForms / MSBuild to close on the plans for writing, including https://github.com/Microsoft/msbuild/issues/2221 before considering this done.

[danmosemsft]

I think the status of this is - @rainersigwald is doing experiments to see whether MSBuild can avoid the serialization/deserialization during .resx building. If so - we would not be motivated to do this because of the new security sensitive surface. If not - we have to rediscuss.

[danmosemsft]

Proposal is currently to leave this as is.

[filipnavara]

@danmosemsft I have one case tracked in issue dotnet/corefx#35114 that breaks our application for a code path where serialized resources are not allowed yet. I'd be willing to prototype a fix and submit a PR. The security impact would be quite minimal since it would affect only manifest resources for which there is already an exception to allow deserialization.

[danmosemsft]

@ericstj what are your thoughts on this above?

[ericstj]

I took a look at the linked issue and it seems like a reasonable tweak to my earlier change. I'm ok with that.

[mjrousos]

@danmosemsft, double-checking that I understand you correctly - the proposal is that @ericstj's fix here will be left as is (modified, perhaps, by the suggestion in dotnet/corefx#35114)?

Do we have a proposal for the writing side?

[ericstj]

https://github.com/Microsoft/msbuild/issues/2221 continues to track the writing side of this and I believe @rainersigwald has a proposal that involves making those changes on the MSBuild side. Let's close this issue to avoid further confusion. If folks have proposals to further change the Resources API in corelib let's reopen an issue and clearly call out those changes.