Remove CAS era security checks around resource loads (#21825)

5 files changed, 43 insertions, 179 deletions

diff --git a/src/System.Private.CoreLib/src/System/Reflection/RuntimeAssembly.cs b/src/System.Private.CoreLib/src/System/Reflection/RuntimeAssembly.cs
index 8d1b8c9254..45d73a65bf 100644
--- a/src/System.Private.CoreLib/src/System/Reflection/RuntimeAssembly.cs
+++ b/src/System.Private.CoreLib/src/System/Reflection/RuntimeAssembly.cs
@@ -194,19 +194,36 @@ namespace System.Reflection
 
         public override bool IsCollectible => GetIsCollectible(GetNativeHandle());
 
+        // GetResource will return a pointer to the resources in memory.
+        [DllImport(JitHelpers.QCall, CharSet = CharSet.Unicode)]
+        private static extern unsafe byte* GetResource(RuntimeAssembly assembly,
+                                                       string resourceName,
+                                                       out uint length);
+
         // Load a resource based on the NameSpace of the type.
-        [System.Security.DynamicSecurityMethod] // Methods containing StackCrawlMark local var has to be marked DynamicSecurityMethod
         public override Stream GetManifestResourceStream(Type type, string name)
         {
-            StackCrawlMark stackMark = StackCrawlMark.LookForMyCaller;
-            return GetManifestResourceStream(type, name, false, ref stackMark);
+            if (type == null && name == null)
+                throw new ArgumentNullException(nameof(type));
+
+            string nameSpace = type?.Namespace;
+            string delimiter = (nameSpace != null && name != null) ? Type.Delimiter.ToString() : null;
+            string resourceName = string.Concat(nameSpace, delimiter, name);
+
+            return GetManifestResourceStream(resourceName);
         }
 
-        [System.Security.DynamicSecurityMethod] // Methods containing StackCrawlMark local var has to be marked DynamicSecurityMethod
-        public override Stream GetManifestResourceStream(string name)
+        public unsafe override Stream GetManifestResourceStream(string name)
         {
-            StackCrawlMark stackMark = StackCrawlMark.LookForMyCaller;
-            return GetManifestResourceStream(name, ref stackMark, false);
+            uint length = 0;
+            byte* pbInMemoryResource = GetResource(GetNativeHandle(), name, out length);
+
+            if (pbInMemoryResource != null)
+            {
+                return new UnmanagedMemoryStream(pbInMemoryResource, length, length, FileAccess.Read);
+            }
+
+            return null;
         }
 
         // ISerializable implementation
@@ -433,19 +450,15 @@ namespace System.Reflection
         private static extern int GetManifestResourceInfo(RuntimeAssembly assembly,
                                                           string resourceName,
                                                           ObjectHandleOnStack assemblyRef,
-                                                          StringHandleOnStack retFileName,
-                                                          StackCrawlMarkHandle stackMark);
+                                                          StringHandleOnStack retFileName);
 
-        [System.Security.DynamicSecurityMethod] // Methods containing StackCrawlMark local var has to be marked DynamicSecurityMethod
         public override ManifestResourceInfo GetManifestResourceInfo(string resourceName)
         {
             RuntimeAssembly retAssembly = null;
             string fileName = null;
-            StackCrawlMark stackMark = StackCrawlMark.LookForMyCaller;
             int location = GetManifestResourceInfo(GetNativeHandle(), resourceName,
                                                    JitHelpers.GetObjectHandleOnStack(ref retAssembly),
-                                                   JitHelpers.GetStringHandleOnStack(ref fileName),
-                                                   JitHelpers.GetStackCrawlMarkHandle(ref stackMark));
+                                                   JitHelpers.GetStringHandleOnStack(ref fileName));
 
             if (location == -1)
                 return null;
@@ -526,61 +539,6 @@ namespace System.Reflection
 #endif // PLATFORM_WINDOWS
         }
 
-        internal Stream GetManifestResourceStream(
-            Type type,
-            string name,
-            bool skipSecurityCheck,
-            ref StackCrawlMark stackMark)
-        {
-            StringBuilder sb = new StringBuilder();
-            if (type == null)
-            {
-                if (name == null)
-                    throw new ArgumentNullException(nameof(type));
-            }
-            else
-            {
-                string nameSpace = type.Namespace;
-                if (nameSpace != null)
-                {
-                    sb.Append(nameSpace);
-                    if (name != null)
-                        sb.Append(Type.Delimiter);
-                }
-            }
-
-            if (name != null)
-                sb.Append(name);
-
-            return GetManifestResourceStream(sb.ToString(), ref stackMark, skipSecurityCheck);
-        }
-
-        // GetResource will return a pointer to the resources in memory.
-        [DllImport(JitHelpers.QCall, CharSet = CharSet.Unicode)]
-        private static extern unsafe byte* GetResource(RuntimeAssembly assembly,
-                                                       string resourceName,
-                                                       out ulong length,
-                                                       StackCrawlMarkHandle stackMark,
-                                                       bool skipSecurityCheck);
-
-        internal unsafe Stream GetManifestResourceStream(string name, ref StackCrawlMark stackMark, bool skipSecurityCheck)
-        {
-            ulong length = 0;
-            byte* pbInMemoryResource = GetResource(GetNativeHandle(), name, out length, JitHelpers.GetStackCrawlMarkHandle(ref stackMark), skipSecurityCheck);
-
-            if (pbInMemoryResource != null)
-            {
-                //Console.WriteLine("Creating an unmanaged memory stream of length "+length);
-                if (length > long.MaxValue)
-                    throw new NotImplementedException(SR.NotImplemented_ResourcesLongerThanInt64Max);
-
-                return new UnmanagedMemoryStream(pbInMemoryResource, (long)length, (long)length, FileAccess.Read);
-            }
-
-            //Console.WriteLine("GetManifestResourceStream: Blob "+name+" not found...");
-            return null;
-        }
-
         [DllImport(JitHelpers.QCall, CharSet = CharSet.Unicode)]
         private static extern void GetVersion(RuntimeAssembly assembly,
                                               out int majVer,
diff --git a/src/System.Private.CoreLib/src/System/Resources/FileBasedResourceGroveler.cs b/src/System.Private.CoreLib/src/System/Resources/FileBasedResourceGroveler.cs
index eafffd34cf..216ebb1e4a 100644
--- a/src/System.Private.CoreLib/src/System/Resources/FileBasedResourceGroveler.cs
+++ b/src/System.Private.CoreLib/src/System/Resources/FileBasedResourceGroveler.cs
@@ -34,7 +34,7 @@ namespace System.Resources
         // Consider modifying IResourceGroveler interface (hence this method signature) when we figure out 
         // serialization compat story for moving ResourceManager members to either file-based or 
         // manifest-based classes. Want to continue tightening the design to get rid of unused params.
-        public ResourceSet GrovelForResourceSet(CultureInfo culture, Dictionary<string, ResourceSet> localResourceSets, bool tryParents, bool createIfNotExists, ref StackCrawlMark stackMark)
+        public ResourceSet GrovelForResourceSet(CultureInfo culture, Dictionary<string, ResourceSet> localResourceSets, bool tryParents, bool createIfNotExists)
         {
             Debug.Assert(culture != null, "culture shouldn't be null; check caller");
 
diff --git a/src/System.Private.CoreLib/src/System/Resources/IResourceGroveler.cs b/src/System.Private.CoreLib/src/System/Resources/IResourceGroveler.cs
index 8151a7a675..b954db969b 100644
--- a/src/System.Private.CoreLib/src/System/Resources/IResourceGroveler.cs
+++ b/src/System.Private.CoreLib/src/System/Resources/IResourceGroveler.cs
@@ -24,6 +24,6 @@ namespace System.Resources
     internal interface IResourceGroveler
     {
         ResourceSet GrovelForResourceSet(CultureInfo culture, Dictionary<string, ResourceSet> localResourceSets, bool tryParents,
-            bool createIfNotExists, ref StackCrawlMark stackMark);
+            bool createIfNotExists);
     }
 }
diff --git a/src/System.Private.CoreLib/src/System/Resources/ManifestBasedResourceGroveler.cs b/src/System.Private.CoreLib/src/System/Resources/ManifestBasedResourceGroveler.cs
index ea754c1489..fe95bf1c8f 100644
--- a/src/System.Private.CoreLib/src/System/Resources/ManifestBasedResourceGroveler.cs
+++ b/src/System.Private.CoreLib/src/System/Resources/ManifestBasedResourceGroveler.cs
@@ -51,7 +51,7 @@ namespace System.Resources
             _mediator = mediator;
         }
 
-        public ResourceSet GrovelForResourceSet(CultureInfo culture, Dictionary<string, ResourceSet> localResourceSets, bool tryParents, bool createIfNotExists, ref StackCrawlMark stackMark)
+        public ResourceSet GrovelForResourceSet(CultureInfo culture, Dictionary<string, ResourceSet> localResourceSets, bool tryParents, bool createIfNotExists)
         {
             Debug.Assert(culture != null, "culture shouldn't be null; check caller");
             Debug.Assert(localResourceSets != null, "localResourceSets shouldn't be null; check caller");
@@ -100,7 +100,7 @@ namespace System.Resources
                     localResourceSets.TryGetValue(culture.Name, out rs);
                 }
 
-                stream = GetManifestResourceStream(satellite, fileName, ref stackMark);
+                stream = GetManifestResourceStream(satellite, fileName);
             }
 
             // 4a. Found a stream; create a ResourceSet if possible
@@ -306,17 +306,12 @@ namespace System.Resources
             }
         }
 
-        private Stream GetManifestResourceStream(RuntimeAssembly satellite, string fileName, ref StackCrawlMark stackMark)
+        private Stream GetManifestResourceStream(RuntimeAssembly satellite, string fileName)
         {
             Debug.Assert(satellite != null, "satellite shouldn't be null; check caller");
             Debug.Assert(fileName != null, "fileName shouldn't be null; check caller");
 
-            // 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);
-
-            Stream stream = satellite.GetManifestResourceStream(_mediator.LocationInfo, fileName, canSkipSecurityCheck, ref stackMark);
+            Stream stream = satellite.GetManifestResourceStream(_mediator.LocationInfo, fileName);
             if (stream == null)
             {
                 stream = CaseInsensitiveManifestResourceStreamLookup(satellite, fileName);
@@ -329,30 +324,19 @@ namespace System.Resources
         // case-insensitive lookup rules.  Yes, this is slow.  The metadata
         // dev lead refuses to make all assembly manifest resource lookups case-insensitive,
         // even optionally case-insensitive.        
-        [System.Security.DynamicSecurityMethod] // Methods containing StackCrawlMark local var has to be marked DynamicSecurityMethod
         private Stream CaseInsensitiveManifestResourceStreamLookup(RuntimeAssembly satellite, string name)
         {
             Debug.Assert(satellite != null, "satellite shouldn't be null; check caller");
             Debug.Assert(name != null, "name shouldn't be null; check caller");
 
-            StringBuilder sb = new StringBuilder();
-            if (_mediator.LocationInfo != null)
-            {
-                string nameSpace = _mediator.LocationInfo.Namespace;
-                if (nameSpace != null)
-                {
-                    sb.Append(nameSpace);
-                    if (name != null)
-                        sb.Append(Type.Delimiter);
-                }
-            }
-            sb.Append(name);
+            string nameSpace = _mediator.LocationInfo?.Namespace;
+            string delimiter = (nameSpace != null && name != null) ? Type.Delimiter.ToString() : null;
+            string resourceName = string.Concat(nameSpace, delimiter, name);
 
-            string givenName = sb.ToString();
             string canonicalName = null;
             foreach (string existingName in satellite.GetManifestResourceNames())
             {
-                if (string.Equals(existingName, givenName, StringComparison.InvariantCultureIgnoreCase))
+                if (string.Equals(existingName, resourceName, StringComparison.InvariantCultureIgnoreCase))
                 {
                     if (canonicalName == null)
                     {
@@ -360,7 +344,7 @@ namespace System.Resources
                     }
                     else
                     {
-                        throw new MissingManifestResourceException(SR.Format(SR.MissingManifestResource_MultipleBlobs, givenName, satellite.ToString()));
+                        throw new MissingManifestResourceException(SR.Format(SR.MissingManifestResource_MultipleBlobs, resourceName, satellite.ToString()));
                     }
                 }
             }
@@ -370,12 +354,7 @@ namespace System.Resources
                 return null;
             }
 
-            // 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;
-            StackCrawlMark stackMark = StackCrawlMark.LookForMyCaller;
-            return satellite.GetManifestResourceStream(canonicalName, ref stackMark, canSkipSecurityCheck);
+            return satellite.GetManifestResourceStream(canonicalName);
         }
 
         private RuntimeAssembly GetSatelliteAssembly(CultureInfo lookForCulture)
diff --git a/src/System.Private.CoreLib/src/System/Resources/ResourceManager.cs b/src/System.Private.CoreLib/src/System/Resources/ResourceManager.cs
index d216db4bc7..69d8e5fc50 100644
--- a/src/System.Private.CoreLib/src/System/Resources/ResourceManager.cs
+++ b/src/System.Private.CoreLib/src/System/Resources/ResourceManager.cs
@@ -148,8 +148,6 @@ namespace System.Resources
         private Version _satelliteContractVersion;
         private bool _lookedForSatelliteContractVersion;
 
-        private RuntimeAssembly _callingAssembly;  // Assembly who created the ResMgr.
-
         private IResourceGroveler resourceGroveler;
 
         public static readonly int MagicNumber = unchecked((int)0xBEEFCACE);  // If only hex had a K...
@@ -180,16 +178,8 @@ namespace System.Resources
         internal const string ResFileExtension = ".resources";
         internal const int ResFileExtensionLength = 10;
 
-        [System.Security.DynamicSecurityMethod] // Methods containing StackCrawlMark local var has to be marked DynamicSecurityMethod
-        private void Init()
-        {
-            _callingAssembly = (RuntimeAssembly)Assembly.GetCallingAssembly();
-        }
-
         protected ResourceManager()
         {
-            Init();
-
             _lastUsedResourceCache = new CultureNameResourceSetPair();
             ResourceManagerMediator mediator = new ResourceManagerMediator(this);
             resourceGroveler = new ManifestBasedResourceGroveler(mediator);
@@ -227,7 +217,6 @@ namespace System.Resources
             resourceGroveler = new FileBasedResourceGroveler(mediator);
         }
 
-        [System.Security.DynamicSecurityMethod] // Methods containing StackCrawlMark local var has to be marked DynamicSecurityMethod
         public ResourceManager(string baseName, Assembly assembly)
         {
             if (null == baseName)
@@ -245,19 +234,8 @@ namespace System.Resources
             SetAppXConfiguration();
 
             CommonAssemblyInit();
-
-            _callingAssembly = (RuntimeAssembly)Assembly.GetCallingAssembly();
-            // Special case for mscorlib - protect mscorlib's private resources.
-            // This isn't for security reasons, but to ensure we can make
-            // breaking changes to mscorlib's internal resources without 
-            // assuming users may have taken a dependency on them.
-            if (assembly == typeof(object).Assembly && _callingAssembly != assembly)
-            {
-                _callingAssembly = null;
-            }
         }
 
-        [System.Security.DynamicSecurityMethod] // Methods containing StackCrawlMark local var has to be marked DynamicSecurityMethod
         public ResourceManager(string baseName, Assembly assembly, Type usingResourceSet)
         {
             if (null == baseName)
@@ -276,16 +254,8 @@ namespace System.Resources
             _userResourceSet = usingResourceSet;
 
             CommonAssemblyInit();
-            _callingAssembly = (RuntimeAssembly)Assembly.GetCallingAssembly();
-            // Special case for mscorlib - protect mscorlib's private resources.
-            // This isn't for security reasons, but to ensure we can make
-            // breaking changes to mscorlib's internal resources without 
-            // assuming users may have taken a dependency on them.
-            if (assembly == typeof(object).Assembly && _callingAssembly != assembly)
-                _callingAssembly = null;
         }
 
-        [System.Security.DynamicSecurityMethod] // Methods containing StackCrawlMark local var has to be marked DynamicSecurityMethod
         public ResourceManager(Type resourceSource)
         {
             if (null == resourceSource)
@@ -301,13 +271,6 @@ namespace System.Resources
             SetAppXConfiguration();
 
             CommonAssemblyInit();
-
-            _callingAssembly = (RuntimeAssembly)Assembly.GetCallingAssembly();
-            // Special case for mscorlib - protect mscorlib's private resources.
-            if (MainAssembly == typeof(object).Assembly && _callingAssembly != MainAssembly)
-            {
-                _callingAssembly = null;
-            }
         }
 
         // Trying to unify code as much as possible, even though having to do a
@@ -466,7 +429,6 @@ namespace System.Resources
         // if it hasn't yet been loaded and if parent CultureInfos should be 
         // loaded as well for resource inheritance.
         //         
-        [System.Security.DynamicSecurityMethod] // Methods containing StackCrawlMark local var has to be marked DynamicSecurityMethod
         public virtual ResourceSet GetResourceSet(CultureInfo culture, bool createIfNotExists, bool tryParents)
         {
             if (null == culture)
@@ -483,13 +445,10 @@ namespace System.Resources
                 }
             }
 
-            StackCrawlMark stackMark = StackCrawlMark.LookForMyCaller;
-
             if (UseManifest && culture.HasInvariantCultureName)
             {
                 string fileName = GetResourceFileName(culture);
-                RuntimeAssembly mainAssembly = (RuntimeAssembly)MainAssembly;
-                Stream stream = mainAssembly.GetManifestResourceStream(_locationInfo, fileName, _callingAssembly == MainAssembly, ref stackMark);
+                Stream stream = MainAssembly.GetManifestResourceStream(_locationInfo, fileName);
                 if (createIfNotExists && stream != null)
                 {
                     rs = ((ManifestBasedResourceGroveler)resourceGroveler).CreateResourceSet(stream, MainAssembly);
@@ -498,14 +457,6 @@ namespace System.Resources
                 }
             }
 
-            // 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);
         }
 
@@ -513,33 +464,22 @@ namespace System.Resources
         // for getting a resource set lives.  Access to it is controlled by
         // threadsafe methods such as GetResourceSet, GetString, & GetObject.  
         // This will take a minimal number of locks.
-        [System.Security.DynamicSecurityMethod] // Methods containing StackCrawlMark local var has to be marked DynamicSecurityMethod
         protected virtual ResourceSet InternalGetResourceSet(CultureInfo culture, bool createIfNotExists, bool tryParents)
         {
             Debug.Assert(culture != null, "culture != null");
 
-            StackCrawlMark stackMark = StackCrawlMark.LookForMyCaller;
-            return InternalGetResourceSet(culture, createIfNotExists, tryParents, ref stackMark);
-        }
-
-        // InternalGetResourceSet is a non-threadsafe method where all the logic
-        // for getting a resource set lives.  Access to it is controlled by
-        // threadsafe methods such as GetResourceSet, GetString, & GetObject.  
-        // This will take a minimal number of locks.
-        private ResourceSet InternalGetResourceSet(CultureInfo requestedCulture, bool createIfNotExists, bool tryParents, ref StackCrawlMark stackMark)
-        {
             Dictionary<string, ResourceSet> localResourceSets = _resourceSets;
             ResourceSet rs = null;
             CultureInfo foundCulture = null;
             lock (localResourceSets)
             {
-                if (localResourceSets.TryGetValue(requestedCulture.Name, out rs))
+                if (localResourceSets.TryGetValue(culture.Name, out rs))
                 {
                     return rs;
                 }
             }
 
-            ResourceFallbackManager mgr = new ResourceFallbackManager(requestedCulture, _neutralResourcesCulture, tryParents);
+            ResourceFallbackManager mgr = new ResourceFallbackManager(culture, _neutralResourcesCulture, tryParents);
 
             foreach (CultureInfo currentCultureInfo in mgr)
             {
@@ -548,7 +488,7 @@ namespace System.Resources
                     if (localResourceSets.TryGetValue(currentCultureInfo.Name, out rs))
                     {
                         // we need to update the cache if we fellback
-                        if (requestedCulture != currentCultureInfo) foundCulture = currentCultureInfo;
+                        if (culture != currentCultureInfo) foundCulture = currentCultureInfo;
                         break;
                     }
                 }
@@ -560,7 +500,7 @@ namespace System.Resources
                 // ResourceManager).  It's happened.
 
                 rs = resourceGroveler.GrovelForResourceSet(currentCultureInfo, localResourceSets,
-                                                           tryParents, createIfNotExists, ref stackMark);
+                                                           tryParents, createIfNotExists);
 
                 // found a ResourceSet; we're done
                 if (rs != null)
@@ -768,9 +708,6 @@ namespace System.Resources
 
             RuntimeAssembly resourcesAssembly = (RuntimeAssembly)MainAssembly;
 
-            if (resourcesAssembly == null)
-                resourcesAssembly = _callingAssembly;
-
             if (resourcesAssembly != null)
             {
                 if (resourcesAssembly != typeof(object).Assembly) // We are not loading resources for mscorlib
@@ -1016,11 +953,6 @@ namespace System.Resources
 
             foreach (CultureInfo currentCultureInfo in mgr)
             {
-                // 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.
                 ResourceSet rs = InternalGetResourceSet(currentCultureInfo, true, true);
                 if (rs == null)
                     break;
@@ -1137,11 +1069,6 @@ namespace System.Resources
                 set { _rm._fallbackLoc = value; }
             }
 
-            internal RuntimeAssembly CallingAssembly
-            {
-                get { return _rm._callingAssembly; }
-            }
-
             internal RuntimeAssembly MainAssembly
             {
                 get { return (RuntimeAssembly)_rm.MainAssembly; }