Skip to content

Commit

Permalink
Fix a potential out-of-bound read in RuntimeInvokeHostAssemblyResolver (
Browse files Browse the repository at this point in the history
#104536)

* Add asserts

* Add test

* Change return types of reverse FCalls that return Assembly to RuntimeAssembly to ensure type safety

Fixes #104466

* Fix test

* CR feedback:
- Improve tests
- Improve error messages
  • Loading branch information
jkotas authored Jul 28, 2024
1 parent 11ca923 commit e3d15b8
Show file tree
Hide file tree
Showing 11 changed files with 103 additions and 89 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -593,12 +593,12 @@ private CultureInfo GetLocale()
[LibraryImport(RuntimeHelpers.QCall, EntryPoint = "AssemblyNative_GetSimpleName")]
private static partial void GetSimpleName(QCallAssembly assembly, StringHandleOnStack retSimpleName);

internal string? GetSimpleName()
internal string GetSimpleName()
{
RuntimeAssembly runtimeAssembly = this;
string? name = null;
GetSimpleName(new QCallAssembly(ref runtimeAssembly), new StringHandleOnStack(ref name));
return name;
return name!;
}

[LibraryImport(RuntimeHelpers.QCall, EntryPoint = "AssemblyNative_GetHashAlgorithm")]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ internal Assembly LoadFromInMemoryModule(IntPtr moduleHandle)

// This method is invoked by the VM to resolve a satellite assembly reference
// after trying assembly resolution via Load override without success.
private static Assembly? ResolveSatelliteAssembly(IntPtr gchManagedAssemblyLoadContext, AssemblyName assemblyName)
private static RuntimeAssembly? ResolveSatelliteAssembly(IntPtr gchManagedAssemblyLoadContext, AssemblyName assemblyName)
{
AssemblyLoadContext context = (AssemblyLoadContext)(GCHandle.FromIntPtr(gchManagedAssemblyLoadContext).Target)!;

Expand All @@ -136,7 +136,7 @@ private static IntPtr ResolveUnmanagedDllUsingEvent(string unmanagedDllName, Ass

// This method is invoked by the VM to resolve an assembly reference using the Resolving event
// after trying assembly resolution via Load override and TPA load context without success.
private static Assembly? ResolveUsingResolvingEvent(IntPtr gchManagedAssemblyLoadContext, AssemblyName assemblyName)
private static RuntimeAssembly? ResolveUsingResolvingEvent(IntPtr gchManagedAssemblyLoadContext, AssemblyName assemblyName)
{
AssemblyLoadContext context = (AssemblyLoadContext)(GCHandle.FromIntPtr(gchManagedAssemblyLoadContext).Target)!;
// Invoke the AssemblyResolve event callbacks if wired up
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/dlls/mscorrc/mscorrc.rc
Original file line number Diff line number Diff line change
Expand Up @@ -702,7 +702,7 @@ END
STRINGTABLE DISCARDABLE
BEGIN
IDS_HOST_ASSEMBLY_RESOLVER_ASSEMBLY_ALREADY_LOADED_IN_CONTEXT "Assembly with same name is already loaded"
IDS_HOST_ASSEMBLY_RESOLVER_DYNAMICALLY_EMITTED_ASSEMBLIES_UNSUPPORTED "Dynamically emitted assemblies are unsupported during host-based resolution."
IDS_HOST_ASSEMBLY_RESOLVER_DYNAMICALLY_EMITTED_ASSEMBLIES_UNSUPPORTED "Dynamically emitted assemblies are unsupported for AssemblyLoadContext resolution."
END

STRINGTABLE DISCARDABLE
Expand Down
40 changes: 24 additions & 16 deletions src/coreclr/vm/appdomain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3965,7 +3965,7 @@ Assembly* AppDomain::RaiseTypeResolveEventThrowing(Assembly* pAssembly, LPCSTR s
GCX_COOP();

struct {
OBJECTREF AssemblyRef;
ASSEMBLYREF AssemblyRef;
STRINGREF str;
} gc;
gc.AssemblyRef = NULL;
Expand All @@ -3974,7 +3974,7 @@ Assembly* AppDomain::RaiseTypeResolveEventThrowing(Assembly* pAssembly, LPCSTR s
GCPROTECT_BEGIN(gc);

if (pAssembly != NULL)
gc.AssemblyRef = pAssembly->GetExposedObject();
gc.AssemblyRef = (ASSEMBLYREF)pAssembly->GetExposedObject();

MethodDescCallSite onTypeResolve(METHOD__ASSEMBLYLOADCONTEXT__ON_TYPE_RESOLVE);

Expand All @@ -3984,14 +3984,16 @@ Assembly* AppDomain::RaiseTypeResolveEventThrowing(Assembly* pAssembly, LPCSTR s
ObjToArgSlot(gc.AssemblyRef),
ObjToArgSlot(gc.str)
};
ASSEMBLYREF ResultingAssemblyRef = (ASSEMBLYREF) onTypeResolve.Call_RetOBJECTREF(args);
gc.AssemblyRef = (ASSEMBLYREF) onTypeResolve.Call_RetOBJECTREF(args);

if (ResultingAssemblyRef != NULL)
if (gc.AssemblyRef != NULL)
{
pResolvedAssembly = ResultingAssemblyRef->GetAssembly();
_ASSERTE(CoreLibBinder::IsClass(gc.AssemblyRef->GetMethodTable(), CLASS__ASSEMBLY));

pResolvedAssembly = gc.AssemblyRef->GetAssembly();

if (pResultingAssemblyRef)
*pResultingAssemblyRef = ResultingAssemblyRef;
*pResultingAssemblyRef = gc.AssemblyRef;
else
{
if (pResolvedAssembly->IsCollectible())
Expand Down Expand Up @@ -4023,7 +4025,7 @@ Assembly* AppDomain::RaiseResourceResolveEvent(Assembly* pAssembly, LPCSTR szNam
GCX_COOP();

struct {
OBJECTREF AssemblyRef;
ASSEMBLYREF AssemblyRef;
STRINGREF str;
} gc;
gc.AssemblyRef = NULL;
Expand All @@ -4032,7 +4034,7 @@ Assembly* AppDomain::RaiseResourceResolveEvent(Assembly* pAssembly, LPCSTR szNam
GCPROTECT_BEGIN(gc);

if (pAssembly != NULL)
gc.AssemblyRef=pAssembly->GetExposedObject();
gc.AssemblyRef=(ASSEMBLYREF)pAssembly->GetExposedObject();

MethodDescCallSite onResourceResolve(METHOD__ASSEMBLYLOADCONTEXT__ON_RESOURCE_RESOLVE);
gc.str = StringObject::NewString(szName);
Expand All @@ -4041,10 +4043,12 @@ Assembly* AppDomain::RaiseResourceResolveEvent(Assembly* pAssembly, LPCSTR szNam
ObjToArgSlot(gc.AssemblyRef),
ObjToArgSlot(gc.str)
};
ASSEMBLYREF ResultingAssemblyRef = (ASSEMBLYREF) onResourceResolve.Call_RetOBJECTREF(args);
if (ResultingAssemblyRef != NULL)
gc.AssemblyRef = (ASSEMBLYREF) onResourceResolve.Call_RetOBJECTREF(args);
if (gc.AssemblyRef != NULL)
{
pResolvedAssembly = ResultingAssemblyRef->GetAssembly();
_ASSERTE(CoreLibBinder::IsClass(gc.AssemblyRef->GetMethodTable(), CLASS__ASSEMBLY));

pResolvedAssembly = gc.AssemblyRef->GetAssembly();
if (pResolvedAssembly->IsCollectible())
{
COMPlusThrow(kNotSupportedException, W("NotSupported_CollectibleAssemblyResolve"));
Expand Down Expand Up @@ -4085,7 +4089,7 @@ AppDomain::RaiseAssemblyResolveEvent(
Assembly* pAssembly = NULL;

struct {
OBJECTREF AssemblyRef;
ASSEMBLYREF AssemblyRef;
STRINGREF str;
} gc;
gc.AssemblyRef = NULL;
Expand All @@ -4095,7 +4099,7 @@ AppDomain::RaiseAssemblyResolveEvent(
{
if (pSpec->GetParentAssembly() != NULL)
{
gc.AssemblyRef=pSpec->GetParentAssembly()->GetExposedObject();
gc.AssemblyRef=(ASSEMBLYREF)pSpec->GetParentAssembly()->GetExposedObject();
}

MethodDescCallSite onAssemblyResolve(METHOD__ASSEMBLYLOADCONTEXT__ON_ASSEMBLY_RESOLVE);
Expand All @@ -4106,11 +4110,13 @@ AppDomain::RaiseAssemblyResolveEvent(
ObjToArgSlot(gc.str)
};

ASSEMBLYREF ResultingAssemblyRef = (ASSEMBLYREF) onAssemblyResolve.Call_RetOBJECTREF(args);
gc.AssemblyRef = (ASSEMBLYREF) onAssemblyResolve.Call_RetOBJECTREF(args);

if (ResultingAssemblyRef != NULL)
if (gc.AssemblyRef != NULL)
{
pAssembly = ResultingAssemblyRef->GetAssembly();
_ASSERTE(CoreLibBinder::IsClass(gc.AssemblyRef->GetMethodTable(), CLASS__ASSEMBLY));

pAssembly = gc.AssemblyRef->GetAssembly();
if (pAssembly->IsCollectible())
{
COMPlusThrow(kNotSupportedException, W("NotSupported_CollectibleAssemblyResolve"));
Expand Down Expand Up @@ -4543,6 +4549,8 @@ HRESULT RuntimeInvokeHostAssemblyResolver(INT_PTR pManagedAssemblyLoadContextToB
// If we are here, assembly was successfully resolved via Load or Resolving events.
_ASSERTE(_gcRefs.oRefLoadedAssembly != NULL);

_ASSERTE(CoreLibBinder::IsClass(_gcRefs.oRefLoadedAssembly->GetMethodTable(), CLASS__ASSEMBLY));

// We were able to get the assembly loaded. Now, get its name since the host could have
// performed the resolution using an assembly with different name.
DomainAssembly *pDomainAssembly = _gcRefs.oRefLoadedAssembly->GetDomainAssembly();
Expand Down
6 changes: 3 additions & 3 deletions src/coreclr/vm/corelib.h
Original file line number Diff line number Diff line change
Expand Up @@ -867,11 +867,11 @@ DEFINE_FIELD_U(_id, AssemblyLoadContextBaseObject, _id)
DEFINE_FIELD_U(_state, AssemblyLoadContextBaseObject, _state)
DEFINE_FIELD_U(_isCollectible, AssemblyLoadContextBaseObject, _isCollectible)
DEFINE_CLASS(ASSEMBLYLOADCONTEXT, Loader, AssemblyLoadContext)
DEFINE_METHOD(ASSEMBLYLOADCONTEXT, RESOLVE, Resolve, SM_IntPtr_AssemblyName_RetAssemblyBase)
DEFINE_METHOD(ASSEMBLYLOADCONTEXT, RESOLVE, Resolve, SM_IntPtr_AssemblyName_RetAssembly)
DEFINE_METHOD(ASSEMBLYLOADCONTEXT, RESOLVEUNMANAGEDDLL, ResolveUnmanagedDll, SM_Str_IntPtr_RetIntPtr)
DEFINE_METHOD(ASSEMBLYLOADCONTEXT, RESOLVEUNMANAGEDDLLUSINGEVENT, ResolveUnmanagedDllUsingEvent, SM_Str_AssemblyBase_IntPtr_RetIntPtr)
DEFINE_METHOD(ASSEMBLYLOADCONTEXT, RESOLVEUSINGEVENT, ResolveUsingResolvingEvent, SM_IntPtr_AssemblyName_RetAssemblyBase)
DEFINE_METHOD(ASSEMBLYLOADCONTEXT, RESOLVESATELLITEASSEMBLY, ResolveSatelliteAssembly, SM_IntPtr_AssemblyName_RetAssemblyBase)
DEFINE_METHOD(ASSEMBLYLOADCONTEXT, RESOLVEUSINGEVENT, ResolveUsingResolvingEvent, SM_IntPtr_AssemblyName_RetAssembly)
DEFINE_METHOD(ASSEMBLYLOADCONTEXT, RESOLVESATELLITEASSEMBLY, ResolveSatelliteAssembly, SM_IntPtr_AssemblyName_RetAssembly)
DEFINE_FIELD(ASSEMBLYLOADCONTEXT, ASSEMBLY_LOAD, AssemblyLoad)
DEFINE_METHOD(ASSEMBLYLOADCONTEXT, ON_ASSEMBLY_LOAD, OnAssemblyLoad, SM_Assembly_RetVoid)
DEFINE_METHOD(ASSEMBLYLOADCONTEXT, ON_RESOURCE_RESOLVE, OnResourceResolve, SM_Assembly_Str_RetAssembly)
Expand Down
6 changes: 1 addition & 5 deletions src/coreclr/vm/metasig.h
Original file line number Diff line number Diff line change
Expand Up @@ -393,8 +393,6 @@ DEFINE_METASIG(IM(IntPtr_Int_RetVoid, I i, v))
DEFINE_METASIG(IM(IntInt_RetArrByte, i i, a(b)))
DEFINE_METASIG(IM(RetIntPtr, _, I))
DEFINE_METASIG(IM(RetInt, _, i))
DEFINE_METASIG_T(IM(RetAssemblyName, _, C(ASSEMBLY_NAME)))
DEFINE_METASIG_T(IM(RetAssemblyBase, _, C(ASSEMBLYBASE)))
DEFINE_METASIG_T(IM(RetModule, _, C(MODULE)))
DEFINE_METASIG_T(IM(PtrNativeAssemblyNameParts, P(g(NATIVE_ASSEMBLY_NAME_PARTS)), v))
DEFINE_METASIG(SM(PtrCharPtrVoid, P(u) P(v), v))
Expand Down Expand Up @@ -456,8 +454,6 @@ DEFINE_METASIG(IM(Int_Int_Int_Int_RetVoid, i i i i, v))
DEFINE_METASIG_T(IM(Obj_EventArgs_RetVoid, j C(EVENT_ARGS), v))
DEFINE_METASIG_T(IM(Obj_UnhandledExceptionEventArgs_RetVoid, j C(UNHANDLED_EVENTARGS), v))

DEFINE_METASIG_T(IM(Assembly_RetBool, C(ASSEMBLY), F))
DEFINE_METASIG_T(IM(AssemblyBase_RetBool, C(ASSEMBLYBASE), F))
DEFINE_METASIG_T(IM(Exception_RetVoid, C(EXCEPTION), v))

DEFINE_METASIG(IM(IntPtr_RetObj, I, j))
Expand Down Expand Up @@ -565,7 +561,7 @@ DEFINE_METASIG_T(IM(RefGuid_OutIntPtr_RetCustomQueryInterfaceResult, r(g(GUID))
DEFINE_METASIG_T(SM(RefGuid_RefGuid_RetVoid, r(g(GUID)) r(g(GUID)) , v))
DEFINE_METASIG_T(SM(RefGuid_RetVoid, r(g(GUID)), v))

DEFINE_METASIG_T(SM(IntPtr_AssemblyName_RetAssemblyBase, I C(ASSEMBLY_NAME), C(ASSEMBLYBASE)))
DEFINE_METASIG_T(SM(IntPtr_AssemblyName_RetAssembly, I C(ASSEMBLY_NAME), C(ASSEMBLY)))
DEFINE_METASIG_T(SM(Str_AssemblyBase_IntPtr_RetIntPtr, s C(ASSEMBLYBASE) I, I))
DEFINE_METASIG_T(SM(Str_AssemblyBase_Bool_UInt_RetIntPtr, s C(ASSEMBLYBASE) F K, I))

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1011,8 +1011,11 @@
<data name="Argument_CultureNotSupportedInInvariantMode" xml:space="preserve">
<value>Only the invariant culture is supported in globalization-invariant mode. See https://aka.ms/GlobalizationInvariantMode for more information.</value>
</data>
<data name="Argument_CustomAssemblyLoadContextRequestedNameMismatch" xml:space="preserve">
<value>Resolved assembly's simple name should be the same as of the requested assembly.</value>
<data name="InvalidOperation_ResolvedAssemblyMustBeRuntimeAssembly" xml:space="preserve">
<value>Resolved assembly must be a runtime Assembly object.</value>
</data>
<data name="InvalidOperation_ResolvedAssemblyRequestedNameMismatch" xml:space="preserve">
<value>Resolved assembly's simple name must be the same as of the requested assembly.</value>
</data>
<data name="Argument_CustomCultureCannotBePassedByNumber" xml:space="preserve">
<value>Customized cultures cannot be passed by LCID, only by name.</value>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -604,7 +604,7 @@ public void Dispose()
#if !NATIVEAOT
// This method is invoked by the VM when using the host-provided assembly load context
// implementation.
private static Assembly? Resolve(IntPtr gchManagedAssemblyLoadContext, AssemblyName assemblyName)
private static RuntimeAssembly? Resolve(IntPtr gchManagedAssemblyLoadContext, AssemblyName assemblyName)
{
AssemblyLoadContext context = (AssemblyLoadContext)(GCHandle.FromIntPtr(gchManagedAssemblyLoadContext).Target)!;

Expand Down Expand Up @@ -639,56 +639,44 @@ public void Dispose()
return null;
}

private static Assembly ValidateAssemblyNameWithSimpleName(Assembly assembly, string? requestedSimpleName)
private static RuntimeAssembly ValidateAssemblyNameWithSimpleName(Assembly assembly, string? requestedSimpleName)
{
ArgumentException.ThrowIfNullOrEmpty(requestedSimpleName, "AssemblyName.Name");

// Get the name of the loaded assembly
string? loadedSimpleName = null;

// Derived type's Load implementation is expected to use one of the LoadFrom* methods to get the assembly
// which is a RuntimeAssembly instance. However, since Assembly type can be used build any other artifact (e.g. AssemblyBuilder),
// we need to check for RuntimeAssembly.
RuntimeAssembly? rtLoadedAssembly = GetRuntimeAssembly(assembly);
if (rtLoadedAssembly != null)
RuntimeAssembly? runtimeAssembly = GetRuntimeAssembly(assembly);
if (runtimeAssembly == null)
{
loadedSimpleName = rtLoadedAssembly.GetSimpleName();
throw new InvalidOperationException(SR.InvalidOperation_ResolvedAssemblyMustBeRuntimeAssembly);
}

// The simple names should match at the very least
if (string.IsNullOrEmpty(loadedSimpleName) || !requestedSimpleName.Equals(loadedSimpleName, StringComparison.InvariantCultureIgnoreCase))
if (!requestedSimpleName.Equals(runtimeAssembly.GetSimpleName(), StringComparison.InvariantCultureIgnoreCase))
{
throw new InvalidOperationException(SR.Argument_CustomAssemblyLoadContextRequestedNameMismatch);
throw new InvalidOperationException(SR.InvalidOperation_ResolvedAssemblyRequestedNameMismatch);
}

return assembly;
return runtimeAssembly;
}

private Assembly? ResolveUsingLoad(AssemblyName assemblyName)
private RuntimeAssembly? ResolveUsingLoad(AssemblyName assemblyName)
{
string? simpleName = assemblyName.Name;
Assembly? assembly = Load(assemblyName);

if (assembly != null)
{
assembly = ValidateAssemblyNameWithSimpleName(assembly, simpleName);
}
Assembly? assembly = Load(assemblyName);

return assembly;
return (assembly != null) ? ValidateAssemblyNameWithSimpleName(assembly, simpleName) : null;
}

private Assembly? ResolveUsingEvent(AssemblyName assemblyName)
private RuntimeAssembly? ResolveUsingEvent(AssemblyName assemblyName)
{
string? simpleName = assemblyName.Name;

// Invoke the Resolving event callbacks if wired up
Assembly? assembly = GetFirstResolvedAssemblyFromResolvingEvent(assemblyName);
if (assembly != null)
{
assembly = ValidateAssemblyNameWithSimpleName(assembly, simpleName);
}

return assembly;
return (assembly != null) ? ValidateAssemblyNameWithSimpleName(assembly, simpleName) : null;
}

// This method is called by the VM.
Expand Down Expand Up @@ -755,7 +743,7 @@ internal static void InvokeAssemblyLoadEvent(Assembly assembly)
Justification = "Satellite assemblies have no code in them and loading is not a problem")]
[UnconditionalSuppressMessage("SingleFile", "IL3000: Avoid accessing Assembly file path when publishing as a single file",
Justification = "This call is fine because native call runs before this and checks BindSatelliteResourceFromBundle")]
private Assembly? ResolveSatelliteAssembly(AssemblyName assemblyName)
private RuntimeAssembly? ResolveSatelliteAssembly(AssemblyName assemblyName)
{
// Called by native runtime when CultureName is not empty
Debug.Assert(assemblyName.CultureName?.Length > 0);
Expand All @@ -767,7 +755,7 @@ internal static void InvokeAssemblyLoadEvent(Assembly assembly)

string parentAssemblyName = assemblyName.Name.Substring(0, assemblyName.Name.Length - SatelliteSuffix.Length);

Assembly parentAssembly = LoadFromAssemblyName(new AssemblyName(parentAssemblyName));
RuntimeAssembly parentAssembly = (RuntimeAssembly)LoadFromAssemblyName(new AssemblyName(parentAssemblyName));

AssemblyLoadContext parentALC = GetLoadContext(parentAssembly)!;

Expand All @@ -790,7 +778,7 @@ internal static void InvokeAssemblyLoadEvent(Assembly assembly)
exists = FileSystem.FileExists(assemblyPath);
}

Assembly? asm = exists ? parentALC.LoadFromAssemblyPath(assemblyPath) : null;
RuntimeAssembly? asm = exists ? (RuntimeAssembly?)parentALC.LoadFromAssemblyPath(assemblyPath) : null;
#if CORECLR
if (IsTracingEnabled())
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.IO;
using System.Linq;
using System.Reflection;
using System.Reflection.Emit;
using System.Threading.Tasks;

namespace System.Runtime.Loader.Tests
Expand Down Expand Up @@ -233,5 +234,50 @@ public static void SubclassAssemblyLoadContext_Properties()
Assert.Contains(alc, AssemblyLoadContext.All);
Assert.Empty(alc.Assemblies);
}

class RefEmitLoadContext : AssemblyLoadContext
{
protected override Assembly? Load(AssemblyName assemblyName)
{
return AssemblyBuilder.DefineDynamicAssembly(assemblyName, AssemblyBuilderAccess.Run);
}
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/31804", TestRuntimes.Mono)]
public static void LoadRefEmitAssembly()
{
RefEmitLoadContext alc = new();
alc.Resolving += (sender, assembly) => { Assert.Fail("Resolving event not expected"); return null; };
Exception error = Assert.Throws<FileLoadException>(() => alc.LoadFromAssemblyName(new AssemblyName("MyAssembly")));
Assert.IsType<InvalidOperationException>(error.InnerException);
}

class NonRuntimeAssemblyContext : AssemblyLoadContext
{
class NonRuntimeAssembly : Assembly
{
private AssemblyName _name;

public NonRuntimeAssembly(AssemblyName name) => _name = name;

public override AssemblyName GetName(bool copiedName) => _name;
}

protected override Assembly? Load(AssemblyName assemblyName)
{
return new NonRuntimeAssembly(assemblyName);
}
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsAssemblyLoadingSupported))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/31804", TestRuntimes.Mono)]
public static void LoadNonRuntimeAssembly()
{
NonRuntimeAssemblyContext alc = new();
alc.Resolving += (sender, assembly) => { Assert.Fail("Resolving event not expected"); return null; };
Exception error = Assert.Throws<FileLoadException>(() => alc.LoadFromAssemblyName(new AssemblyName("MyAssembly")));
Assert.IsType<InvalidOperationException>(error.InnerException);
}
}
}
Loading

0 comments on commit e3d15b8

Please sign in to comment.