Make ResourceManager use Assembly instead of RuntimeAssembly internally.

[filipnavara]

This is clean-up to aid moving System.Resources classes into shared CoreLib partition.

[filipnavara]

This is not directly related to the PR, but it should be fixed nevertheless. The method takes the throwOnFileNotFound parameter, but it didn't actually use it.

[filipnavara]

I moved the name generation to InternalGetSatelliteAssembly to avoid unnecessarily duplicating it in ManifestBasedResourceGroveler.

[jkotas]

cc @sdmaclea This refactoring is to support sharing of the ResourceManager and friends with CoreRT.

[filipnavara]

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test

[filipnavara]

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test

[filipnavara]

Any updates on this? Anything I could do to speed things up? I have some dependent changes in my queue.

[jkotas]

Resources use binary serialization underneath. This change is allowing ResourceManager and friends to be initialized from arbitrary assembly (e.g. from System.Reflection.MetadataLoadContext) and thus creates a new indirect path to run arbitrary code via binary serialization.

@bartonjs @GrabYourPitchforks Is this a valid security concern? Should we keep limiting ResourceManager to runtime created assemblies only?

[danmoseley]

@morganbr

[filipnavara]

@morganbr Is the new Serialization Guard going to affect this somehow?

Resources use binary serialization underneath. This change is allowing ResourceManager and friends to be initialized from arbitrary assembly (e.g. from System.Reflection.MetadataLoadContext) and thus creates a new indirect path to run arbitrary code via binary serialization.

Fair enough. I was about to suggest that I can add back the RuntimeAssembly guard as #if !CORERT, but you made me realise that's actually a bad idea since it unnecessary limits the usefulness of the MetadataLoadContext. If the deserialization is deemed harmful a middle ground is to pass enough information to ManifestBasedResourceGroveler to create the ResourceSet objects with permitDeserialization: false.

[jkotas]

@filipnavara The serialization guard is a secondary "defense in depth" when somebody uses binary deserialization on untrusted data. It is a speed bump, no something to depend on.

[jkotas]

a bad idea since it unnecessary limits the usefulness of MetadataLoadContext

MetadataLoadContext is not the first implementation of reflection object model. The custom reflection object models have been around for a very long time. I have not ever heard somebody to try to use custom reflection object model with ResourceManager and running into this block.

[filipnavara]

@jkotas Understood.

I am trying to figure out whether there is some scenario where the deserialization would be useful. It was added back mostly because of WinForms resources. I can imagine that tools like ILSpy (or some other *Spy tool) may want to load such resources from MetadataLoadContext.

[filipnavara]

I have added the restriction to enable deserialization only for RuntimeAssembly. This will still allow to use ResourceManager on assemblies loaded through MetadataLoadContext, but for all practical purposes it will be restricted to string resources or raw resource data (looks like it is not exposed through ResourceSet).

[jkotas]

If ILSpy or its variant comes with a request to make this possible, we can look into how to make it work. I do think these tools would actually ever want to use ResourceManager to dump managed resources.

[filipnavara]

If ILSpy or its variant comes with a request to make this possible, we can look into how to make it work.

I assume you won't get the request because they have already copied the code from .NET Core and Mono (https://github.com/icsharpcode/ILSpy/blob/ec964bf4ba00d3a8b3a983e52e8a6225720fcfbd/ICSharpCode.Decompiler/Util/ResourcesFile.cs & history).

We do something like this in our private localization tool, but we didn't port it to .NET Core yet and probably won't do that for a foreseeable future. In that tool we only use it to read strings anyway.

Update: I checked our localization tool and it roughly does the equivalent of

Assembly a = Assembly.ReflectionOnlyLoadFrom(filename);
foreach (string name in a.GetManifestResourceNames())
{
     ...
     ResourceSet resources = new ResourceSet(a.GetManifestResourceStream(name));

It doesn't use ResourceManager directly and instead uses the lower level types. It would eventually break because ResourceReader and ResourceSet don't have public constructors that permit the deserialization, but that's getting a bit off topic...

[jkotas]

It would eventually break because ResourceReader and ResourceSet don't have public constructors that permit the deserialization, but that's getting a bit off topic...

I do not think it is off topic. It demonstrates that relaxing the checks in ResourceManager does not actually help the tools like the ones you have mentioned.

[jkotas]

I think we should keep the upfront checks that restrict the ResourceManager to runtime implemented assemblies only. We do have IsRuntimeImplemented() method to Type. It should be fine to add same method for assemblies too.

[filipnavara]

We do have IsRuntimeImplemented() method to Type. It should be fine to add same method for assemblies too.

Alright, will do that.

[filipnavara]

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test

[filipnavara]

/AzurePipelines run coreclr-ci

[azure-pipelines]

Success! 1 created and queued.