Skip to content

Commit

Permalink
Move TryGetType to NodeSettings
Browse files Browse the repository at this point in the history
Add NodeSettings.RequireGetType, which handles mixed runtime type resolution (e.g., Framework to Core or different runtime versions).

Embedded debug symbols to get better stack traces in production.
  • Loading branch information
menees committed Oct 8, 2022
1 parent a824148 commit 570fe99
Show file tree
Hide file tree
Showing 6 changed files with 173 additions and 60 deletions.
3 changes: 2 additions & 1 deletion src/Directory.Build.props
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,11 @@
<Nullable>enable</Nullable>
<TreatWarningsAsErrors>true</TreatWarningsAsErrors>
<ProduceReferenceAssembly>false</ProduceReferenceAssembly>
<DebugType>embedded</DebugType>

<!-- Make the assembly, file, and NuGet package versions the same. -->
<!-- https://docs.microsoft.com/en-us/nuget/concepts/package-versioning#pre-release-versions -->
<Version>0.7.0-beta</Version>
<Version>0.8.0-beta</Version>
</PropertyGroup>

<PropertyGroup Condition="'$(Configuration)'=='Debug'">
Expand Down
5 changes: 0 additions & 5 deletions src/Menees.Remoting/IServer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,6 @@ public interface IServer : IDisposable
/// </summary>
Action<Exception>? ReportUnhandledException { get; set; }

/// <summary>
/// See <see cref="Node.TryGetType"/>.
/// </summary>
Func<string, Type?> TryGetType { get; set; }

#endregion

#region Public Methods
Expand Down
50 changes: 2 additions & 48 deletions src/Menees.Remoting/Node.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ public abstract class Node : IDisposable
private readonly string serverPath;

private bool disposed;
private Func<string, Type?> tryGetType = RequireGetType;
private Func<string, Type?> tryGetType;
private ISerializer? systemSerializer;
private ISerializer? userSerializer;
private Func<string, ILogger>? createLogger;
Expand All @@ -35,52 +35,13 @@ public abstract class Node : IDisposable
protected Node(NodeSettings settings)
{
this.serverPath = settings.ServerPath;
this.tryGetType = settings?.TryGetType ?? NodeSettings.RequireGetType;
this.userSerializer = settings?.Serializer;
this.createLogger = settings?.CreateLogger;
}

#endregion

#region Public Properties

/// <summary>
/// Allows customization of how an assembly-qualified type name (serialized from
/// <see cref="Type.AssemblyQualifiedName"/>) should be deserialized into a .NET
/// <see cref="Type"/>.
/// </summary>
/// <remarks>
/// A secure system needs to support a known list of legal/safe/valid types that it
/// can load dynamically. It shouldn't just trust and load an arbitrary assembly and
/// then load an arbitrary type out of it. Doing that can execute malicious code
/// in the current process (e.g., via the Type's static constructor or the assembly's
/// module initializer). So a security best practice is to validate every assembly-
/// qualified type name before you load the type.
/// <para/>
/// However, this is a case where security is at odds with convenience. The default for
/// this property just calls <see cref="Type.GetType(string, bool)"/> to try to load the type,
/// and it throws an exception if the type can't be loaded.
/// <para/>
/// https://github.com/dotnet/runtime/issues/31567#issuecomment-558335944
/// https://stackoverflow.com/a/66963611/1882616
/// https://github.com/dotnet/runtime/issues/43482#issue-722814247 (related Exception comment)
/// </remarks>
public Func<string, Type?> TryGetType
{
get => this.tryGetType;
set
{
if (this.tryGetType != value)
{
this.tryGetType = value ?? RequireGetType;

// On the next serialization, we need to create a new serializer instance using the new tryGetType lambda.
this.systemSerializer = null;
}
}
}

#endregion

#region Internal Properties

internal ISerializer SystemSerializer
Expand Down Expand Up @@ -154,13 +115,6 @@ protected virtual void Dispose(bool disposing)

#endregion

#region Private Methods

private static Type? RequireGetType(string qualifiedTypeName)
=> Type.GetType(qualifiedTypeName, throwOnError: true);

#endregion

#region Private Types

private sealed class ScopedLogger<TScope> : ILogger
Expand Down
104 changes: 104 additions & 0 deletions src/Menees.Remoting/NodeSettings.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

using Menees.Remoting.Security;
using Microsoft.Extensions.Logging;
using System.Reflection;
using System.Runtime.InteropServices;

#endregion

Expand All @@ -12,6 +14,16 @@
/// </summary>
public abstract class NodeSettings
{
#region Private Data Members

// I'm keeping this private for now (even though BaseTests duplicates it) because I may want to
// support other CLR scopes later (e.g., Framework, Core, Mono, Wasm, SQL CLR, Native).
private static readonly bool IsDotNetFramework = RuntimeInformation.FrameworkDescription.Contains("Framework");

private Func<string, Type?> tryGetType = RequireGetType;

#endregion

#region Constructors

/// <summary>
Expand Down Expand Up @@ -60,6 +72,98 @@ protected NodeSettings(string serverPath)
/// </summary>
public NodeSecurity? Security => this.GetSecurity();

/// <summary>
/// Allows customization of how an assembly-qualified type name (serialized from
/// <see cref="Type.AssemblyQualifiedName"/>) should be deserialized into a .NET
/// <see cref="Type"/>.
/// </summary>
/// <remarks>
/// This is useful for type translation and security. It's for translation if you're supporting
/// calls between different runtimes (e.g., Framework and "Core") or versions
/// (e.g., .NET 6.0 and 7.0). When mixing runtimes, many types will be in different
/// assemblies (e.g., int, string, Uri, IPAddress, Stack&lt;T>), so this handler needs
/// to deal with that for all your supported types. Even mixing versions of the same
/// runtime is complicated because strongly-named assemblies embed their version
/// in their AssemblyQualifiedName.
/// <para/>
/// A secure system needs to support a known list of legal/safe/valid types that it
/// can load dynamically. It shouldn't just trust and load an arbitrary assembly and
/// then load an arbitrary type out of it. Doing that can execute malicious code
/// in the current process (e.g., via the Type's static constructor or the assembly's
/// module initializer). So, a security best practice is to validate every assembly-
/// qualified type name before you load the type.
/// <para/>
/// However, this is a case where security is at odds with convenience. The default for
/// this property just calls <see cref="RequireGetType"/> to try to load the type,
/// and it throws an exception if the type can't be loaded.
/// <para/>
/// https://github.com/dotnet/runtime/issues/31567#issuecomment-558335944
/// https://stackoverflow.com/a/66963611/1882616
/// https://github.com/dotnet/runtime/issues/43482#issue-722814247 (related Exception comment)
/// </remarks>
public Func<string, Type?> TryGetType
{
get => this.tryGetType;
set => this.tryGetType = value ?? RequireGetType;
}

#endregion

#region Internal Methods

/// <summary>
/// Loads a type given an assembly-qualified type name.
/// </summary>
/// <param name="typeName">An assembly-qualified type name.</param>
/// <returns>The .NET Type associated with <paramref name="typeName"/>.</returns>
public static Type RequireGetType(string typeName)
{
Type? result = Type.GetType(typeName, throwOnError: false, ignoreCase: true);
if (result == null)
{
// Fallback to the Type.GetType overload where we can pass a custom assembly resolver.
// This is important since a single type name can contain multiple assembly references
// (e.g., Dictionary<string, Uri> includes asm refs for Dictionary<>, string, and Uri).
Assembly[]? assemblies = null;
result = Type.GetType(
typeName,
assemblyName =>
{
Assembly? assembly = null;
string simpleName = assemblyName.Name ?? string.Empty;

// Try to translate the simple built-in scalar types correctly across different runtimes.
if ((IsDotNetFramework && simpleName.Equals("System.Private.CoreLib", StringComparison.OrdinalIgnoreCase))
|| (!IsDotNetFramework && simpleName.Equals("MsCorLib", StringComparison.OrdinalIgnoreCase)))
{
assembly = typeof(string).Assembly;
}
else
{
// See if any assembly is already loaded with the same simple name.
// This ignores versions and strong naming, so it's convenient but insecure.
// We'll allow a lower version to match in case a .NET 7.0 client needs to
// call into a .NET 6.0 server.
// https://github.com/dotnet/fsharp/issues/3408#issuecomment-319519926
assemblies ??= AppDomain.CurrentDomain.GetAssemblies();
assembly = assemblies.FirstOrDefault(asm => asm.GetName().Name?.Equals(simpleName, StringComparison.OrdinalIgnoreCase) ?? false);
}

return assembly;
},
typeResolver: null,
throwOnError: false,
ignoreCase: true);
}

if (result == null)
{
throw new TypeLoadException($"Unable to load type \"{typeName}\".");
}

return result;
}

#endregion

#region Private Protected Methods
Expand Down
54 changes: 54 additions & 0 deletions tests/Menees.Remoting.Tests/NodeSettingsTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
namespace Menees.Remoting;

[TestClass]
public class NodeSettingsTests
{
[TestMethod]
public void RequireGetType()
{
const string CoreStringTypeName = "System.String, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e";
const string FrameworkStringTypeName = "System.String, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089";
TestVersions(typeof(string), CoreStringTypeName, FrameworkStringTypeName);

const string CoreDictionaryTypeName = "System.Collections.Generic.IReadOnlyDictionary`2[" +
"[System.String, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]," +
"[System.Object, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]" +
"], System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e";
const string FrameworkDictionaryTypeName = "System.Collections.Generic.IReadOnlyDictionary`2[" +
"[System.String, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089]," +
"[System.Object, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089]" +
"], mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089";
TestVersions(typeof(IReadOnlyDictionary<string, object>), CoreDictionaryTypeName, FrameworkDictionaryTypeName);

Should.Throw<TypeLoadException>(() => NodeSettings.RequireGetType("This.Type.Does.Not.Exist"));

static void TestVersions(Type expected, params string[] typeNames)
{
foreach (string typeName in typeNames)
{
// Make sure mixed runtimes work correctly for built-in types.
NodeSettings.RequireGetType(typeName).ShouldBe(expected);

// Make sure older and newer versions will resolve to the current version.
NodeSettings.RequireGetType(AdjustVersion(typeName, 1)).ShouldBe(expected);
NodeSettings.RequireGetType(AdjustVersion(typeName, 999)).ShouldBe(expected);
}
}

static string AdjustVersion(string typeName, uint majorVersion)
{
string result = typeName;

const string Prefix = ", Version=";
int startIndex = 0;
while ((startIndex = result.IndexOf(Prefix, startIndex)) >= 0)
{
int endIndex = result.IndexOf('.', startIndex);
result = result.Substring(0, startIndex + Prefix.Length) + majorVersion + result.Substring(endIndex);
startIndex = endIndex;
}

return result;
}
}
}
17 changes: 11 additions & 6 deletions tests/Menees.Remoting.Tests/RmiServerTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,18 @@ private interface IDiamond : ILevel1A, ILevel1B
public void CloneString()
{
string serverPath = this.GenerateServerPath();
string expected = Guid.NewGuid().ToString();
using RmiServer<ICloneable> server = new(expected, serverPath, loggerFactory: this.LoggerFactory);
ServerSettings settings = new(serverPath)
{
CreateLogger = this.LoggerFactory.CreateLogger,

// This is a super weak, insecure example since it just checks for the word "System".
server.TryGetType = typeName => typeName.Contains(nameof(System))
? Type.GetType(typeName, true)
: throw new ArgumentException("TryGetType disallowed " + typeName);
// This is a super weak, insecure example since it just checks for the word "System".
TryGetType = typeName => typeName.Contains(nameof(System))
? Type.GetType(typeName, true)
: throw new ArgumentException("TryGetType disallowed " + typeName),
};

string expected = Guid.NewGuid().ToString();
using RmiServer<ICloneable> server = new(expected, settings);

server.ReportUnhandledException = WriteUnhandledServerException;
server.Start();
Expand Down

0 comments on commit 570fe99

Please sign in to comment.