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 CAS era security checks around resource loads and stack crawls #21629

Closed
danmosemsft opened this Issue Dec 21, 2018 · 6 comments

Comments

Projects
None yet
3 participants
@danmosemsft
Copy link
Member

danmosemsft commented Dec 21, 2018

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

This comment has been minimized.

Copy link
Member

jkotas commented Dec 21, 2018

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.

@danmosemsft

This comment has been minimized.

Copy link
Member Author

danmosemsft commented Dec 21, 2018

skipSecurityCheck all seems to end up here,

coreclr/src/vm/pefile.cpp

Lines 1787 to 1807 in 75c8c54

if (!IsMrPublic(dwResourceFlags) && pStackMark && !fSkipSecurityCheck)
{
Assembly *pCallersAssembly = SystemDomain::GetCallersAssembly(pStackMark);
if (pCallersAssembly && // full trust for interop
(!pCallersAssembly->GetManifestFile()->Equals(this)))
{
RefSecContext sCtx(AccessCheckOptions::kMemberAccess);
AccessCheckOptions accessCheckOptions(
AccessCheckOptions::kMemberAccess, /*accessCheckType*/
NULL, /*pAccessContext*/
FALSE, /*throwIfTargetIsInaccessible*/
(MethodTable *) NULL /*pTargetMT*/
);
// SL: return TRUE only if the caller is critical
// Desktop: return TRUE only if demanding MemberAccess succeeds
if (!accessCheckOptions.DemandMemberAccessOrFail(&sCtx, NULL, TRUE /*visibilityCheck*/))
return FALSE;
}
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.

@danmosemsft

This comment has been minimized.

Copy link
Member Author

danmosemsft commented Dec 28, 2018

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

@filipnavara

This comment has been minimized.

Copy link
Collaborator

filipnavara commented Jan 5, 2019

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:

Stream stream = satellite.GetManifestResourceStream(_mediator.LocationInfo, fileName, canSkipSecurityCheck, ref stackMark);
if (stream == null)
{
stream = CaseInsensitiveManifestResourceStreamLookup(satellite, fileName);
}

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):

// Note: ideally we could plumb through the stack crawl mark here, but we must
// call the virtual 3-argument InternalGetResourceSet method for compatibility.
// Security-wise, we're not overly interested in protecting access to resources,
// since full-trust callers can get them already and most resources are public.
// Also, the JIT inliner could always inline a caller into another assembly's
// method.
// So if we happen to return some resources in cases where we should really be
// doing a demand for member access permissions, we're not overly concerned.
return InternalGetResourceSet(culture, createIfNotExists, tryParents);

// Note: Technically this method should be passed in a stack crawl mark that we then pass
// to InternalGetResourceSet for ensuring we demand permissions to read your private resources
// if you're reading resources from an assembly other than yourself. But, we must call our
// three argument overload (without the stack crawl mark) for compatibility. After
// consideration, we aren't worried about the security impact.

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:

// If we're looking in the main assembly AND if the main assembly was the person who
// created the ResourceManager, skip a security check for private manifest resources.
bool canSkipSecurityCheck = (_mediator.MainAssembly == satellite)
&& (_mediator.CallingAssembly == _mediator.MainAssembly);

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

This comment has been minimized.

Copy link
Member

jkotas commented Jan 5, 2019

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

This comment has been minimized.

Copy link
Collaborator

filipnavara commented Jan 5, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment