Remove CAS era security checks around resource loads and stack crawls

[danmoseley]

I assume everything related to skipSecurityCheck and stackMark in the resource loading in corelib and VM is related to CAS and can be removed. But what about StackCrawlMark in general. It is in 70 places in the VM. Is it all simply for CAS link demand checks, and we can remove it everywhere?

@jkotas

[jkotas]

StackCrawlMark in general. It is in 70 places.

Unfortunately, there is a ton of legacy APIs that were added during netstandard2.0 push whose behavior depend on the caller. The caller is basically passed in as an implicit argument to the API. Most of these StackCrawlMarks are there to support these APIs. There may be a few that are doing this by accident, but I do not think it is possible to cleanup most of this without breaking stuff.

skipSecurityCheck in the resource loading

Yes, this one can be cleaned up probably. The flag is passed in through many layers. It should be cleaned up from all layers - it will allow us to double check that it is not used by anything important.

[danmoseley]

skipSecurityCheck all seems to end up here, https://github.com/dotnet/coreclr/blob/75c8c54950c78f1192e481a5bbd6eeff1b3bd1cd/src/vm/pefile.cpp#L1787-L1807 and from looking at AccessCheckType it mixes visibility checks (which we want to keep) and transparency related checks. If that enum can be simplified, then code can be simplified further up.

[danmoseley]

I think this cleanup would be valuable, but I do not plan to do this analysis.

[filipnavara]

I did some further analysis and found that not only the checks are quite useless security-wise, but some of them have been broken for quite a while.

Effectively the check listed in the previous comment says that private embedded manifest resources are allowed to be accessed when the caller is the within the same assembly as the resources (or a friend assembly).

The documentation says that it should be allowed to access those private resources when ReflectionPermission is given, which I believe is something implicit in full-trust scenario like CoreCLR. Correct?

So the only thing that should actually be prevented is the visibility check with DisablePrivateReflection specified in the target assembly.

Another problem is that the stack crawl mark is not always passed through correctly. Look at the following code:

https://github.com/dotnet/coreclr/blob/6e986f5ed76701109f83d50df1da53ddb23624ab/src/System.Private.CoreLib/src/System/Resources/ManifestBasedResourceGroveler.cs#L319-L323

If GetManifestResourceStream fails due to security check and returns null then it proceeds to CaseInsensitiveManifestResourceStreamLookup, which will try to match the same resources, but this time the stack mark is incorrectly pointing into CoreLib. So it will allow loading private resources from CoreLib and it will fail for private resources that should be otherwise accessible (and would be returned if the name was matched case-sensitively).

Numerous other comments suggest that the checks are not very useful (and sometimes propagate stack mark pointing into CoreLib too):

https://github.com/dotnet/coreclr/blob/6e986f5ed76701109f83d50df1da53ddb23624ab/src/System.Private.CoreLib/src/System/Resources/ResourceManager.cs#L501-L509

https://github.com/dotnet/coreclr/blob/6e986f5ed76701109f83d50df1da53ddb23624ab/src/System.Private.CoreLib/src/System/Resources/ResourceManager.cs#L1020-L1024

The above code in GetObject will actually always pass in CoreLib's stack mark every time and thus marks the whole stack mark check useless. You can still access your own private resources thanks to canSkipSecurityCheck workaround here:

https://github.com/dotnet/coreclr/blob/6e986f5ed76701109f83d50df1da53ddb23624ab/src/System.Private.CoreLib/src/System/Resources/ManifestBasedResourceGroveler.cs#L314-L317

This doesn't consider friend assemblies though, which should be allowed to access private resources.

Most of the problems went unnoticed because there are no tests for it. The private embedded manifest resources are also extremely rare. CoreRT doesn't have any of the checks.

This issue should be resolved in order to allow moving rest of System.Resources classes to shared CoreLib.

What is the preferred solution?

1. Repair the security checks to actually do what they were supposed to do? This will be difficult, require additional tests and will make code sharing with CoreRT a bit more difficult.
2. Remove the checks altogether? There's no security impact since one can always access the same resources manually. It may have some impact on correctness for a very rare case (private manifest resources in assembly with DisablePrivateReflection attribute would be accessible from different calling assembly that is not a friend assembly).
3. Simplify the security checks by passing in _callingAssembly from ResourceManager down to the runtime and use it instead of the stack mark? There won't be any performance regression and it will remove the canSkipSecurityCheck parameter passed to runtime. However it won't be exactly correct for cases where the ResourceManager object is created with one calling assembly and passed to another one which calls GetResourceSet on it. This is already partially broken today though since the security checks are bypassed if the assembly calling the constructor is allowed to access the private resources.
4. Leave it as is?

[jkotas]

ReflectionPermission is given, which I believe is something implicit in full-trust scenario like CoreCLR. Correct?

Correct.

prevented is the visibility check with DisablePrivateReflection

DisablePrivateReflection is about preventing invocation (i.e. running code). It does not prevent inspection (ie. reading stuff - getting MethodInfo for private methods, etc.). Resource reading is inspection.

I think we should remove these checks altogether.

Thanks for a great analysis!

[filipnavara]

Great, thanks for confirming it. I will submit a PR to remove the checks.