From c429b2f6f23849d3df6883f87aaeb66a25c02d23 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Tue, 28 Jul 2026 13:50:36 -0700 Subject: [PATCH 01/10] Harden UDT assembly loading against server-supplied assembly names SqlConnection.ResolveTypeAssembly handed the assembly name carried by a server-supplied UDT assembly-qualified name straight to Assembly.Load, and GetUdtValue then invoked a static member on the resolved type without checking that it was a user-defined type at all. Loading an assembly runs its module initializer and invoking a static member runs the type's static constructor, so a compromised or hostile server -- or an attacker on the network path of a connection that has opted out of certificate validation -- could choose which code the client process executes. Add a deny-by-default policy that decides whether an assembly may be loaded before the name reaches the loader: - Restricted (default) permits Microsoft.SqlServer.Types, the application's allow list, assemblies already loaded into the process, and assemblies statically referenced by them. - Strict, via Switch.Microsoft.Data.SqlClient.UseStrictUdtAssemblyLoad, drops the loaded/referenced allowance. - Legacy, via Switch.Microsoft.Data.SqlClient.UseLegacyUdtAssemblyLoad, restores the previous behavior as a compatibility escape hatch and takes precedence over Strict. The Microsoft.SqlServer.Types exemption now pins the public key token as well as the version, so it cannot be satisfied by a same-named assembly on the probing path. Independently of the mode, CheckGetExtendedUDTInfo now rejects a resolved type that is not annotated with SqlUserDefinedTypeAttribute. Reading custom attributes does not run a static constructor, so this is the last point at which the driver can decline without executing any of the type's code, and it covers every call site uniformly. Applications with lazily-loaded custom UDT assemblies can name them through the Microsoft.Data.SqlClient.UdtAssemblyAllowList AppContext data element. The known-assembly-name set is cached and invalidated only by AppDomain.AssemblyLoad, so a server streaming distinct names costs a hash lookup rather than a probe. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60806d12-bdac-413c-bf83-f7c12e3cc12c --- .github/instructions/features.instructions.md | 31 ++ .../Data/SqlClient/LocalAppContextSwitches.cs | 64 +++ .../Microsoft/Data/SqlClient/SqlConnection.cs | 45 +- .../src/Microsoft/Data/SqlClient/SqlUtil.cs | 10 + .../Data/SqlClient/UdtAssemblyPolicy.cs | 475 ++++++++++++++++++ .../src/Resources/Strings.Designer.cs | 18 + .../src/Resources/Strings.resx | 6 + .../Common/LocalAppContextSwitchesHelper.cs | 30 ++ .../SqlClient/LocalAppContextSwitchesTest.cs | 4 + .../SqlClient/UdtAssemblyLoadHardeningTest.cs | 306 +++++++++++ .../Data/SqlClient/UdtAssemblyPolicyTest.cs | 392 +++++++++++++++ 11 files changed, 1379 insertions(+), 2 deletions(-) create mode 100644 src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/UdtAssemblyPolicy.cs create mode 100644 src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyLoadHardeningTest.cs create mode 100644 src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs diff --git a/.github/instructions/features.instructions.md b/.github/instructions/features.instructions.md index 34262b8db6..e82d43834e 100644 --- a/.github/instructions/features.instructions.md +++ b/.github/instructions/features.instructions.md @@ -257,6 +257,37 @@ AppContext switches allow runtime behavior changes without modifying connection | `Switch.Microsoft.Data.SqlClient.UseConnectionPoolV2` | `false` | Enables the new `ChannelDbConnectionPool` implementation | | `Switch.Microsoft.Data.SqlClient.UseManagedNetworkingOnWindows` | `false` | Forces managed SNI on Windows (instead of native SNI) | | `Switch.Microsoft.Data.SqlClient.UseOneSecFloorInTimeoutCalculationDuringLogin` | `false` | Sets 1-second minimum in login timeout calculations | +| `Switch.Microsoft.Data.SqlClient.UseLegacyUdtAssemblyLoad` | `false` | Restores the pre-policy behavior of loading any assembly named by a server-supplied UDT assembly-qualified name, and of skipping the `[SqlUserDefinedType]` check | +| `Switch.Microsoft.Data.SqlClient.UseStrictUdtAssemblyLoad` | `false` | Restricts UDT assembly loads to `Microsoft.SqlServer.Types` and the allow list only, excluding assemblies that merely happen to be present in the process | + +### UDT Assembly Load Policy + +A server-supplied UDT assembly-qualified name reaches `Assembly.Load`, so the +driver applies a deny-by-default policy before handing the name to the loader. + +| Mode | Selected by | Permits | +|------|-------------|---------| +| `Restricted` (default) | neither switch | `Microsoft.SqlServer.Types` (identity pinned), the allow list, assemblies already loaded into the process, and assemblies statically referenced by them | +| `Strict` | `UseStrictUdtAssemblyLoad` | `Microsoft.SqlServer.Types` (identity pinned) and the allow list only | +| `Legacy` | `UseLegacyUdtAssemblyLoad` (wins over `Strict`) | everything, i.e. the pre-policy behavior | + +Applications that use custom UDTs whose assemblies are loaded on demand can name +them explicitly through the `Microsoft.Data.SqlClient.UdtAssemblyAllowList` +AppContext data element, a semicolon-separated list of assembly names: + +```csharp +AppDomain.CurrentDomain.SetData( + "Microsoft.Data.SqlClient.UdtAssemblyAllowList", + "Contoso.Udts;Fabrikam.Udts, Version=2.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a"); +``` + +Each entry is matched only on the components it specifies, so a simple name +permits any version, culture, and public key token, while a fully-qualified name +must match exactly. + +Independently of the mode, a resolved type that is not annotated with +`SqlUserDefinedTypeAttribute` is rejected before any of its code runs (except in +`Legacy` mode). ### Usage Example ```csharp diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs index 06bf6c4f0e..809e03a19f 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs @@ -139,6 +139,23 @@ internal static class LocalAppContextSwitches private const string UseOverallConnectTimeoutForPoolWaitString = "Switch.Microsoft.Data.SqlClient.UseOverallConnectTimeoutForPoolWait"; + /// + /// The name of the app context switch that controls whether the driver + /// loads any assembly named by a server-supplied UDT assembly-qualified + /// name, restoring the behavior that predates the UDT assembly load policy. + /// + private const string UseLegacyUdtAssemblyLoadString = + "Switch.Microsoft.Data.SqlClient.UseLegacyUdtAssemblyLoad"; + + /// + /// The name of the app context switch that controls whether the UDT + /// assembly load policy refuses to load assemblies that are merely present + /// in the process, permitting only the built-in SQL Server CLR types + /// assembly and assemblies named on the application's allow list. + /// + private const string UseStrictUdtAssemblyLoadString = + "Switch.Microsoft.Data.SqlClient.UseStrictUdtAssemblyLoad"; + #if NET /// /// The name of the app context switch that controls whether to use the @@ -258,6 +275,16 @@ private enum SwitchValue : byte /// private static SwitchValue s_useOverallConnectTimeoutForPoolWait = SwitchValue.None; + /// + /// The cached value of the UseLegacyUdtAssemblyLoad switch. + /// + private static SwitchValue s_useLegacyUdtAssemblyLoad = SwitchValue.None; + + /// + /// The cached value of the UseStrictUdtAssemblyLoad switch. + /// + private static SwitchValue s_useStrictUdtAssemblyLoad = SwitchValue.None; + #if NET /// /// The cached value of the UseManagedNetworking switch. @@ -612,6 +639,43 @@ public static bool UseCompatibilityAsyncBehaviour defaultValue: false, ref s_useOverallConnectTimeoutForPoolWait); + /// + /// When set to true, the driver loads any assembly named by a + /// server-supplied UDT assembly-qualified name, which is the behavior that + /// predates the UDT assembly load policy. + /// + /// This switch takes precedence over + /// . Enabling it allows a server, or + /// an attacker on the network path of a connection that has opted out of + /// certificate validation, to choose which assemblies the client process + /// loads, so it should only be used as a temporary compatibility measure. + /// + /// The default value of this switch is false. + /// + public static bool UseLegacyUdtAssemblyLoad => + AcquireAndReturn( + UseLegacyUdtAssemblyLoadString, + defaultValue: false, + ref s_useLegacyUdtAssemblyLoad); + + /// + /// When set to true, the UDT assembly load policy permits only the built-in + /// Microsoft.SqlServer.Types assembly and assemblies named on the + /// application's allow list (the Microsoft.Data.SqlClient.UdtAssemblyAllowList + /// AppContext data element). + /// + /// When false (the default), assemblies that are already loaded into the + /// process, or that are statically referenced by an assembly that is, are + /// also permitted. + /// + /// The default value of this switch is false. + /// + public static bool UseStrictUdtAssemblyLoad => + AcquireAndReturn( + UseStrictUdtAssemblyLoadString, + defaultValue: false, + ref s_useStrictUdtAssemblyLoad); + #if NET /// /// When set to true, .NET on Windows will use the managed SNI diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs index d63571bd55..a30e1cc321 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs @@ -2964,13 +2964,34 @@ private void CopyFrom(SqlConnection connection) private Assembly ResolveTypeAssembly(AssemblyName asmRef, bool throwOnError) { Debug.Assert(TypeSystemAssemblyVersion != null, "TypeSystemAssembly should be set !"); - if (string.Equals(asmRef.Name, "Microsoft.SqlServer.Types", StringComparison.OrdinalIgnoreCase)) + + if (UdtAssemblyPolicy.IsSqlServerTypesAssembly(asmRef)) { if (asmRef.Version != TypeSystemAssemblyVersion && SqlClientEventSource.Log.IsTraceEnabled()) { SqlClientEventSource.Log.TryTraceEvent("SqlConnection.ResolveTypeAssembly | SQL CLR type version change: Server sent {0}, client will instantiate {1}", asmRef.Version, TypeSystemAssemblyVersion); } - asmRef.Version = TypeSystemAssemblyVersion; + + // Pin both the version and the public key token so that the + // built-in exemption cannot be satisfied by a same-named + // assembly that happens to sit on the probing path. + UdtAssemblyPolicy.PinSqlServerTypesIdentity(asmRef, TypeSystemAssemblyVersion); + } + + // The assembly name arrives from the server, and loading an assembly + // runs its module initializer, so the driver must decide whether it + // is willing to load this assembly before it hands the name to the + // loader. + if (!UdtAssemblyPolicy.IsAllowed(asmRef)) + { + SqlClientEventSource.Log.TryTraceEvent("SqlConnection.ResolveTypeAssembly | ERR | UDT assembly '{0}' was not loaded because it is not permitted by the '{1}' UDT assembly load policy.", asmRef.Name, UdtAssemblyPolicy.Mode); + + if (throwOnError) + { + throw SQL.UdtAssemblyNotAllowed(asmRef.Name); + } + + return null; } try @@ -2999,6 +3020,26 @@ internal void CheckGetExtendedUDTInfo(SqlMetaDataPriv metaData, bool fThrow) metaData.udt.Type = Type.GetType(typeName: metaData.udt.AssemblyQualifiedName, assemblyResolver: asmRef => ResolveTypeAssembly(asmRef, fThrow), typeResolver: null, throwOnError: fThrow); + // Nothing has executed any of the resolved type's code yet: + // reading its custom attributes does not run its static + // constructor. This is therefore the last point at which the + // driver can reject a type that the server named but that is not + // actually a user-defined type, and it must happen before + // GetUdtValue invokes anything on it. + if (metaData.udt.Type != null && + !UdtAssemblyPolicy.LegacyBehaviorEnabled && + SqlUdtInfo.TryGetFromType(metaData.udt.Type) == null) + { + SqlClientEventSource.Log.TryTraceEvent("SqlConnection.CheckGetExtendedUDTInfo | ERR | Type '{0}' is not annotated with SqlUserDefinedTypeAttribute and will not be used.", metaData.udt.AssemblyQualifiedName); + + metaData.udt.Type = null; + + if (fThrow) + { + throw SQL.UdtTypeNotUserDefined(metaData.udt.AssemblyQualifiedName); + } + } + if (fThrow && metaData.udt.Type == null) { throw SQL.UDTUnexpectedResult(metaData.udt.AssemblyQualifiedName); diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlUtil.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlUtil.cs index c9388a42f1..5961532711 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlUtil.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlUtil.cs @@ -580,6 +580,16 @@ internal static Exception UDTUnexpectedResult(string exceptionText) return ADP.TypeLoad(StringsHelper.GetString(Strings.SQLUDT_Unexpected, exceptionText)); } + internal static Exception UdtAssemblyNotAllowed(string assemblyName) + { + return ADP.TypeLoad(StringsHelper.GetString(Strings.SQLUDT_AssemblyNotAllowed, assemblyName)); + } + + internal static Exception UdtTypeNotUserDefined(string assemblyQualifiedName) + { + return ADP.TypeLoad(StringsHelper.GetString(Strings.SQLUDT_TypeNotUserDefined, assemblyQualifiedName)); + } + internal static Exception ConversionOverflow() { return new OverflowException(StringsHelper.GetString(Strings.SqlMisc_ConversionOverflowMessage)); diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/UdtAssemblyPolicy.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/UdtAssemblyPolicy.cs new file mode 100644 index 0000000000..4c1226ab01 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/UdtAssemblyPolicy.cs @@ -0,0 +1,475 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Collections.Generic; +using System.Reflection; +using System.Threading; +using Microsoft.Data.Common; +using Microsoft.Data.SqlClient.Internal; + +#nullable enable + +namespace Microsoft.Data.SqlClient; + +/// +/// The policy modes that govern which assemblies the driver is willing to load +/// while resolving a server-supplied UDT assembly-qualified name. +/// +internal enum UdtAssemblyLoadMode +{ + /// + /// Only the pinned Microsoft.SqlServer.Types assembly and assemblies + /// named on the application-supplied allow list may be loaded. + /// + Strict, + + /// + /// The set, plus assemblies that are already loaded + /// into the process and assemblies that are statically referenced by + /// already-loaded assemblies. This is the default. + /// + Restricted, + + /// + /// Any assembly named by the server may be loaded. This restores the + /// behavior of the driver prior to the introduction of this policy and is + /// not recommended. + /// + Legacy +} + +/// +/// Decides whether the driver may load an assembly named by a server-supplied +/// UDT assembly-qualified name. +/// +/// A TDS response describing a UDT column or output parameter carries an +/// AssemblyQualifiedName that the driver must resolve to a CLR +/// . Resolving it involves loading the named assembly, and +/// loading an assembly executes that assembly's module initializer. A server +/// (or an on-path attacker against a connection that has opted out of +/// certificate validation) therefore gets to choose which assembly the client +/// process loads unless the driver constrains the choice, which is what this +/// class does. +/// +/// The evaluation is deliberately cheap: apart from a one-time subscription to +/// , a decision is a couple of hash-set +/// lookups. The set of known assembly names is rebuilt only when an assembly +/// is actually loaded into the process, so a hostile server that streams a +/// large number of distinct assembly names cannot force repeated disk probing. +/// +internal static class UdtAssemblyPolicy +{ + #region Constants + + /// + /// The simple name of the assembly that ships the built-in SQL Server CLR + /// types (geography, geometry, hierarchyid). It is always permitted, but + /// only with the identity pinned by + /// . + /// + internal const string SqlServerTypesAssemblyName = "Microsoft.SqlServer.Types"; + + /// + /// The name of the AppContext data element that holds the application's + /// UDT assembly allow list. The value is a string containing one or more + /// assembly names separated by semicolons. An entry may be a simple name + /// (Contoso.Udts), in which case only the simple name is compared, + /// or a full assembly name + /// (Contoso.Udts, Version=1.0.0.0, Culture=neutral, PublicKeyToken=...), + /// in which case every component that the entry specifies must also match. + /// + internal const string AllowListAppContextDataName = + "Microsoft.Data.SqlClient.UdtAssemblyAllowList"; + + /// + /// The public key token that every shipped build of + /// Microsoft.SqlServer.Types is signed with. + /// + private static readonly byte[] s_sqlServerTypesPublicKeyToken = + { 0x89, 0x84, 0x5d, 0xcd, 0x80, 0x80, 0xcc, 0x91 }; + + #endregion + + #region Fields + + /// + /// Guards the cached allow list and known-assembly-name set. + /// + private static readonly object s_lock = new(); + + /// + /// Incremented every time an assembly is loaded into the process. Used to + /// invalidate . Written with + /// from the + /// callback and only read while is held. + /// + private static int s_assemblyLoadVersion; + + /// + /// Set to true once the handler has + /// been attached. The handler is attached lazily so that applications that + /// never read a UDT value never pay for it. + /// + private static bool s_assemblyLoadHandlerAttached; + + /// + /// The simple names of every assembly that is loaded into the process, plus + /// the simple names of every assembly they statically reference. Null when + /// it has not been built yet. + /// + private static HashSet? s_knownAssemblyNames; + + /// + /// The value of at the time + /// was built. + /// + private static int s_knownAssemblyNamesVersion = -1; + + /// + /// The raw allow list string that was parsed + /// from, used to detect that the application has changed it. + /// + private static string? s_allowListSource; + + /// + /// The parsed allow list. Null when it has not been parsed yet. + /// + private static List? s_allowList; + + #endregion + + #region Properties + + /// + /// The policy mode currently in effect. + /// + internal static UdtAssemblyLoadMode Mode + { + get + { + // Legacy wins over Strict so that an application that has opted + // back into the old behavior gets it unambiguously. + if (LocalAppContextSwitches.UseLegacyUdtAssemblyLoad) + { + return UdtAssemblyLoadMode.Legacy; + } + + return LocalAppContextSwitches.UseStrictUdtAssemblyLoad + ? UdtAssemblyLoadMode.Strict + : UdtAssemblyLoadMode.Restricted; + } + } + + /// + /// True when the policy has been disabled entirely in favor of the + /// pre-policy behavior. + /// + internal static bool LegacyBehaviorEnabled => Mode == UdtAssemblyLoadMode.Legacy; + + #endregion + + #region Methods + + /// + /// Determines whether names the built-in SQL + /// Server CLR types assembly. + /// + internal static bool IsSqlServerTypesAssembly(AssemblyName asmRef) => + string.Equals(asmRef.Name, SqlServerTypesAssemblyName, StringComparison.OrdinalIgnoreCase); + + /// + /// Pins the identity of the built-in SQL Server CLR types assembly. + /// + /// The version is normalized to the type system version negotiated for the + /// connection, which is long-standing behavior: the server advertises the + /// version it holds, and the client instantiates the version it has. + /// + /// The public key token is normalized to the token that Microsoft signs the + /// assembly with. Without this, a server that omits the token (or supplies + /// a different one) would cause a partial-name bind that an unsigned + /// same-named assembly on the probing path could satisfy. + /// + /// The assembly reference to normalize, in place. + /// + /// The type system assembly version negotiated for the connection. + /// + internal static void PinSqlServerTypesIdentity(AssemblyName asmRef, Version typeSystemAssemblyVersion) + { + asmRef.Version = typeSystemAssemblyVersion; + asmRef.SetPublicKeyToken((byte[])s_sqlServerTypesPublicKeyToken.Clone()); + } + + /// + /// Determines whether the driver is permitted to load the assembly named by + /// . + /// + /// The server-supplied assembly reference. + /// True when the assembly may be loaded. + internal static bool IsAllowed(AssemblyName asmRef) + { + UdtAssemblyLoadMode mode = Mode; + + if (mode == UdtAssemblyLoadMode.Legacy) + { + return true; + } + + string? simpleName = asmRef.Name; + if (string.IsNullOrEmpty(simpleName)) + { + return false; + } + + // The built-in types assembly is always permitted. Its identity has + // already been pinned by the caller, so this cannot be satisfied by an + // arbitrary assembly that merely borrows the name. + if (IsSqlServerTypesAssembly(asmRef)) + { + return true; + } + + if (MatchesAllowList(asmRef)) + { + return true; + } + + if (mode == UdtAssemblyLoadMode.Restricted && IsKnownToProcess(simpleName!)) + { + return true; + } + + return false; + } + + /// + /// Discards all cached state. Intended for use by tests, which need to + /// observe the effect of changing the allow list or the policy switches. + /// + internal static void ResetCache() + { + lock (s_lock) + { + s_allowList = null; + s_allowListSource = null; + s_knownAssemblyNames = null; + s_knownAssemblyNamesVersion = -1; + } + } + + #endregion + + #region Helpers + + /// + /// Determines whether matches an entry on the + /// application-supplied allow list. + /// + private static bool MatchesAllowList(AssemblyName asmRef) + { + List allowList = GetAllowList(); + + for (int i = 0; i < allowList.Count; i++) + { + if (Matches(allowList[i], asmRef)) + { + return true; + } + } + + return false; + } + + /// + /// Determines whether a server-supplied assembly reference satisfies an + /// allow list entry. Only the components that the entry actually specifies + /// are compared, so a simple-name entry permits any version, culture, and + /// public key token. + /// + private static bool Matches(AssemblyName allowed, AssemblyName candidate) + { + if (!string.Equals(allowed.Name, candidate.Name, StringComparison.OrdinalIgnoreCase)) + { + return false; + } + + if (allowed.Version is not null && !allowed.Version.Equals(candidate.Version)) + { + return false; + } + + // AssemblyName.CultureName is the empty string for the neutral culture + // and null when the entry did not specify a culture at all. + if (allowed.CultureName is not null && + !string.Equals(allowed.CultureName, candidate.CultureName ?? string.Empty, StringComparison.OrdinalIgnoreCase)) + { + return false; + } + + byte[]? allowedToken = allowed.GetPublicKeyToken(); + if (allowedToken is { Length: > 0 }) + { + byte[]? candidateToken = candidate.GetPublicKeyToken(); + if (candidateToken is null || candidateToken.Length != allowedToken.Length) + { + return false; + } + + for (int i = 0; i < allowedToken.Length; i++) + { + if (allowedToken[i] != candidateToken[i]) + { + return false; + } + } + } + + return true; + } + + /// + /// Returns the parsed allow list, re-parsing it if the application has + /// changed the underlying AppContext data since it was last read. + /// + private static List GetAllowList() + { + string source = AppContext.GetData(AllowListAppContextDataName) as string ?? string.Empty; + + lock (s_lock) + { + if (s_allowList is not null && string.Equals(s_allowListSource, source, StringComparison.Ordinal)) + { + return s_allowList; + } + + List parsed = new(); + + foreach (string entry in source.Split(';')) + { + string trimmed = entry.Trim(); + if (trimmed.Length == 0) + { + continue; + } + + try + { + AssemblyName name = new(trimmed); + if (!string.IsNullOrEmpty(name.Name)) + { + parsed.Add(name); + } + } + catch (Exception e) when (ADP.IsCatchableExceptionType(e)) + { + // A malformed entry must not take down the application, and + // it must not silently widen the policy either, so it is + // traced and skipped. + SqlClientEventSource.Log.TryTraceEvent( + "UdtAssemblyPolicy.GetAllowList | ERR | Ignoring malformed UDT assembly allow list entry '{0}'.", + trimmed); + } + } + + s_allowList = parsed; + s_allowListSource = source; + + return parsed; + } + } + + /// + /// Determines whether an assembly with the given simple name is already + /// loaded into the process, or is statically referenced by an assembly that + /// is. + /// + private static bool IsKnownToProcess(string simpleName) => + GetKnownAssemblyNames().Contains(simpleName); + + /// + /// Returns the set of assembly simple names that are loaded into the + /// process or referenced by an assembly that is, rebuilding it only if an + /// assembly has been loaded since it was last built. + /// + private static HashSet GetKnownAssemblyNames() + { + EnsureAssemblyLoadHandlerAttached(); + + lock (s_lock) + { + int version = s_assemblyLoadVersion; + + if (s_knownAssemblyNames is not null && s_knownAssemblyNamesVersion == version) + { + return s_knownAssemblyNames; + } + + HashSet names = new(StringComparer.OrdinalIgnoreCase); + + foreach (Assembly assembly in AppDomain.CurrentDomain.GetAssemblies()) + { + if (assembly.IsDynamic) + { + // A dynamic assembly has no manifest to read references + // from, and it cannot be a target of Assembly.Load by name + // anyway. + continue; + } + + string? name = assembly.GetName().Name; + if (!string.IsNullOrEmpty(name)) + { + names.Add(name!); + } + + try + { + foreach (AssemblyName reference in assembly.GetReferencedAssemblies()) + { + if (!string.IsNullOrEmpty(reference.Name)) + { + names.Add(reference.Name!); + } + } + } + catch (Exception e) when (ADP.IsCatchableExceptionType(e)) + { + // Reading the reference list can fail for assemblies loaded + // from a byte array or produced by a trimmer. Losing one + // assembly's references only makes the policy stricter. + SqlClientEventSource.Log.TryTraceEvent( + "UdtAssemblyPolicy.GetKnownAssemblyNames | INFO | Unable to read references of '{0}'.", + name); + } + } + + s_knownAssemblyNames = names; + s_knownAssemblyNamesVersion = version; + + return names; + } + } + + /// + /// Attaches the assembly load handler that invalidates the cached + /// known-assembly-name set, if it has not been attached already. + /// + private static void EnsureAssemblyLoadHandlerAttached() + { + lock (s_lock) + { + if (s_assemblyLoadHandlerAttached) + { + return; + } + + AppDomain.CurrentDomain.AssemblyLoad += static (_, _) => + Interlocked.Increment(ref s_assemblyLoadVersion); + + s_assemblyLoadHandlerAttached = true; + } + } + + #endregion +} diff --git a/src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs b/src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs index 7064a6c19c..8649956ce9 100644 --- a/src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs +++ b/src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs @@ -5082,6 +5082,24 @@ internal static string SQLUDT_Unexpected { } } + /// + /// Looks up a localized string similar to The assembly '{0}', named by a user-defined type returned by the server, was not loaded because it is not permitted by the user-defined type assembly load policy. To permit it, add the assembly name to the 'Microsoft.Data.SqlClient.UdtAssemblyAllowList' AppContext data element.. + /// + internal static string SQLUDT_AssemblyNotAllowed { + get { + return ResourceManager.GetString("SQLUDT_AssemblyNotAllowed", resourceCulture); + } + } + + /// + /// Looks up a localized string similar to The type '{0}', named by a user-defined type returned by the server, is not a user-defined type because it is not annotated with SqlUserDefinedTypeAttribute.. + /// + internal static string SQLUDT_TypeNotUserDefined { + get { + return ResourceManager.GetString("SQLUDT_TypeNotUserDefined", resourceCulture); + } + } + /// /// Looks up a localized string similar to UdtTypeName property must be set only for UDT parameters.. /// diff --git a/src/Microsoft.Data.SqlClient/src/Resources/Strings.resx b/src/Microsoft.Data.SqlClient/src/Resources/Strings.resx index e7f438873f..55cd033d12 100644 --- a/src/Microsoft.Data.SqlClient/src/Resources/Strings.resx +++ b/src/Microsoft.Data.SqlClient/src/Resources/Strings.resx @@ -1122,6 +1122,12 @@ unexpected error encountered in SqlClient data provider. {0} + + The assembly '{0}', named by a user-defined type returned by the server, was not loaded because it is not permitted by the user-defined type assembly load policy. To permit it, add the assembly name to the 'Microsoft.Data.SqlClient.UdtAssemblyAllowList' AppContext data element. + + + The type '{0}', named by a user-defined type returned by the server, is not a user-defined type because it is not annotated with SqlUserDefinedTypeAttribute. + UdtTypeName property must be set for UDT parameters. diff --git a/src/Microsoft.Data.SqlClient/tests/Common/LocalAppContextSwitchesHelper.cs b/src/Microsoft.Data.SqlClient/tests/Common/LocalAppContextSwitchesHelper.cs index 49ad2712ec..6818c43727 100644 --- a/src/Microsoft.Data.SqlClient/tests/Common/LocalAppContextSwitchesHelper.cs +++ b/src/Microsoft.Data.SqlClient/tests/Common/LocalAppContextSwitchesHelper.cs @@ -59,6 +59,8 @@ public sealed class LocalAppContextSwitchesHelper : IDisposable private readonly bool? _useConnectionPoolV2Original; private readonly bool? _useLegacyIdleTimeoutBehaviorOriginal; private readonly bool? _useOverallConnectTimeoutForPoolWaitOriginal; + private readonly bool? _useLegacyUdtAssemblyLoadOriginal; + private readonly bool? _useStrictUdtAssemblyLoadOriginal; #if NET // The s_useManagedNetworking field only exists in the SqlClient assembly // when it is built for .NET on Windows, so it is captured/restored at @@ -127,6 +129,10 @@ public LocalAppContextSwitchesHelper() GetSwitchValue("s_useLegacyIdleTimeoutBehavior"); _useOverallConnectTimeoutForPoolWaitOriginal = GetSwitchValue("s_useOverallConnectTimeoutForPoolWait"); + _useLegacyUdtAssemblyLoadOriginal = + GetSwitchValue("s_useLegacyUdtAssemblyLoad"); + _useStrictUdtAssemblyLoadOriginal = + GetSwitchValue("s_useStrictUdtAssemblyLoad"); #if NET if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) { @@ -203,6 +209,12 @@ public void Dispose() SetSwitchValue( "s_useOverallConnectTimeoutForPoolWait", _useOverallConnectTimeoutForPoolWaitOriginal); + SetSwitchValue( + "s_useLegacyUdtAssemblyLoad", + _useLegacyUdtAssemblyLoadOriginal); + SetSwitchValue( + "s_useStrictUdtAssemblyLoad", + _useStrictUdtAssemblyLoadOriginal); #if NET if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) { @@ -370,6 +382,24 @@ public bool? UseOverallConnectTimeoutForPoolWait set => SetSwitchValue("s_useOverallConnectTimeoutForPoolWait", value); } + /// + /// Get or set the UseLegacyUdtAssemblyLoad switch value. + /// + public bool? UseLegacyUdtAssemblyLoad + { + get => GetSwitchPropertyValue(nameof(UseLegacyUdtAssemblyLoad)); + set => SetSwitchValue("s_useLegacyUdtAssemblyLoad", value); + } + + /// + /// Get or set the UseStrictUdtAssemblyLoad switch value. + /// + public bool? UseStrictUdtAssemblyLoad + { + get => GetSwitchPropertyValue(nameof(UseStrictUdtAssemblyLoad)); + set => SetSwitchValue("s_useStrictUdtAssemblyLoad", value); + } + #if NET /// /// Get or set the UseManagedNetworking switch value. diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/LocalAppContextSwitchesTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/LocalAppContextSwitchesTest.cs index c159389bf7..49a916b412 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/LocalAppContextSwitchesTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/LocalAppContextSwitchesTest.cs @@ -43,6 +43,8 @@ public void TestDefaultAppContextSwitchValues() switchesHelper.UseConnectionPoolV2 = null; switchesHelper.UseLegacyIdleTimeoutBehavior = null; switchesHelper.UseMinimumLoginTimeout = null; + switchesHelper.UseLegacyUdtAssemblyLoad = null; + switchesHelper.UseStrictUdtAssemblyLoad = null; #if NET switchesHelper.GlobalizationInvariantMode = null; if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) @@ -68,6 +70,8 @@ public void TestDefaultAppContextSwitchValues() Assert.False(switchesHelper.IgnoreServerProvidedFailoverPartner); Assert.False(switchesHelper.UseLegacyFailoverAlternationOnLoginSqlErrors); Assert.False(switchesHelper.EnableMultiSubnetFailoverByDefault); + Assert.False(switchesHelper.UseLegacyUdtAssemblyLoad); + Assert.False(switchesHelper.UseStrictUdtAssemblyLoad); #if NET Assert.False(switchesHelper.GlobalizationInvariantMode); if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyLoadHardeningTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyLoadHardeningTest.cs new file mode 100644 index 0000000000..90a58646ac --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyLoadHardeningTest.cs @@ -0,0 +1,306 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Collections.Generic; +using System.Reflection; +using Microsoft.Data.SqlClient.Tests.Common; +using Microsoft.SqlServer.Server; +using Xunit; + +namespace Microsoft.Data.SqlClient.UnitTests; + +/// +/// Provides regression tests for the UDT assembly load hardening, driving +/// directly with the kind of +/// assembly-qualified name a hostile or compromised server could return. +/// +/// +/// Before the fix, handed any server-supplied +/// assembly name straight to , which +/// runs the target assembly's module initializer, and then invoked a static +/// member on the resolved type without checking that it was a user-defined type +/// at all, which runs the type's static constructor. These tests assert that +/// neither happens. +/// +public class UdtAssemblyLoadHardeningTest +{ + /// + /// A connection string that is never opened. Only the parsed connection + /// options are needed, so that the type system assembly version the policy + /// pins against is available. + /// + private const string ConnectionString = "Data Source=localhost;Integrated Security=true"; + + /// + /// The assembly-qualified name of a type in an assembly that is neither + /// loaded into the test process nor referenced by anything that is. + /// + private const string HostileAssemblyQualifiedName = + "Contoso.Evil.Payload, Contoso.Evil, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null"; + + #region Assembly load policy + + /// + /// Verifies that resolving a UDT whose assembly is not permitted never + /// reaches the assembly loader, and reports a policy failure rather than + /// silently succeeding. + /// + [Fact] + public void CheckGetExtendedUDTInfo_UnknownAssembly_IsNeverLoaded() + { + using PolicyScope scope = new(); + using AssemblyLoadRecorder recorder = new(); + + SqlConnection connection = new(ConnectionString); + SqlMetaDataPriv metaData = CreateUdtMetaData(HostileAssemblyQualifiedName); + + Exception exception = Record.Exception( + () => connection.CheckGetExtendedUDTInfo(metaData, fThrow: true)); + + Assert.NotNull(exception); + Assert.Null(metaData.udt.Type); + Assert.DoesNotContain("Contoso.Evil", recorder.LoadedNames); + } + + /// + /// Verifies that the non-throwing call sites (for example + /// SqlDataReader.GetFieldType) still tolerate a denied assembly, leaving the + /// resolved type null instead of faulting the read. + /// + [Fact] + public void CheckGetExtendedUDTInfo_UnknownAssembly_DoesNotThrowWhenNotRequested() + { + using PolicyScope scope = new(); + using AssemblyLoadRecorder recorder = new(); + + SqlConnection connection = new(ConnectionString); + SqlMetaDataPriv metaData = CreateUdtMetaData(HostileAssemblyQualifiedName); + + connection.CheckGetExtendedUDTInfo(metaData, fThrow: false); + + Assert.Null(metaData.udt.Type); + Assert.DoesNotContain("Contoso.Evil", recorder.LoadedNames); + } + + /// + /// Verifies that the legacy switch restores the pre-fix behavior, so an + /// application that depends on it has a documented escape hatch. The load + /// is still expected to fail, because the assembly does not exist, but it + /// must fail in the loader rather than in the policy. + /// + [Fact] + public void CheckGetExtendedUDTInfo_LegacyMode_ReachesTheLoader() + { + using PolicyScope scope = new(legacy: true); + + SqlConnection connection = new(ConnectionString); + SqlMetaDataPriv metaData = CreateUdtMetaData(HostileAssemblyQualifiedName); + + Exception exception = Record.Exception( + () => connection.CheckGetExtendedUDTInfo(metaData, fThrow: true)); + + // The loader, not the policy, is what refuses the load in legacy mode. + Assert.IsAssignableFrom(exception); + } + + #endregion + + #region User-defined type validation + + /// + /// Verifies that a type which resolves successfully but is not annotated + /// with SqlUserDefinedTypeAttribute is rejected before any of its code can + /// run. + /// + /// + /// This is the second half of the vulnerability: GetUdtValue's null branch + /// calls InvokeMember("Null", ... Static ...) on the resolved type, which + /// runs its static constructor. Rejecting the type in + /// CheckGetExtendedUDTInfo is the last point at which the driver can decline + /// without executing anything, because reading custom attributes does not + /// trigger a static constructor. + /// + [Fact] + public void CheckGetExtendedUDTInfo_TypeWithoutUdtAttribute_IsRejected() + { + using PolicyScope scope = new(); + + // This test assembly is loaded, so the assembly load policy permits it + // in the default Restricted mode; only the attribute check stands + // between the server-supplied name and the type's code. + SqlConnection connection = new(ConnectionString); + SqlMetaDataPriv metaData = CreateUdtMetaData( + typeof(NotAUserDefinedType).AssemblyQualifiedName!); + + Exception exception = Record.Exception( + () => connection.CheckGetExtendedUDTInfo(metaData, fThrow: true)); + + Assert.NotNull(exception); + Assert.Null(metaData.udt.Type); + Assert.False( + StaticConstructorMarker.Ran, + "The type's static constructor must not have been triggered."); + } + + /// + /// Verifies that a legitimate user-defined type in a permitted assembly is + /// still resolved, so the hardening does not break the supported scenario. + /// + [Fact] + public void CheckGetExtendedUDTInfo_UserDefinedType_IsResolved() + { + using PolicyScope scope = new(); + + SqlConnection connection = new(ConnectionString); + SqlMetaDataPriv metaData = CreateUdtMetaData( + typeof(AUserDefinedType).AssemblyQualifiedName!); + + connection.CheckGetExtendedUDTInfo(metaData, fThrow: true); + + Assert.Equal(typeof(AUserDefinedType), metaData.udt.Type); + } + + /// + /// Verifies that legacy mode also bypasses the user-defined type check, so + /// the switch fully restores the previous behavior. + /// + [Fact] + public void CheckGetExtendedUDTInfo_LegacyMode_SkipsUdtAttributeCheck() + { + using PolicyScope scope = new(legacy: true); + + SqlConnection connection = new(ConnectionString); + SqlMetaDataPriv metaData = CreateUdtMetaData( + typeof(NotAUserDefinedType).AssemblyQualifiedName!); + + connection.CheckGetExtendedUDTInfo(metaData, fThrow: true); + + Assert.Equal(typeof(NotAUserDefinedType), metaData.udt.Type); + } + + #endregion + + #region Helpers + + private static SqlMetaDataPriv CreateUdtMetaData(string assemblyQualifiedName) => + new() + { + udt = new SqlMetaDataUdt + { + DatabaseName = "db", + SchemaName = "dbo", + TypeName = "udt", + AssemblyQualifiedName = assemblyQualifiedName, + }, + }; + + /// + /// Forces the policy switches to known values and clears the allow list and + /// every policy cache for the duration of a test. + /// + private sealed class PolicyScope : IDisposable + { + private readonly LocalAppContextSwitchesHelper _switches; + private readonly object? _originalAllowList; + + public PolicyScope(bool legacy = false, bool strict = false) + { + _switches = new LocalAppContextSwitchesHelper(); + _originalAllowList = + AppContext.GetData(UdtAssemblyPolicy.AllowListAppContextDataName); + + _switches.UseLegacyUdtAssemblyLoad = legacy; + _switches.UseStrictUdtAssemblyLoad = strict; + + AppDomain.CurrentDomain.SetData( + UdtAssemblyPolicy.AllowListAppContextDataName, + null); + UdtAssemblyPolicy.ResetCache(); + } + + public void Dispose() + { + AppDomain.CurrentDomain.SetData( + UdtAssemblyPolicy.AllowListAppContextDataName, + _originalAllowList); + UdtAssemblyPolicy.ResetCache(); + _switches.Dispose(); + } + } + + /// + /// Records the simple name of every assembly loaded into the process while + /// it is alive, so a test can assert that a load never happened. + /// + private sealed class AssemblyLoadRecorder : IDisposable + { + private readonly List _loadedNames = new(); + + public AssemblyLoadRecorder() + { + AppDomain.CurrentDomain.AssemblyLoad += OnAssemblyLoad; + } + + public IReadOnlyList LoadedNames + { + get + { + lock (_loadedNames) + { + return _loadedNames.ToArray(); + } + } + } + + public void Dispose() => + AppDomain.CurrentDomain.AssemblyLoad -= OnAssemblyLoad; + + private void OnAssemblyLoad(object? sender, AssemblyLoadEventArgs args) + { + string? name = args.LoadedAssembly.GetName().Name; + if (name is not null) + { + lock (_loadedNames) + { + _loadedNames.Add(name); + } + } + } + } + + /// + /// A type that a hostile server could name but that is not a user-defined + /// type. Its static constructor records that it ran so a test can prove it + /// did not. + /// + private sealed class NotAUserDefinedType + { + static NotAUserDefinedType() + { + StaticConstructorMarker.Ran = true; + } + } + + /// + /// Holds the flag that 's static + /// constructor sets. It lives in a separate class so that reading it does + /// not itself trigger the constructor under test. + /// + private static class StaticConstructorMarker + { + internal static bool Ran; + } + + /// + /// A well-formed user-defined type, used to prove the hardening does not + /// reject legitimate types. + /// + [SqlUserDefinedType(Format.UserDefined, MaxByteSize = 8)] + private sealed class AUserDefinedType + { + } + + #endregion +} diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs new file mode 100644 index 0000000000..afa6e77fc0 --- /dev/null +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs @@ -0,0 +1,392 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Reflection; +using Microsoft.Data.SqlClient.Tests.Common; +using Xunit; + +namespace Microsoft.Data.SqlClient.UnitTests; + +/// +/// Provides unit tests for , the deny-by-default +/// policy that governs which assemblies the driver is willing to load while +/// resolving a server-supplied UDT assembly-qualified name. +/// +public class UdtAssemblyPolicyTest +{ + /// + /// The public key token that Microsoft signs Microsoft.SqlServer.Types with. + /// + private const string SqlServerTypesPublicKeyToken = "89845dcd8080cc91"; + + /// + /// An assembly name that is neither loaded into the test process nor + /// referenced by anything that is. + /// + private const string UnknownAssemblyName = "Contoso.Totally.Unknown.Assembly"; + + #region Scope + + /// + /// Acquires the app context switch lock, forces the policy switches to + /// known values, and clears the allow list and every policy cache. Disposal + /// restores the original switch values and allow list, and clears the caches + /// again so no state leaks into the next test. + /// + private sealed class PolicyScope : IDisposable + { + private readonly LocalAppContextSwitchesHelper _switches; + private readonly object? _originalAllowList; + + public PolicyScope(bool legacy = false, bool strict = false) + { + _switches = new LocalAppContextSwitchesHelper(); + _originalAllowList = + AppContext.GetData(UdtAssemblyPolicy.AllowListAppContextDataName); + + _switches.UseLegacyUdtAssemblyLoad = legacy; + _switches.UseStrictUdtAssemblyLoad = strict; + + SetAllowList(null); + } + + public static void SetAllowList(string? value) + { + AppDomain.CurrentDomain.SetData( + UdtAssemblyPolicy.AllowListAppContextDataName, + value); + UdtAssemblyPolicy.ResetCache(); + } + + public void Dispose() + { + AppDomain.CurrentDomain.SetData( + UdtAssemblyPolicy.AllowListAppContextDataName, + _originalAllowList); + UdtAssemblyPolicy.ResetCache(); + _switches.Dispose(); + } + } + + #endregion + + #region Mode + + /// + /// Verifies that the policy defaults to Restricted when neither switch is + /// set. + /// + [Fact] + public void Mode_DefaultsToRestricted() + { + using PolicyScope scope = new(); + + Assert.Equal(UdtAssemblyLoadMode.Restricted, UdtAssemblyPolicy.Mode); + Assert.False(UdtAssemblyPolicy.LegacyBehaviorEnabled); + } + + /// + /// Verifies that the strict switch selects Strict mode. + /// + [Fact] + public void Mode_StrictSwitch_SelectsStrict() + { + using PolicyScope scope = new(strict: true); + + Assert.Equal(UdtAssemblyLoadMode.Strict, UdtAssemblyPolicy.Mode); + Assert.False(UdtAssemblyPolicy.LegacyBehaviorEnabled); + } + + /// + /// Verifies that the legacy switch selects Legacy mode and takes precedence + /// over the strict switch, so an application that has opted back into the + /// old behavior gets it unambiguously. + /// + [Theory] + [InlineData(false)] + [InlineData(true)] + public void Mode_LegacySwitch_WinsOverStrict(bool strict) + { + using PolicyScope scope = new(legacy: true, strict: strict); + + Assert.Equal(UdtAssemblyLoadMode.Legacy, UdtAssemblyPolicy.Mode); + Assert.True(UdtAssemblyPolicy.LegacyBehaviorEnabled); + } + + #endregion + + #region SqlServerTypes + + /// + /// Verifies that the built-in SQL Server CLR types assembly is recognized + /// case-insensitively and is permitted in every non-legacy mode. + /// + [Theory] + [InlineData(false)] + [InlineData(true)] + public void IsAllowed_SqlServerTypes_IsPermitted(bool strict) + { + using PolicyScope scope = new(strict: strict); + + Assert.True(UdtAssemblyPolicy.IsSqlServerTypesAssembly( + new AssemblyName("microsoft.sqlserver.types"))); + Assert.True(UdtAssemblyPolicy.IsAllowed( + new AssemblyName("Microsoft.SqlServer.Types"))); + } + + /// + /// Verifies that pinning normalizes both the version and the public key + /// token, so a server that omits or forges the token cannot cause a + /// partial-name bind that an unsigned same-named assembly could satisfy. + /// + [Fact] + public void PinSqlServerTypesIdentity_PinsVersionAndPublicKeyToken() + { + // A reference as an attacker-controlled server might send it: the right + // simple name, but a bogus version and no strong-name identity. + AssemblyName asmRef = new("Microsoft.SqlServer.Types") + { + Version = new Version(1, 2, 3, 4), + }; + + UdtAssemblyPolicy.PinSqlServerTypesIdentity(asmRef, new Version(14, 0, 0, 0)); + + Assert.Equal(new Version(14, 0, 0, 0), asmRef.Version); + Assert.Equal( + SqlServerTypesPublicKeyToken, + ToHex(asmRef.GetPublicKeyToken())); + } + + /// + /// Verifies that pinning overwrites a public key token supplied by the + /// server rather than trusting it. + /// + [Fact] + public void PinSqlServerTypesIdentity_OverwritesServerSuppliedToken() + { + AssemblyName asmRef = new( + "Microsoft.SqlServer.Types, Version=1.0.0.0, Culture=neutral, PublicKeyToken=0123456789abcdef"); + + UdtAssemblyPolicy.PinSqlServerTypesIdentity(asmRef, new Version(11, 0, 0, 0)); + + Assert.Equal( + SqlServerTypesPublicKeyToken, + ToHex(asmRef.GetPublicKeyToken())); + } + + #endregion + + #region Deny by default + + /// + /// Verifies that an assembly the process has never heard of is denied in + /// both enforcing modes. This is the reporter's scenario: a server-supplied + /// name that resolves to a DLL planted on the probing path. + /// + [Theory] + [InlineData(false)] + [InlineData(true)] + public void IsAllowed_UnknownAssembly_IsDenied(bool strict) + { + using PolicyScope scope = new(strict: strict); + + Assert.False(UdtAssemblyPolicy.IsAllowed(new AssemblyName(UnknownAssemblyName))); + } + + /// + /// Verifies that legacy mode permits everything, restoring the behavior that + /// predates the policy. + /// + [Fact] + public void IsAllowed_LegacyMode_PermitsEverything() + { + using PolicyScope scope = new(legacy: true); + + Assert.True(UdtAssemblyPolicy.IsAllowed(new AssemblyName(UnknownAssemblyName))); + } + + #endregion + + #region Loaded and referenced assemblies + + /// + /// Verifies that an assembly already loaded into the process is permitted in + /// Restricted mode but denied in Strict mode. + /// + [Fact] + public void IsAllowed_LoadedAssembly_DependsOnMode() + { + // This test assembly is, by definition, loaded. + string loadedName = typeof(UdtAssemblyPolicyTest).Assembly.GetName().Name!; + + using (PolicyScope restricted = new()) + { + Assert.True(UdtAssemblyPolicy.IsAllowed(new AssemblyName(loadedName))); + } + + using (PolicyScope strict = new(strict: true)) + { + Assert.False(UdtAssemblyPolicy.IsAllowed(new AssemblyName(loadedName))); + } + } + + /// + /// Verifies that an assembly that is statically referenced by a loaded + /// assembly, but that may not itself be loaded yet, is permitted in + /// Restricted mode. This is what keeps lazily-loaded custom UDT assemblies + /// working. + /// + [Fact] + public void IsAllowed_ReferencedAssembly_IsPermittedWhenRestricted() + { + AssemblyName[] references = + typeof(UdtAssemblyPolicyTest).Assembly.GetReferencedAssemblies(); + Assert.NotEmpty(references); + + using PolicyScope scope = new(); + + foreach (AssemblyName reference in references) + { + Assert.True( + UdtAssemblyPolicy.IsAllowed(new AssemblyName(reference.Name!)), + $"Expected referenced assembly '{reference.Name}' to be permitted."); + } + } + + #endregion + + #region Allow list + + /// + /// Verifies that a simple-name allow list entry permits the assembly in + /// every enforcing mode, and that it does so regardless of the version, + /// culture, and public key token the server supplies. + /// + [Theory] + [InlineData(false)] + [InlineData(true)] + public void IsAllowed_AllowListSimpleName_Permits(bool strict) + { + using PolicyScope scope = new(strict: strict); + PolicyScope.SetAllowList(UnknownAssemblyName); + + Assert.True(UdtAssemblyPolicy.IsAllowed(new AssemblyName(UnknownAssemblyName))); + Assert.True(UdtAssemblyPolicy.IsAllowed(new AssemblyName( + $"{UnknownAssemblyName}, Version=9.9.9.9, Culture=neutral, PublicKeyToken=0123456789abcdef"))); + } + + /// + /// Verifies that allow list matching is case-insensitive on the simple name + /// and tolerates surrounding whitespace and empty entries. + /// + [Fact] + public void IsAllowed_AllowList_IgnoresCaseAndWhitespace() + { + using PolicyScope scope = new(strict: true); + PolicyScope.SetAllowList($" ; {UnknownAssemblyName.ToUpperInvariant()} ; "); + + Assert.True(UdtAssemblyPolicy.IsAllowed(new AssemblyName(UnknownAssemblyName))); + } + + /// + /// Verifies that a fully-qualified allow list entry is matched on every + /// component it specifies, so an assembly that merely borrows the simple + /// name is still denied. + /// + [Fact] + public void IsAllowed_AllowListFullName_MatchesAllSpecifiedComponents() + { + using PolicyScope scope = new(strict: true); + PolicyScope.SetAllowList( + $"{UnknownAssemblyName}, Version=1.0.0.0, Culture=neutral, PublicKeyToken=0123456789abcdef"); + + // Exact match. + Assert.True(UdtAssemblyPolicy.IsAllowed(new AssemblyName( + $"{UnknownAssemblyName}, Version=1.0.0.0, Culture=neutral, PublicKeyToken=0123456789abcdef"))); + + // Wrong version. + Assert.False(UdtAssemblyPolicy.IsAllowed(new AssemblyName( + $"{UnknownAssemblyName}, Version=2.0.0.0, Culture=neutral, PublicKeyToken=0123456789abcdef"))); + + // Wrong public key token. + Assert.False(UdtAssemblyPolicy.IsAllowed(new AssemblyName( + $"{UnknownAssemblyName}, Version=1.0.0.0, Culture=neutral, PublicKeyToken=fedcba9876543210"))); + + // No public key token at all. + Assert.False(UdtAssemblyPolicy.IsAllowed(new AssemblyName( + $"{UnknownAssemblyName}, Version=1.0.0.0, Culture=neutral"))); + } + + /// + /// Verifies that a malformed allow list entry is skipped without throwing + /// and without widening the policy, while valid entries alongside it still + /// take effect. + /// + [Fact] + public void IsAllowed_MalformedAllowListEntry_IsSkipped() + { + using PolicyScope scope = new(strict: true); + PolicyScope.SetAllowList($", , Version=bogus ; {UnknownAssemblyName}"); + + Assert.True(UdtAssemblyPolicy.IsAllowed(new AssemblyName(UnknownAssemblyName))); + Assert.False(UdtAssemblyPolicy.IsAllowed(new AssemblyName("Some.Other.Assembly"))); + } + + /// + /// Verifies that changing the allow list at runtime takes effect, i.e. that + /// the cached parse is keyed on the source string. + /// + [Fact] + public void IsAllowed_AllowListChange_IsObserved() + { + using PolicyScope scope = new(strict: true); + + Assert.False(UdtAssemblyPolicy.IsAllowed(new AssemblyName(UnknownAssemblyName))); + + AppDomain.CurrentDomain.SetData( + UdtAssemblyPolicy.AllowListAppContextDataName, + UnknownAssemblyName); + + Assert.True(UdtAssemblyPolicy.IsAllowed(new AssemblyName(UnknownAssemblyName))); + } + + /// + /// Verifies that an assembly reference with no simple name is denied rather + /// than falling through to a load attempt. + /// + [Fact] + public void IsAllowed_EmptySimpleName_IsDenied() + { + using PolicyScope scope = new(); + + Assert.False(UdtAssemblyPolicy.IsAllowed(new AssemblyName())); + } + + #endregion + + #region Helpers + + private static string? ToHex(byte[]? bytes) + { + if (bytes is null) + { + return null; + } + + char[] chars = new char[bytes.Length * 2]; + for (int i = 0; i < bytes.Length; i++) + { + chars[i * 2] = GetHexDigit(bytes[i] >> 4); + chars[(i * 2) + 1] = GetHexDigit(bytes[i] & 0xF); + } + + return new string(chars); + } + + private static char GetHexDigit(int value) => + (char)(value < 10 ? '0' + value : 'a' + (value - 10)); + + #endregion +} From 5552db724d18f0a0e4e1855df060356e6464fedb Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Fri, 7 Aug 2026 12:37:32 -0700 Subject: [PATCH 02/10] Address self-review gaps in UDT assembly load hardening Fixes six issues found while reviewing the initial hardening commit: - CheckGetExtendedUDTInfo now wraps SqlUdtInfo.TryGetFromType in a catch, so attribute resolution failures do not start throwing at the fThrow: false call sites (GetFieldType and the provider-specific field type) that previously tolerated an unresolvable UDT. - SmiMetaData.Type's assembly-qualified name fallback was a second, ungated Type.GetType sink. It now resolves through the same policy. Every reachable caller currently passes null, so this is defence in depth rather than a live hole. - Pinning the identity of Microsoft.SqlServer.Types is now folded into UdtAssemblyPolicy.IsAllowed, so it is not possible to consult the policy without also pinning. The built-in exemption is granted on the simple name alone, so an unpinned reference would have let an unsigned assembly borrow the name. - The known-assembly-name set is now maintained incrementally by the AssemblyLoad handler instead of being rebuilt on every load, removing a full enumeration of the process's assemblies and their reference lists from the hot path. - Adds regression tests for a type name that carries no assembly part. Type.GetType resolves such a name without ever consulting the assembly resolver, so the SqlUserDefinedTypeAttribute check is the only gate it passes through; the tests lock that in for both fThrow values. - Verified that an exception thrown from inside the assembly resolver propagates out of Type.GetType unwrapped for both throwOnError values, so SQL.UdtAssemblyNotAllowed reaches the caller intact. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60806d12-bdac-413c-bf83-f7c12e3cc12c --- .../Data/SqlClient/Server/SmiMetaData.cs | 17 +- .../Microsoft/Data/SqlClient/SqlConnection.cs | 59 ++++-- .../Data/SqlClient/UdtAssemblyPolicy.cs | 200 ++++++++++-------- .../SqlClient/UdtAssemblyLoadHardeningTest.cs | 46 ++++ .../Data/SqlClient/UdtAssemblyPolicyTest.cs | 77 ++++--- 5 files changed, 269 insertions(+), 130 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SmiMetaData.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SmiMetaData.cs index f2d430f2e2..0bd8d74dbc 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SmiMetaData.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SmiMetaData.cs @@ -9,6 +9,7 @@ using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Globalization; +using System.Reflection; namespace Microsoft.Data.SqlClient.Server { @@ -377,7 +378,21 @@ internal Type Type // Fault-in UDT clr types on access if have assembly-qualified name if (_clrType == null && SqlDbType.Udt == _databaseType && _udtAssemblyQualifiedName != null) { - _clrType = Type.GetType(_udtAssemblyQualifiedName, true); + // The assembly-qualified name can originate from the server, + // and loading an assembly runs its module initializer, so + // the resolution goes through the same policy that + // SqlConnection.ResolveTypeAssembly applies. There is no + // connection context here, so no type system version is + // available to pin the built-in SQL CLR types assembly to; + // its public key token is still pinned. + _clrType = Type.GetType( + typeName: _udtAssemblyQualifiedName, + assemblyResolver: static asmRef => + UdtAssemblyPolicy.IsAllowed(asmRef, typeSystemAssemblyVersion: null) + ? Assembly.Load(asmRef) + : throw SQL.UdtAssemblyNotAllowed(asmRef.Name), + typeResolver: null, + throwOnError: true); } return _clrType; } diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs index a30e1cc321..b9af101ef5 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs @@ -2965,24 +2965,21 @@ private Assembly ResolveTypeAssembly(AssemblyName asmRef, bool throwOnError) { Debug.Assert(TypeSystemAssemblyVersion != null, "TypeSystemAssembly should be set !"); - if (UdtAssemblyPolicy.IsSqlServerTypesAssembly(asmRef)) + if (UdtAssemblyPolicy.IsSqlServerTypesAssembly(asmRef) && + asmRef.Version != TypeSystemAssemblyVersion && + SqlClientEventSource.Log.IsTraceEnabled()) { - if (asmRef.Version != TypeSystemAssemblyVersion && SqlClientEventSource.Log.IsTraceEnabled()) - { - SqlClientEventSource.Log.TryTraceEvent("SqlConnection.ResolveTypeAssembly | SQL CLR type version change: Server sent {0}, client will instantiate {1}", asmRef.Version, TypeSystemAssemblyVersion); - } - - // Pin both the version and the public key token so that the - // built-in exemption cannot be satisfied by a same-named - // assembly that happens to sit on the probing path. - UdtAssemblyPolicy.PinSqlServerTypesIdentity(asmRef, TypeSystemAssemblyVersion); + SqlClientEventSource.Log.TryTraceEvent("SqlConnection.ResolveTypeAssembly | SQL CLR type version change: Server sent {0}, client will instantiate {1}", asmRef.Version, TypeSystemAssemblyVersion); } // The assembly name arrives from the server, and loading an assembly // runs its module initializer, so the driver must decide whether it // is willing to load this assembly before it hands the name to the - // loader. - if (!UdtAssemblyPolicy.IsAllowed(asmRef)) + // loader. This call also pins the identity (version and public key + // token) of the built-in SQL CLR types assembly, so that the + // built-in exemption cannot be satisfied by a same-named assembly + // that happens to sit on the probing path. + if (!UdtAssemblyPolicy.IsAllowed(asmRef, TypeSystemAssemblyVersion)) { SqlClientEventSource.Log.TryTraceEvent("SqlConnection.ResolveTypeAssembly | ERR | UDT assembly '{0}' was not loaded because it is not permitted by the '{1}' UDT assembly load policy.", asmRef.Name, UdtAssemblyPolicy.Mode); @@ -3026,17 +3023,41 @@ internal void CheckGetExtendedUDTInfo(SqlMetaDataPriv metaData, bool fThrow) // driver can reject a type that the server named but that is not // actually a user-defined type, and it must happen before // GetUdtValue invokes anything on it. - if (metaData.udt.Type != null && - !UdtAssemblyPolicy.LegacyBehaviorEnabled && - SqlUdtInfo.TryGetFromType(metaData.udt.Type) == null) + // + // This check also backstops the assembly policy: a type name + // that carries no assembly part is resolved without ever + // consulting the assembly resolver, so this is the only gate a + // name such as "System.String" passes through. + if (metaData.udt.Type != null && !UdtAssemblyPolicy.LegacyBehaviorEnabled) { - SqlClientEventSource.Log.TryTraceEvent("SqlConnection.CheckGetExtendedUDTInfo | ERR | Type '{0}' is not annotated with SqlUserDefinedTypeAttribute and will not be used.", metaData.udt.AssemblyQualifiedName); + bool isUserDefinedType; - metaData.udt.Type = null; + try + { + isUserDefinedType = SqlUdtInfo.TryGetFromType(metaData.udt.Type) != null; + } + catch (Exception e) when (ADP.IsCatchableExceptionType(e)) + { + // Reading custom attributes can fail if the attribute or + // one of its arguments lives in an assembly that cannot + // be loaded. Treat that as "not a user-defined type" + // rather than letting it escape, so that callers that + // pass fThrow: false keep tolerating an unusable type. + SqlClientEventSource.Log.TryTraceEvent("SqlConnection.CheckGetExtendedUDTInfo | ERR | Unable to read the attributes of type '{0}'.", metaData.udt.AssemblyQualifiedName); + + isUserDefinedType = false; + } - if (fThrow) + if (!isUserDefinedType) { - throw SQL.UdtTypeNotUserDefined(metaData.udt.AssemblyQualifiedName); + SqlClientEventSource.Log.TryTraceEvent("SqlConnection.CheckGetExtendedUDTInfo | ERR | Type '{0}' is not annotated with SqlUserDefinedTypeAttribute and will not be used.", metaData.udt.AssemblyQualifiedName); + + metaData.udt.Type = null; + + if (fThrow) + { + throw SQL.UdtTypeNotUserDefined(metaData.udt.AssemblyQualifiedName); + } } } diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/UdtAssemblyPolicy.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/UdtAssemblyPolicy.cs index 4c1226ab01..9ee2930456 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/UdtAssemblyPolicy.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/UdtAssemblyPolicy.cs @@ -5,7 +5,6 @@ using System; using System.Collections.Generic; using System.Reflection; -using System.Threading; using Microsoft.Data.Common; using Microsoft.Data.SqlClient.Internal; @@ -99,14 +98,6 @@ internal static class UdtAssemblyPolicy /// private static readonly object s_lock = new(); - /// - /// Incremented every time an assembly is loaded into the process. Used to - /// invalidate . Written with - /// from the - /// callback and only read while is held. - /// - private static int s_assemblyLoadVersion; - /// /// Set to true once the handler has /// been attached. The handler is attached lazily so that applications that @@ -118,15 +109,15 @@ internal static class UdtAssemblyPolicy /// The simple names of every assembly that is loaded into the process, plus /// the simple names of every assembly they statically reference. Null when /// it has not been built yet. + /// + /// Once built, the set is maintained incrementally by the + /// handler rather than rebuilt, so an + /// application that loads assemblies while reading UDTs does not repeatedly + /// pay for a full enumeration of the process's assemblies and their + /// reference lists. /// private static HashSet? s_knownAssemblyNames; - /// - /// The value of at the time - /// was built. - /// - private static int s_knownAssemblyNamesVersion = -1; - /// /// The raw allow list string that was parsed /// from, used to detect that the application has changed it. @@ -180,34 +171,28 @@ internal static bool IsSqlServerTypesAssembly(AssemblyName asmRef) => string.Equals(asmRef.Name, SqlServerTypesAssemblyName, StringComparison.OrdinalIgnoreCase); /// - /// Pins the identity of the built-in SQL Server CLR types assembly. - /// - /// The version is normalized to the type system version negotiated for the - /// connection, which is long-standing behavior: the server advertises the - /// version it holds, and the client instantiates the version it has. + /// Decides whether the driver may load the assembly named by + /// , pinning the identity of the built-in SQL + /// Server CLR types assembly as a side effect when that is what it names. /// - /// The public key token is normalized to the token that Microsoft signs the - /// assembly with. Without this, a server that omits the token (or supplies - /// a different one) would cause a partial-name bind that an unsigned - /// same-named assembly on the probing path could satisfy. + /// Pinning and the decision are deliberately performed by a single call so + /// that it is not possible to consult the policy without also pinning: the + /// built-in exemption is granted on the simple name alone, so an unpinned + /// reference would let an unsigned assembly that merely borrows the name + /// satisfy it. /// - /// The assembly reference to normalize, in place. + /// + /// The server-supplied assembly reference. It is normalized in place when + /// it names the built-in SQL Server CLR types assembly. + /// /// - /// The type system assembly version negotiated for the connection. + /// The type system assembly version negotiated for the connection, used to + /// pin the version of the built-in SQL Server CLR types assembly. Null + /// when no connection context is available, in which case only the public + /// key token is pinned and the loader picks the version. /// - internal static void PinSqlServerTypesIdentity(AssemblyName asmRef, Version typeSystemAssemblyVersion) - { - asmRef.Version = typeSystemAssemblyVersion; - asmRef.SetPublicKeyToken((byte[])s_sqlServerTypesPublicKeyToken.Clone()); - } - - /// - /// Determines whether the driver is permitted to load the assembly named by - /// . - /// - /// The server-supplied assembly reference. /// True when the assembly may be loaded. - internal static bool IsAllowed(AssemblyName asmRef) + internal static bool IsAllowed(AssemblyName asmRef, Version? typeSystemAssemblyVersion) { UdtAssemblyLoadMode mode = Mode; @@ -222,11 +207,12 @@ internal static bool IsAllowed(AssemblyName asmRef) return false; } - // The built-in types assembly is always permitted. Its identity has - // already been pinned by the caller, so this cannot be satisfied by an - // arbitrary assembly that merely borrows the name. + // The built-in types assembly is always permitted, but only once its + // identity has been pinned, so the exemption cannot be satisfied by an + // arbitrary assembly that borrows the name. if (IsSqlServerTypesAssembly(asmRef)) { + PinSqlServerTypesIdentity(asmRef, typeSystemAssemblyVersion); return true; } @@ -243,6 +229,33 @@ internal static bool IsAllowed(AssemblyName asmRef) return false; } + /// + /// Pins the identity of the built-in SQL Server CLR types assembly. + /// + /// The version is normalized to the type system version negotiated for the + /// connection, which is long-standing behavior: the server advertises the + /// version it holds, and the client instantiates the version it has. + /// + /// The public key token is normalized to the token that Microsoft signs the + /// assembly with. Without this, a server that omits the token (or supplies + /// a different one) would cause a partial-name bind that an unsigned + /// same-named assembly on the probing path could satisfy. + /// + /// The assembly reference to normalize, in place. + /// + /// The type system assembly version negotiated for the connection, or null + /// to leave the version unconstrained. + /// + private static void PinSqlServerTypesIdentity(AssemblyName asmRef, Version? typeSystemAssemblyVersion) + { + if (typeSystemAssemblyVersion is not null) + { + asmRef.Version = typeSystemAssemblyVersion; + } + + asmRef.SetPublicKeyToken((byte[])s_sqlServerTypesPublicKeyToken.Clone()); + } + /// /// Discards all cached state. Intended for use by tests, which need to /// observe the effect of changing the allow list or the policy switches. @@ -254,7 +267,6 @@ internal static void ResetCache() s_allowList = null; s_allowListSource = null; s_knownAssemblyNames = null; - s_knownAssemblyNamesVersion = -1; } } @@ -389,8 +401,9 @@ private static bool IsKnownToProcess(string simpleName) => /// /// Returns the set of assembly simple names that are loaded into the - /// process or referenced by an assembly that is, rebuilding it only if an - /// assembly has been loaded since it was last built. + /// process or referenced by an assembly that is, building it on first use + /// and thereafter relying on the + /// handler to keep it current. /// private static HashSet GetKnownAssemblyNames() { @@ -398,9 +411,7 @@ private static HashSet GetKnownAssemblyNames() lock (s_lock) { - int version = s_assemblyLoadVersion; - - if (s_knownAssemblyNames is not null && s_knownAssemblyNamesVersion == version) + if (s_knownAssemblyNames is not null) { return s_knownAssemblyNames; } @@ -409,51 +420,60 @@ private static HashSet GetKnownAssemblyNames() foreach (Assembly assembly in AppDomain.CurrentDomain.GetAssemblies()) { - if (assembly.IsDynamic) - { - // A dynamic assembly has no manifest to read references - // from, and it cannot be a target of Assembly.Load by name - // anyway. - continue; - } - - string? name = assembly.GetName().Name; - if (!string.IsNullOrEmpty(name)) - { - names.Add(name!); - } - - try - { - foreach (AssemblyName reference in assembly.GetReferencedAssemblies()) - { - if (!string.IsNullOrEmpty(reference.Name)) - { - names.Add(reference.Name!); - } - } - } - catch (Exception e) when (ADP.IsCatchableExceptionType(e)) - { - // Reading the reference list can fail for assemblies loaded - // from a byte array or produced by a trimmer. Losing one - // assembly's references only makes the policy stricter. - SqlClientEventSource.Log.TryTraceEvent( - "UdtAssemblyPolicy.GetKnownAssemblyNames | INFO | Unable to read references of '{0}'.", - name); - } + AddAssemblyNames(names, assembly); } s_knownAssemblyNames = names; - s_knownAssemblyNamesVersion = version; return names; } } /// - /// Attaches the assembly load handler that invalidates the cached - /// known-assembly-name set, if it has not been attached already. + /// Adds the simple name of and the simple names + /// of every assembly it statically references to . + /// + private static void AddAssemblyNames(HashSet names, Assembly assembly) + { + if (assembly.IsDynamic) + { + // A dynamic assembly has no manifest to read references from, and + // it cannot be a target of Assembly.Load by name anyway. + return; + } + + string? name = null; + + try + { + name = assembly.GetName().Name; + if (!string.IsNullOrEmpty(name)) + { + names.Add(name!); + } + + foreach (AssemblyName reference in assembly.GetReferencedAssemblies()) + { + if (!string.IsNullOrEmpty(reference.Name)) + { + names.Add(reference.Name!); + } + } + } + catch (Exception e) when (ADP.IsCatchableExceptionType(e)) + { + // Reading the name or the reference list can fail for assemblies + // loaded from a byte array or produced by a trimmer. Losing one + // assembly's references only makes the policy stricter. + SqlClientEventSource.Log.TryTraceEvent( + "UdtAssemblyPolicy.AddAssemblyNames | INFO | Unable to read references of '{0}'.", + name); + } + } + + /// + /// Attaches the assembly load handler that keeps the cached + /// known-assembly-name set current, if it has not been attached already. /// private static void EnsureAssemblyLoadHandlerAttached() { @@ -464,8 +484,18 @@ private static void EnsureAssemblyLoadHandlerAttached() return; } - AppDomain.CurrentDomain.AssemblyLoad += static (_, _) => - Interlocked.Increment(ref s_assemblyLoadVersion); + AppDomain.CurrentDomain.AssemblyLoad += static (_, args) => + { + lock (s_lock) + { + // Nothing to update if the set has not been built yet; it + // will pick the assembly up when it is. + if (s_knownAssemblyNames is not null) + { + AddAssemblyNames(s_knownAssemblyNames, args.LoadedAssembly); + } + } + }; s_assemblyLoadHandlerAttached = true; } diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyLoadHardeningTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyLoadHardeningTest.cs index 90a58646ac..8e4e27102b 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyLoadHardeningTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyLoadHardeningTest.cs @@ -180,6 +180,52 @@ public void CheckGetExtendedUDTInfo_LegacyMode_SkipsUdtAttributeCheck() Assert.Equal(typeof(NotAUserDefinedType), metaData.udt.Type); } + /// + /// Verifies that a type name carrying no assembly part is still rejected. + /// + /// + /// Type.GetType resolves a bare type name against the core library without + /// ever consulting the assembly resolver, so the assembly load policy is + /// structurally bypassed for such a name. The SqlUserDefinedTypeAttribute + /// check is the only gate that stands in its way, and this test locks that + /// in: a server that sends "System.String" must not end up with the driver + /// invoking members on System.String. + /// + [Theory] + [InlineData("System.String")] + [InlineData("System.Diagnostics.Process")] + public void CheckGetExtendedUDTInfo_TypeNameWithoutAssembly_IsRejected(string typeName) + { + using PolicyScope scope = new(); + + SqlConnection connection = new(ConnectionString); + SqlMetaDataPriv metaData = CreateUdtMetaData(typeName); + + Exception exception = Record.Exception( + () => connection.CheckGetExtendedUDTInfo(metaData, fThrow: true)); + + Assert.NotNull(exception); + Assert.Null(metaData.udt.Type); + } + + /// + /// Verifies that a bare type name is rejected without throwing at the call + /// sites that ask not to throw, which is how GetFieldType probes UDT + /// metadata. + /// + [Fact] + public void CheckGetExtendedUDTInfo_TypeNameWithoutAssembly_DoesNotThrowWhenNotRequested() + { + using PolicyScope scope = new(); + + SqlConnection connection = new(ConnectionString); + SqlMetaDataPriv metaData = CreateUdtMetaData("System.String"); + + connection.CheckGetExtendedUDTInfo(metaData, fThrow: false); + + Assert.Null(metaData.udt.Type); + } + #endregion #region Helpers diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs index afa6e77fc0..e5628610ce 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs @@ -133,17 +133,22 @@ public void IsAllowed_SqlServerTypes_IsPermitted(bool strict) Assert.True(UdtAssemblyPolicy.IsSqlServerTypesAssembly( new AssemblyName("microsoft.sqlserver.types"))); Assert.True(UdtAssemblyPolicy.IsAllowed( - new AssemblyName("Microsoft.SqlServer.Types"))); + new AssemblyName("Microsoft.SqlServer.Types"), null)); } /// - /// Verifies that pinning normalizes both the version and the public key - /// token, so a server that omits or forges the token cannot cause a - /// partial-name bind that an unsigned same-named assembly could satisfy. + /// Verifies that permitting the built-in types assembly also normalizes both + /// its version and its public key token, so a server that omits or forges + /// the token cannot cause a partial-name bind that an unsigned same-named + /// assembly could satisfy. The two must happen together: the exemption is + /// granted on the simple name alone, so an unpinned reference would let an + /// arbitrary assembly borrow the name. /// [Fact] - public void PinSqlServerTypesIdentity_PinsVersionAndPublicKeyToken() + public void IsAllowed_SqlServerTypes_PinsVersionAndPublicKeyToken() { + using PolicyScope scope = new(strict: false); + // A reference as an attacker-controlled server might send it: the right // simple name, but a bogus version and no strong-name identity. AssemblyName asmRef = new("Microsoft.SqlServer.Types") @@ -151,7 +156,7 @@ public void PinSqlServerTypesIdentity_PinsVersionAndPublicKeyToken() Version = new Version(1, 2, 3, 4), }; - UdtAssemblyPolicy.PinSqlServerTypesIdentity(asmRef, new Version(14, 0, 0, 0)); + Assert.True(UdtAssemblyPolicy.IsAllowed(asmRef, new Version(14, 0, 0, 0))); Assert.Equal(new Version(14, 0, 0, 0), asmRef.Version); Assert.Equal( @@ -164,13 +169,35 @@ public void PinSqlServerTypesIdentity_PinsVersionAndPublicKeyToken() /// server rather than trusting it. /// [Fact] - public void PinSqlServerTypesIdentity_OverwritesServerSuppliedToken() + public void IsAllowed_SqlServerTypes_OverwritesServerSuppliedToken() { + using PolicyScope scope = new(strict: false); + AssemblyName asmRef = new( "Microsoft.SqlServer.Types, Version=1.0.0.0, Culture=neutral, PublicKeyToken=0123456789abcdef"); - UdtAssemblyPolicy.PinSqlServerTypesIdentity(asmRef, new Version(11, 0, 0, 0)); + Assert.True(UdtAssemblyPolicy.IsAllowed(asmRef, new Version(11, 0, 0, 0))); + + Assert.Equal( + SqlServerTypesPublicKeyToken, + ToHex(asmRef.GetPublicKeyToken())); + } + + /// + /// Verifies that the public key token is still pinned when no type system + /// version is available to pin the version to, which is the case for callers + /// that have no connection context. + /// + [Fact] + public void IsAllowed_SqlServerTypes_WithoutVersion_StillPinsToken() + { + using PolicyScope scope = new(strict: false); + + AssemblyName asmRef = new("Microsoft.SqlServer.Types, Version=1.0.0.0"); + + Assert.True(UdtAssemblyPolicy.IsAllowed(asmRef, null)); + Assert.Equal(new Version(1, 0, 0, 0), asmRef.Version); Assert.Equal( SqlServerTypesPublicKeyToken, ToHex(asmRef.GetPublicKeyToken())); @@ -192,7 +219,7 @@ public void IsAllowed_UnknownAssembly_IsDenied(bool strict) { using PolicyScope scope = new(strict: strict); - Assert.False(UdtAssemblyPolicy.IsAllowed(new AssemblyName(UnknownAssemblyName))); + Assert.False(UdtAssemblyPolicy.IsAllowed(new AssemblyName(UnknownAssemblyName), null)); } /// @@ -204,7 +231,7 @@ public void IsAllowed_LegacyMode_PermitsEverything() { using PolicyScope scope = new(legacy: true); - Assert.True(UdtAssemblyPolicy.IsAllowed(new AssemblyName(UnknownAssemblyName))); + Assert.True(UdtAssemblyPolicy.IsAllowed(new AssemblyName(UnknownAssemblyName), null)); } #endregion @@ -223,12 +250,12 @@ public void IsAllowed_LoadedAssembly_DependsOnMode() using (PolicyScope restricted = new()) { - Assert.True(UdtAssemblyPolicy.IsAllowed(new AssemblyName(loadedName))); + Assert.True(UdtAssemblyPolicy.IsAllowed(new AssemblyName(loadedName), null)); } using (PolicyScope strict = new(strict: true)) { - Assert.False(UdtAssemblyPolicy.IsAllowed(new AssemblyName(loadedName))); + Assert.False(UdtAssemblyPolicy.IsAllowed(new AssemblyName(loadedName), null)); } } @@ -250,7 +277,7 @@ public void IsAllowed_ReferencedAssembly_IsPermittedWhenRestricted() foreach (AssemblyName reference in references) { Assert.True( - UdtAssemblyPolicy.IsAllowed(new AssemblyName(reference.Name!)), + UdtAssemblyPolicy.IsAllowed(new AssemblyName(reference.Name!), null), $"Expected referenced assembly '{reference.Name}' to be permitted."); } } @@ -272,9 +299,9 @@ public void IsAllowed_AllowListSimpleName_Permits(bool strict) using PolicyScope scope = new(strict: strict); PolicyScope.SetAllowList(UnknownAssemblyName); - Assert.True(UdtAssemblyPolicy.IsAllowed(new AssemblyName(UnknownAssemblyName))); + Assert.True(UdtAssemblyPolicy.IsAllowed(new AssemblyName(UnknownAssemblyName), null)); Assert.True(UdtAssemblyPolicy.IsAllowed(new AssemblyName( - $"{UnknownAssemblyName}, Version=9.9.9.9, Culture=neutral, PublicKeyToken=0123456789abcdef"))); + $"{UnknownAssemblyName}, Version=9.9.9.9, Culture=neutral, PublicKeyToken=0123456789abcdef"), null)); } /// @@ -287,7 +314,7 @@ public void IsAllowed_AllowList_IgnoresCaseAndWhitespace() using PolicyScope scope = new(strict: true); PolicyScope.SetAllowList($" ; {UnknownAssemblyName.ToUpperInvariant()} ; "); - Assert.True(UdtAssemblyPolicy.IsAllowed(new AssemblyName(UnknownAssemblyName))); + Assert.True(UdtAssemblyPolicy.IsAllowed(new AssemblyName(UnknownAssemblyName), null)); } /// @@ -304,19 +331,19 @@ public void IsAllowed_AllowListFullName_MatchesAllSpecifiedComponents() // Exact match. Assert.True(UdtAssemblyPolicy.IsAllowed(new AssemblyName( - $"{UnknownAssemblyName}, Version=1.0.0.0, Culture=neutral, PublicKeyToken=0123456789abcdef"))); + $"{UnknownAssemblyName}, Version=1.0.0.0, Culture=neutral, PublicKeyToken=0123456789abcdef"), null)); // Wrong version. Assert.False(UdtAssemblyPolicy.IsAllowed(new AssemblyName( - $"{UnknownAssemblyName}, Version=2.0.0.0, Culture=neutral, PublicKeyToken=0123456789abcdef"))); + $"{UnknownAssemblyName}, Version=2.0.0.0, Culture=neutral, PublicKeyToken=0123456789abcdef"), null)); // Wrong public key token. Assert.False(UdtAssemblyPolicy.IsAllowed(new AssemblyName( - $"{UnknownAssemblyName}, Version=1.0.0.0, Culture=neutral, PublicKeyToken=fedcba9876543210"))); + $"{UnknownAssemblyName}, Version=1.0.0.0, Culture=neutral, PublicKeyToken=fedcba9876543210"), null)); // No public key token at all. Assert.False(UdtAssemblyPolicy.IsAllowed(new AssemblyName( - $"{UnknownAssemblyName}, Version=1.0.0.0, Culture=neutral"))); + $"{UnknownAssemblyName}, Version=1.0.0.0, Culture=neutral"), null)); } /// @@ -330,8 +357,8 @@ public void IsAllowed_MalformedAllowListEntry_IsSkipped() using PolicyScope scope = new(strict: true); PolicyScope.SetAllowList($", , Version=bogus ; {UnknownAssemblyName}"); - Assert.True(UdtAssemblyPolicy.IsAllowed(new AssemblyName(UnknownAssemblyName))); - Assert.False(UdtAssemblyPolicy.IsAllowed(new AssemblyName("Some.Other.Assembly"))); + Assert.True(UdtAssemblyPolicy.IsAllowed(new AssemblyName(UnknownAssemblyName), null)); + Assert.False(UdtAssemblyPolicy.IsAllowed(new AssemblyName("Some.Other.Assembly"), null)); } /// @@ -343,13 +370,13 @@ public void IsAllowed_AllowListChange_IsObserved() { using PolicyScope scope = new(strict: true); - Assert.False(UdtAssemblyPolicy.IsAllowed(new AssemblyName(UnknownAssemblyName))); + Assert.False(UdtAssemblyPolicy.IsAllowed(new AssemblyName(UnknownAssemblyName), null)); AppDomain.CurrentDomain.SetData( UdtAssemblyPolicy.AllowListAppContextDataName, UnknownAssemblyName); - Assert.True(UdtAssemblyPolicy.IsAllowed(new AssemblyName(UnknownAssemblyName))); + Assert.True(UdtAssemblyPolicy.IsAllowed(new AssemblyName(UnknownAssemblyName), null)); } /// @@ -361,7 +388,7 @@ public void IsAllowed_EmptySimpleName_IsDenied() { using PolicyScope scope = new(); - Assert.False(UdtAssemblyPolicy.IsAllowed(new AssemblyName())); + Assert.False(UdtAssemblyPolicy.IsAllowed(new AssemblyName(), null)); } #endregion From c191b748334bdf7d2cb0030cc6c70c583a615b30 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Fri, 7 Aug 2026 13:13:36 -0700 Subject: [PATCH 03/10] Serialize the UDT policy tests with the other AppContext switch tests The merge of origin/main brought in PR #4495, which introduced AppContextSwitchTestCollection to serialize tests that mutate process-wide cached AppContext switch values. Both UDT test classes do exactly that, so they join the collection; without it they can race against other collections and observe each other's temporary settings. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60806d12-bdac-413c-bf83-f7c12e3cc12c --- .../Microsoft/Data/SqlClient/UdtAssemblyLoadHardeningTest.cs | 1 + .../UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs | 1 + 2 files changed, 2 insertions(+) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyLoadHardeningTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyLoadHardeningTest.cs index 8e4e27102b..db2fa20b4a 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyLoadHardeningTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyLoadHardeningTest.cs @@ -24,6 +24,7 @@ namespace Microsoft.Data.SqlClient.UnitTests; /// at all, which runs the type's static constructor. These tests assert that /// neither happens. /// +[Collection(AppContextSwitchTestCollection.Name)] public class UdtAssemblyLoadHardeningTest { /// diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs index e5628610ce..64bf505c08 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs @@ -14,6 +14,7 @@ namespace Microsoft.Data.SqlClient.UnitTests; /// policy that governs which assemblies the driver is willing to load while /// resolving a server-supplied UDT assembly-qualified name. /// +[Collection(AppContextSwitchTestCollection.Name)] public class UdtAssemblyPolicyTest { /// From a3f184680a7bf7c1057a134a9b0e36e16c520b27 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Mon, 24 Aug 2026 11:01:51 -0700 Subject: [PATCH 04/10] Collapse the UDT assembly load policy to a single enforcing mode Replaces the Restricted/Strict/Legacy taxonomy with one enforcing behavior plus the legacy escape hatch, and removes the UseStrictUdtAssemblyLoad switch. The three-mode design was over-built for what this layer does. Measured on CoreCLR, neither Assembly.Load, nor resolving a type from the loaded assembly, nor reading that type's custom attributes executes any code from the target; a module initializer runs on first real member access, which is what GetUdtValue would perform. The SqlUserDefinedTypeAttribute check in CheckGetExtendedUDTInfo is therefore the gate that actually prevents foreign code execution, and the assembly policy in front of it is a resource-load gate that does not warrant two tiers. The single enforcing mode permits the pinned Microsoft.SqlServer.Types assembly, the application's allow list, and assemblies already loaded into the process. The static reference closure is no longer permitted, because loading a referenced-but-unloaded assembly is a genuinely new load, which is the thing this policy exists to keep under the application's control. Applications with custom UDTs whose assembly is not yet loaded must now name it on the allow list. Also fixes an identity-binding hole in the already-loaded tier. It matched on simple name and then handed the server's full reference, including version and public key token, to the loader, so a server could name a loaded simple name with a different identity and still trigger a new load. The policy now returns the loaded instance itself, and callers use it rather than re-binding server-controlled identity. Note that ECMA-335 permits a runtime to run a module initializer at load time, and only CoreCLR was measured here, so the assembly policy is retained as defence in depth pending verification on .NET Framework. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60806d12-bdac-413c-bf83-f7c12e3cc12c --- .github/instructions/features.instructions.md | 33 ++- .../Data/SqlClient/LocalAppContextSwitches.cs | 46 +-- .../Data/SqlClient/Server/SmiMetaData.cs | 7 +- .../Microsoft/Data/SqlClient/SqlConnection.cs | 28 +- .../Data/SqlClient/UdtAssemblyPolicy.cs | 268 ++++++++---------- .../Common/LocalAppContextSwitchesHelper.cs | 15 - .../SqlClient/LocalAppContextSwitchesTest.cs | 2 - .../SqlClient/UdtAssemblyLoadHardeningTest.cs | 3 +- .../Data/SqlClient/UdtAssemblyPolicyTest.cs | 205 ++++++++------ 9 files changed, 292 insertions(+), 315 deletions(-) diff --git a/.github/instructions/features.instructions.md b/.github/instructions/features.instructions.md index e82d43834e..1da3e117d8 100644 --- a/.github/instructions/features.instructions.md +++ b/.github/instructions/features.instructions.md @@ -258,20 +258,29 @@ AppContext switches allow runtime behavior changes without modifying connection | `Switch.Microsoft.Data.SqlClient.UseManagedNetworkingOnWindows` | `false` | Forces managed SNI on Windows (instead of native SNI) | | `Switch.Microsoft.Data.SqlClient.UseOneSecFloorInTimeoutCalculationDuringLogin` | `false` | Sets 1-second minimum in login timeout calculations | | `Switch.Microsoft.Data.SqlClient.UseLegacyUdtAssemblyLoad` | `false` | Restores the pre-policy behavior of loading any assembly named by a server-supplied UDT assembly-qualified name, and of skipping the `[SqlUserDefinedType]` check | -| `Switch.Microsoft.Data.SqlClient.UseStrictUdtAssemblyLoad` | `false` | Restricts UDT assembly loads to `Microsoft.SqlServer.Types` and the allow list only, excluding assemblies that merely happen to be present in the process | ### UDT Assembly Load Policy A server-supplied UDT assembly-qualified name reaches `Assembly.Load`, so the driver applies a deny-by-default policy before handing the name to the loader. +There is a single enforcing behavior, which permits: -| Mode | Selected by | Permits | -|------|-------------|---------| -| `Restricted` (default) | neither switch | `Microsoft.SqlServer.Types` (identity pinned), the allow list, assemblies already loaded into the process, and assemblies statically referenced by them | -| `Strict` | `UseStrictUdtAssemblyLoad` | `Microsoft.SqlServer.Types` (identity pinned) and the allow list only | -| `Legacy` | `UseLegacyUdtAssemblyLoad` (wins over `Strict`) | everything, i.e. the pre-policy behavior | +| Permitted | Notes | +|-----------|-------| +| `Microsoft.SqlServer.Types` | Identity pinned: the version is normalized to the connection's negotiated type system version, and the public key token to the one Microsoft signs with | +| Assemblies on the allow list | The application explicitly naming what it is willing to have loaded | +| Assemblies already loaded into the process | Resolved to the instance the process already holds; the server-supplied version and public key token are discarded | -Applications that use custom UDTs whose assemblies are loaded on demand can name +Everything else is refused. In particular, an assembly that is only *statically +referenced* by a loaded assembly is **not** permitted, because loading it is a +genuinely new load — precisely what this policy keeps under the application's +control rather than the server's. + +Setting `UseLegacyUdtAssemblyLoad` disables the policy entirely and restores the +pre-policy behavior. It is a temporary compatibility escape hatch, not a +supported configuration. + +Applications that use custom UDTs whose assemblies are loaded on demand must name them explicitly through the `Microsoft.Data.SqlClient.UdtAssemblyAllowList` AppContext data element, a semicolon-separated list of assembly names: @@ -285,9 +294,13 @@ Each entry is matched only on the components it specifies, so a simple name permits any version, culture, and public key token, while a fully-qualified name must match exactly. -Independently of the mode, a resolved type that is not annotated with -`SqlUserDefinedTypeAttribute` is rejected before any of its code runs (except in -`Legacy` mode). +Independently of the assembly policy, a resolved type that is not annotated with +`SqlUserDefinedTypeAttribute` is rejected before any of its code runs (except +under `UseLegacyUdtAssemblyLoad`). This is the gate that actually prevents +foreign code execution: on CoreCLR, neither `Assembly.Load`, nor resolving a type +from the assembly, nor reading that type's custom attributes runs anything from +it — a module initializer or static constructor runs on first real member access, +which is what `GetUdtValue` would otherwise perform. ### Usage Example ```csharp diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs index 809e03a19f..37fb25f003 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs @@ -147,15 +147,6 @@ internal static class LocalAppContextSwitches private const string UseLegacyUdtAssemblyLoadString = "Switch.Microsoft.Data.SqlClient.UseLegacyUdtAssemblyLoad"; - /// - /// The name of the app context switch that controls whether the UDT - /// assembly load policy refuses to load assemblies that are merely present - /// in the process, permitting only the built-in SQL Server CLR types - /// assembly and assemblies named on the application's allow list. - /// - private const string UseStrictUdtAssemblyLoadString = - "Switch.Microsoft.Data.SqlClient.UseStrictUdtAssemblyLoad"; - #if NET /// /// The name of the app context switch that controls whether to use the @@ -280,11 +271,6 @@ private enum SwitchValue : byte /// private static SwitchValue s_useLegacyUdtAssemblyLoad = SwitchValue.None; - /// - /// The cached value of the UseStrictUdtAssemblyLoad switch. - /// - private static SwitchValue s_useStrictUdtAssemblyLoad = SwitchValue.None; - #if NET /// /// The cached value of the UseManagedNetworking switch. @@ -641,14 +627,14 @@ public static bool UseCompatibilityAsyncBehaviour /// /// When set to true, the driver loads any assembly named by a - /// server-supplied UDT assembly-qualified name, which is the behavior that - /// predates the UDT assembly load policy. + /// server-supplied UDT assembly-qualified name, and skips the check that + /// the resolved type is annotated with SqlUserDefinedTypeAttribute. This is + /// the behavior that predates the UDT assembly load policy. /// - /// This switch takes precedence over - /// . Enabling it allows a server, or - /// an attacker on the network path of a connection that has opted out of - /// certificate validation, to choose which assemblies the client process - /// loads, so it should only be used as a temporary compatibility measure. + /// Enabling it allows a server, or an attacker on the network path of a + /// connection that has opted out of certificate validation, to choose which + /// assemblies the client process loads, so it should only be used as a + /// temporary compatibility measure. /// /// The default value of this switch is false. /// @@ -658,24 +644,6 @@ public static bool UseCompatibilityAsyncBehaviour defaultValue: false, ref s_useLegacyUdtAssemblyLoad); - /// - /// When set to true, the UDT assembly load policy permits only the built-in - /// Microsoft.SqlServer.Types assembly and assemblies named on the - /// application's allow list (the Microsoft.Data.SqlClient.UdtAssemblyAllowList - /// AppContext data element). - /// - /// When false (the default), assemblies that are already loaded into the - /// process, or that are statically referenced by an assembly that is, are - /// also permitted. - /// - /// The default value of this switch is false. - /// - public static bool UseStrictUdtAssemblyLoad => - AcquireAndReturn( - UseStrictUdtAssemblyLoadString, - defaultValue: false, - ref s_useStrictUdtAssemblyLoad); - #if NET /// /// When set to true, .NET on Windows will use the managed SNI diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SmiMetaData.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SmiMetaData.cs index 0bd8d74dbc..a6b828d295 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SmiMetaData.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SmiMetaData.cs @@ -379,8 +379,7 @@ internal Type Type if (_clrType == null && SqlDbType.Udt == _databaseType && _udtAssemblyQualifiedName != null) { // The assembly-qualified name can originate from the server, - // and loading an assembly runs its module initializer, so - // the resolution goes through the same policy that + // so the resolution goes through the same policy that // SqlConnection.ResolveTypeAssembly applies. There is no // connection context here, so no type system version is // available to pin the built-in SQL CLR types assembly to; @@ -388,8 +387,8 @@ internal Type Type _clrType = Type.GetType( typeName: _udtAssemblyQualifiedName, assemblyResolver: static asmRef => - UdtAssemblyPolicy.IsAllowed(asmRef, typeSystemAssemblyVersion: null) - ? Assembly.Load(asmRef) + UdtAssemblyPolicy.TryResolve(asmRef, typeSystemAssemblyVersion: null, out Assembly loaded) + ? loaded ?? Assembly.Load(asmRef) : throw SQL.UdtAssemblyNotAllowed(asmRef.Name), typeResolver: null, throwOnError: true); diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs index 7c47426421..4261a0265f 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs @@ -2980,16 +2980,16 @@ private Assembly ResolveTypeAssembly(AssemblyName asmRef, bool throwOnError) SqlClientEventSource.Log.TryTraceEvent("SqlConnection.ResolveTypeAssembly | SQL CLR type version change: Server sent {0}, client will instantiate {1}", asmRef.Version, TypeSystemAssemblyVersion); } - // The assembly name arrives from the server, and loading an assembly - // runs its module initializer, so the driver must decide whether it - // is willing to load this assembly before it hands the name to the - // loader. This call also pins the identity (version and public key - // token) of the built-in SQL CLR types assembly, so that the - // built-in exemption cannot be satisfied by a same-named assembly - // that happens to sit on the probing path. - if (!UdtAssemblyPolicy.IsAllowed(asmRef, TypeSystemAssemblyVersion)) + // The assembly name arrives from the server, so the driver must + // decide whether it is willing to bring this assembly into the + // process before it hands the name to the loader. This call also + // pins the identity (version and public key token) of the built-in + // SQL CLR types assembly, so that the built-in exemption cannot be + // satisfied by a same-named assembly that happens to sit on the + // probing path. + if (!UdtAssemblyPolicy.TryResolve(asmRef, TypeSystemAssemblyVersion, out Assembly alreadyLoaded)) { - SqlClientEventSource.Log.TryTraceEvent("SqlConnection.ResolveTypeAssembly | ERR | UDT assembly '{0}' was not loaded because it is not permitted by the '{1}' UDT assembly load policy.", asmRef.Name, UdtAssemblyPolicy.Mode); + SqlClientEventSource.Log.TryTraceEvent("SqlConnection.ResolveTypeAssembly | ERR | UDT assembly '{0}' was not loaded because the UDT assembly load policy does not permit it.", asmRef.Name); if (throwOnError) { @@ -2999,6 +2999,16 @@ private Assembly ResolveTypeAssembly(AssemblyName asmRef, bool throwOnError) return null; } + // The policy permitted the reference because the process had already + // loaded an assembly of that simple name. Use that instance rather + // than binding the server-supplied version and public key token, + // which could otherwise resolve to a different assembly and cause + // the new load this policy exists to prevent. + if (alreadyLoaded != null) + { + return alreadyLoaded; + } + try { return Assembly.Load(asmRef); diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/UdtAssemblyPolicy.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/UdtAssemblyPolicy.cs index 9ee2930456..57f1869b8e 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/UdtAssemblyPolicy.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/UdtAssemblyPolicy.cs @@ -12,51 +12,45 @@ namespace Microsoft.Data.SqlClient; -/// -/// The policy modes that govern which assemblies the driver is willing to load -/// while resolving a server-supplied UDT assembly-qualified name. -/// -internal enum UdtAssemblyLoadMode -{ - /// - /// Only the pinned Microsoft.SqlServer.Types assembly and assemblies - /// named on the application-supplied allow list may be loaded. - /// - Strict, - - /// - /// The set, plus assemblies that are already loaded - /// into the process and assemblies that are statically referenced by - /// already-loaded assemblies. This is the default. - /// - Restricted, - - /// - /// Any assembly named by the server may be loaded. This restores the - /// behavior of the driver prior to the introduction of this policy and is - /// not recommended. - /// - Legacy -} - /// /// Decides whether the driver may load an assembly named by a server-supplied /// UDT assembly-qualified name. /// /// A TDS response describing a UDT column or output parameter carries an /// AssemblyQualifiedName that the driver must resolve to a CLR -/// . Resolving it involves loading the named assembly, and -/// loading an assembly executes that assembly's module initializer. A server -/// (or an on-path attacker against a connection that has opted out of -/// certificate validation) therefore gets to choose which assembly the client -/// process loads unless the driver constrains the choice, which is what this -/// class does. +/// . A server (or an on-path attacker against a connection +/// that has opted out of certificate validation) therefore gets to choose which +/// assembly the client process loads unless the driver constrains the choice, +/// which is what this class does. +/// +/// There is a single enforcing behavior. An assembly may be loaded when it is +/// the built-in Microsoft.SqlServer.Types assembly with its identity +/// pinned, when the application has named it on the allow list, or when it is +/// already loaded into the process. Everything else is refused. +/// +/// The already-loaded case is free: re-loading an assembly that the process has +/// already loaded returns the existing instance and introduces nothing new. +/// Assemblies that are merely statically referenced are deliberately *not* +/// permitted, because loading one is a genuinely new load, which is the thing +/// this policy exists to keep under the application's control rather than the +/// server's. An application whose custom UDT assembly is not loaded at the time +/// its first UDT value arrives must name it on the allow list. +/// +/// Note that loading an assembly is not by itself the point at which foreign +/// code runs: on CoreCLR neither , nor +/// resolving a type from it, nor reading that type's custom attributes executes +/// anything from the target assembly; a module initializer runs on first real +/// access to a member. That final gate is +/// SqlConnection.CheckGetExtendedUDTInfo, which requires +/// SqlUserDefinedTypeAttribute before GetUdtValue may invoke +/// anything. This class is the layer in front of it, limiting which assemblies +/// a server can cause to be pulled into the process at all. /// /// The evaluation is deliberately cheap: apart from a one-time subscription to -/// , a decision is a couple of hash-set -/// lookups. The set of known assembly names is rebuilt only when an assembly -/// is actually loaded into the process, so a hostile server that streams a -/// large number of distinct assembly names cannot force repeated disk probing. +/// , a decision is a dictionary lookup. The +/// map of loaded assemblies is maintained incrementally, so a hostile server +/// that streams a large number of distinct assembly names cannot force repeated +/// enumeration or disk probing. /// internal static class UdtAssemblyPolicy { @@ -106,17 +100,21 @@ internal static class UdtAssemblyPolicy private static bool s_assemblyLoadHandlerAttached; /// - /// The simple names of every assembly that is loaded into the process, plus - /// the simple names of every assembly they statically reference. Null when - /// it has not been built yet. + /// Maps the simple name of every assembly loaded into the process to the + /// loaded instance. Null when it has not been built yet. /// - /// Once built, the set is maintained incrementally by the - /// handler rather than rebuilt, so an - /// application that loads assemblies while reading UDTs does not repeatedly - /// pay for a full enumeration of the process's assemblies and their - /// reference lists. + /// The instance is retained, not just the name, so that a reference which + /// is permitted because the process has already loaded that simple name is + /// satisfied with the assembly the process actually holds. Binding the + /// server-supplied version, culture and public key token instead would let + /// a server name a loaded simple name with a different identity and thereby + /// still trigger a new load, which is exactly what this tier must not do. + /// + /// When several assemblies share a simple name, the first one seen wins. + /// All of them are already in the process, so the choice cannot widen the + /// policy; at worst the subsequent type lookup fails. /// - private static HashSet? s_knownAssemblyNames; + private static Dictionary? s_loadedAssemblies; /// /// The raw allow list string that was parsed @@ -133,31 +131,13 @@ internal static class UdtAssemblyPolicy #region Properties - /// - /// The policy mode currently in effect. - /// - internal static UdtAssemblyLoadMode Mode - { - get - { - // Legacy wins over Strict so that an application that has opted - // back into the old behavior gets it unambiguously. - if (LocalAppContextSwitches.UseLegacyUdtAssemblyLoad) - { - return UdtAssemblyLoadMode.Legacy; - } - - return LocalAppContextSwitches.UseStrictUdtAssemblyLoad - ? UdtAssemblyLoadMode.Strict - : UdtAssemblyLoadMode.Restricted; - } - } - /// /// True when the policy has been disabled entirely in favor of the - /// pre-policy behavior. + /// pre-policy behavior, in which any assembly the server names may be + /// loaded and no user-defined type check is performed. /// - internal static bool LegacyBehaviorEnabled => Mode == UdtAssemblyLoadMode.Legacy; + internal static bool LegacyBehaviorEnabled => + LocalAppContextSwitches.UseLegacyUdtAssemblyLoad; #endregion @@ -191,12 +171,22 @@ internal static bool IsSqlServerTypesAssembly(AssemblyName asmRef) => /// when no connection context is available, in which case only the public /// key token is pinned and the loader picks the version. /// - /// True when the assembly may be loaded. - internal static bool IsAllowed(AssemblyName asmRef, Version? typeSystemAssemblyVersion) + /// + /// On a permitted result, the assembly the caller must use, or null when + /// the caller is to load itself. A non-null value + /// means the process had already loaded an assembly with this simple name + /// and the caller must use that instance rather than binding the + /// server-supplied identity. + /// + /// True when the assembly may be used. + internal static bool TryResolve( + AssemblyName asmRef, + Version? typeSystemAssemblyVersion, + out Assembly? assembly) { - UdtAssemblyLoadMode mode = Mode; + assembly = null; - if (mode == UdtAssemblyLoadMode.Legacy) + if (LegacyBehaviorEnabled) { return true; } @@ -216,17 +206,18 @@ internal static bool IsAllowed(AssemblyName asmRef, Version? typeSystemAssemblyV return true; } + // The allow list is the application stating which assemblies it is + // willing to have loaded on a server's say-so, so the reference is + // handed to the loader as given. if (MatchesAllowList(asmRef)) { return true; } - if (mode == UdtAssemblyLoadMode.Restricted && IsKnownToProcess(simpleName!)) - { - return true; - } - - return false; + // Otherwise the only remaining basis is that the process already holds + // an assembly by this simple name, in which case that instance is used + // and the server-supplied identity is discarded. + return TryGetLoadedAssembly(simpleName!, out assembly); } /// @@ -266,7 +257,7 @@ internal static void ResetCache() { s_allowList = null; s_allowListSource = null; - s_knownAssemblyNames = null; + s_loadedAssemblies = null; } } @@ -392,113 +383,106 @@ private static List GetAllowList() } /// - /// Determines whether an assembly with the given simple name is already - /// loaded into the process, or is statically referenced by an assembly that - /// is. + /// Looks up an assembly that the process has already loaded under the given + /// simple name. /// - private static bool IsKnownToProcess(string simpleName) => - GetKnownAssemblyNames().Contains(simpleName); + private static bool TryGetLoadedAssembly(string simpleName, out Assembly? assembly) + { + lock (s_lock) + { + return GetLoadedAssemblies().TryGetValue(simpleName, out assembly); + } + } /// - /// Returns the set of assembly simple names that are loaded into the - /// process or referenced by an assembly that is, building it on first use - /// and thereafter relying on the - /// handler to keep it current. + /// Returns the map of loaded assembly simple names to instances, building it + /// on first use and thereafter relying on the + /// handler to keep it current. /// - private static HashSet GetKnownAssemblyNames() + /// + /// Callers must hold . + /// + private static Dictionary GetLoadedAssemblies() { EnsureAssemblyLoadHandlerAttached(); - lock (s_lock) + if (s_loadedAssemblies is not null) { - if (s_knownAssemblyNames is not null) - { - return s_knownAssemblyNames; - } + return s_loadedAssemblies; + } - HashSet names = new(StringComparer.OrdinalIgnoreCase); + Dictionary loaded = new(StringComparer.OrdinalIgnoreCase); - foreach (Assembly assembly in AppDomain.CurrentDomain.GetAssemblies()) - { - AddAssemblyNames(names, assembly); - } + foreach (Assembly assembly in AppDomain.CurrentDomain.GetAssemblies()) + { + Remember(loaded, assembly); + } - s_knownAssemblyNames = names; + s_loadedAssemblies = loaded; - return names; - } + return loaded; } /// - /// Adds the simple name of and the simple names - /// of every assembly it statically references to . + /// Records under its simple name, keeping the + /// first assembly seen for a given name. /// - private static void AddAssemblyNames(HashSet names, Assembly assembly) + private static void Remember(Dictionary loaded, Assembly assembly) { if (assembly.IsDynamic) { - // A dynamic assembly has no manifest to read references from, and - // it cannot be a target of Assembly.Load by name anyway. + // A dynamic assembly cannot be the target of Assembly.Load by name, + // and asking one for its name can throw. return; } - string? name = null; - try { - name = assembly.GetName().Name; - if (!string.IsNullOrEmpty(name)) - { - names.Add(name!); - } + string? name = assembly.GetName().Name; - foreach (AssemblyName reference in assembly.GetReferencedAssemblies()) + if (!string.IsNullOrEmpty(name) && !loaded.ContainsKey(name!)) { - if (!string.IsNullOrEmpty(reference.Name)) - { - names.Add(reference.Name!); - } + loaded.Add(name!, assembly); } } catch (Exception e) when (ADP.IsCatchableExceptionType(e)) { - // Reading the name or the reference list can fail for assemblies - // loaded from a byte array or produced by a trimmer. Losing one - // assembly's references only makes the policy stricter. + // Reading the name can fail for assemblies loaded from a byte array + // or produced by a trimmer. Losing one only makes the policy + // stricter. SqlClientEventSource.Log.TryTraceEvent( - "UdtAssemblyPolicy.AddAssemblyNames | INFO | Unable to read references of '{0}'.", - name); + "UdtAssemblyPolicy.Remember | INFO | Unable to read the name of a loaded assembly."); } } /// - /// Attaches the assembly load handler that keeps the cached - /// known-assembly-name set current, if it has not been attached already. + /// Attaches the assembly load handler that keeps the cached map of loaded + /// assemblies current, if it has not been attached already. /// + /// + /// Callers must hold . + /// private static void EnsureAssemblyLoadHandlerAttached() { - lock (s_lock) + if (s_assemblyLoadHandlerAttached) { - if (s_assemblyLoadHandlerAttached) - { - return; - } + return; + } - AppDomain.CurrentDomain.AssemblyLoad += static (_, args) => + AppDomain.CurrentDomain.AssemblyLoad += static (_, args) => + { + lock (s_lock) { - lock (s_lock) + // Nothing to update if the map has not been built yet; it will + // pick the assembly up when it is. + if (s_loadedAssemblies is not null) { - // Nothing to update if the set has not been built yet; it - // will pick the assembly up when it is. - if (s_knownAssemblyNames is not null) - { - AddAssemblyNames(s_knownAssemblyNames, args.LoadedAssembly); - } + Remember(s_loadedAssemblies, args.LoadedAssembly); } - }; + } + }; - s_assemblyLoadHandlerAttached = true; - } + s_assemblyLoadHandlerAttached = true; } #endregion diff --git a/src/Microsoft.Data.SqlClient/tests/Common/LocalAppContextSwitchesHelper.cs b/src/Microsoft.Data.SqlClient/tests/Common/LocalAppContextSwitchesHelper.cs index 6818c43727..e67d39415d 100644 --- a/src/Microsoft.Data.SqlClient/tests/Common/LocalAppContextSwitchesHelper.cs +++ b/src/Microsoft.Data.SqlClient/tests/Common/LocalAppContextSwitchesHelper.cs @@ -60,7 +60,6 @@ public sealed class LocalAppContextSwitchesHelper : IDisposable private readonly bool? _useLegacyIdleTimeoutBehaviorOriginal; private readonly bool? _useOverallConnectTimeoutForPoolWaitOriginal; private readonly bool? _useLegacyUdtAssemblyLoadOriginal; - private readonly bool? _useStrictUdtAssemblyLoadOriginal; #if NET // The s_useManagedNetworking field only exists in the SqlClient assembly // when it is built for .NET on Windows, so it is captured/restored at @@ -131,8 +130,6 @@ public LocalAppContextSwitchesHelper() GetSwitchValue("s_useOverallConnectTimeoutForPoolWait"); _useLegacyUdtAssemblyLoadOriginal = GetSwitchValue("s_useLegacyUdtAssemblyLoad"); - _useStrictUdtAssemblyLoadOriginal = - GetSwitchValue("s_useStrictUdtAssemblyLoad"); #if NET if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) { @@ -212,9 +209,6 @@ public void Dispose() SetSwitchValue( "s_useLegacyUdtAssemblyLoad", _useLegacyUdtAssemblyLoadOriginal); - SetSwitchValue( - "s_useStrictUdtAssemblyLoad", - _useStrictUdtAssemblyLoadOriginal); #if NET if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) { @@ -391,15 +385,6 @@ public bool? UseLegacyUdtAssemblyLoad set => SetSwitchValue("s_useLegacyUdtAssemblyLoad", value); } - /// - /// Get or set the UseStrictUdtAssemblyLoad switch value. - /// - public bool? UseStrictUdtAssemblyLoad - { - get => GetSwitchPropertyValue(nameof(UseStrictUdtAssemblyLoad)); - set => SetSwitchValue("s_useStrictUdtAssemblyLoad", value); - } - #if NET /// /// Get or set the UseManagedNetworking switch value. diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/LocalAppContextSwitchesTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/LocalAppContextSwitchesTest.cs index 468edb950d..72c72403f3 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/LocalAppContextSwitchesTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/LocalAppContextSwitchesTest.cs @@ -45,7 +45,6 @@ public void TestDefaultAppContextSwitchValues() switchesHelper.UseLegacyIdleTimeoutBehavior = null; switchesHelper.UseMinimumLoginTimeout = null; switchesHelper.UseLegacyUdtAssemblyLoad = null; - switchesHelper.UseStrictUdtAssemblyLoad = null; #if NET switchesHelper.GlobalizationInvariantMode = null; if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) @@ -72,7 +71,6 @@ public void TestDefaultAppContextSwitchValues() Assert.False(switchesHelper.UseLegacyFailoverAlternationOnLoginSqlErrors); Assert.False(switchesHelper.EnableMultiSubnetFailoverByDefault); Assert.False(switchesHelper.UseLegacyUdtAssemblyLoad); - Assert.False(switchesHelper.UseStrictUdtAssemblyLoad); #if NET Assert.False(switchesHelper.GlobalizationInvariantMode); if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyLoadHardeningTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyLoadHardeningTest.cs index db2fa20b4a..423d86aead 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyLoadHardeningTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyLoadHardeningTest.cs @@ -252,14 +252,13 @@ private sealed class PolicyScope : IDisposable private readonly LocalAppContextSwitchesHelper _switches; private readonly object? _originalAllowList; - public PolicyScope(bool legacy = false, bool strict = false) + public PolicyScope(bool legacy = false) { _switches = new LocalAppContextSwitchesHelper(); _originalAllowList = AppContext.GetData(UdtAssemblyPolicy.AllowListAppContextDataName); _switches.UseLegacyUdtAssemblyLoad = legacy; - _switches.UseStrictUdtAssemblyLoad = strict; AppDomain.CurrentDomain.SetData( UdtAssemblyPolicy.AllowListAppContextDataName, diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs index 64bf505c08..51844d9eda 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs @@ -3,6 +3,8 @@ // See the LICENSE file in the project root for more information. using System; +using System.Collections.Generic; +using System.Linq; using System.Reflection; using Microsoft.Data.SqlClient.Tests.Common; using Xunit; @@ -28,6 +30,13 @@ public class UdtAssemblyPolicyTest /// private const string UnknownAssemblyName = "Contoso.Totally.Unknown.Assembly"; + /// + /// Asks the policy for a decision, discarding the resolved assembly. Most + /// tests care only whether the reference was permitted. + /// + private static bool IsAllowed(AssemblyName asmRef, Version? typeSystemAssemblyVersion) => + UdtAssemblyPolicy.TryResolve(asmRef, typeSystemAssemblyVersion, out _); + #region Scope /// @@ -41,14 +50,13 @@ private sealed class PolicyScope : IDisposable private readonly LocalAppContextSwitchesHelper _switches; private readonly object? _originalAllowList; - public PolicyScope(bool legacy = false, bool strict = false) + public PolicyScope(bool legacy = false) { _switches = new LocalAppContextSwitchesHelper(); _originalAllowList = AppContext.GetData(UdtAssemblyPolicy.AllowListAppContextDataName); _switches.UseLegacyUdtAssemblyLoad = legacy; - _switches.UseStrictUdtAssemblyLoad = strict; SetAllowList(null); } @@ -73,46 +81,28 @@ public void Dispose() #endregion - #region Mode + #region Enforcement /// - /// Verifies that the policy defaults to Restricted when neither switch is - /// set. + /// Verifies that the policy enforces by default, and that there is exactly + /// one enforcing behavior: the only alternative is the legacy escape hatch. /// [Fact] - public void Mode_DefaultsToRestricted() + public void Policy_EnforcesByDefault() { using PolicyScope scope = new(); - Assert.Equal(UdtAssemblyLoadMode.Restricted, UdtAssemblyPolicy.Mode); Assert.False(UdtAssemblyPolicy.LegacyBehaviorEnabled); } /// - /// Verifies that the strict switch selects Strict mode. + /// Verifies that the legacy switch disables the policy entirely. /// [Fact] - public void Mode_StrictSwitch_SelectsStrict() + public void Policy_LegacySwitch_DisablesEnforcement() { - using PolicyScope scope = new(strict: true); - - Assert.Equal(UdtAssemblyLoadMode.Strict, UdtAssemblyPolicy.Mode); - Assert.False(UdtAssemblyPolicy.LegacyBehaviorEnabled); - } - - /// - /// Verifies that the legacy switch selects Legacy mode and takes precedence - /// over the strict switch, so an application that has opted back into the - /// old behavior gets it unambiguously. - /// - [Theory] - [InlineData(false)] - [InlineData(true)] - public void Mode_LegacySwitch_WinsOverStrict(bool strict) - { - using PolicyScope scope = new(legacy: true, strict: strict); + using PolicyScope scope = new(legacy: true); - Assert.Equal(UdtAssemblyLoadMode.Legacy, UdtAssemblyPolicy.Mode); Assert.True(UdtAssemblyPolicy.LegacyBehaviorEnabled); } @@ -124,16 +114,14 @@ public void Mode_LegacySwitch_WinsOverStrict(bool strict) /// Verifies that the built-in SQL Server CLR types assembly is recognized /// case-insensitively and is permitted in every non-legacy mode. /// - [Theory] - [InlineData(false)] - [InlineData(true)] - public void IsAllowed_SqlServerTypes_IsPermitted(bool strict) + [Fact] + public void IsAllowed_SqlServerTypes_IsPermitted() { - using PolicyScope scope = new(strict: strict); + using PolicyScope scope = new(); Assert.True(UdtAssemblyPolicy.IsSqlServerTypesAssembly( new AssemblyName("microsoft.sqlserver.types"))); - Assert.True(UdtAssemblyPolicy.IsAllowed( + Assert.True(IsAllowed( new AssemblyName("Microsoft.SqlServer.Types"), null)); } @@ -148,7 +136,7 @@ public void IsAllowed_SqlServerTypes_IsPermitted(bool strict) [Fact] public void IsAllowed_SqlServerTypes_PinsVersionAndPublicKeyToken() { - using PolicyScope scope = new(strict: false); + using PolicyScope scope = new(); // A reference as an attacker-controlled server might send it: the right // simple name, but a bogus version and no strong-name identity. @@ -157,7 +145,7 @@ public void IsAllowed_SqlServerTypes_PinsVersionAndPublicKeyToken() Version = new Version(1, 2, 3, 4), }; - Assert.True(UdtAssemblyPolicy.IsAllowed(asmRef, new Version(14, 0, 0, 0))); + Assert.True(IsAllowed(asmRef, new Version(14, 0, 0, 0))); Assert.Equal(new Version(14, 0, 0, 0), asmRef.Version); Assert.Equal( @@ -172,12 +160,12 @@ public void IsAllowed_SqlServerTypes_PinsVersionAndPublicKeyToken() [Fact] public void IsAllowed_SqlServerTypes_OverwritesServerSuppliedToken() { - using PolicyScope scope = new(strict: false); + using PolicyScope scope = new(); AssemblyName asmRef = new( "Microsoft.SqlServer.Types, Version=1.0.0.0, Culture=neutral, PublicKeyToken=0123456789abcdef"); - Assert.True(UdtAssemblyPolicy.IsAllowed(asmRef, new Version(11, 0, 0, 0))); + Assert.True(IsAllowed(asmRef, new Version(11, 0, 0, 0))); Assert.Equal( SqlServerTypesPublicKeyToken, @@ -192,11 +180,11 @@ public void IsAllowed_SqlServerTypes_OverwritesServerSuppliedToken() [Fact] public void IsAllowed_SqlServerTypes_WithoutVersion_StillPinsToken() { - using PolicyScope scope = new(strict: false); + using PolicyScope scope = new(); AssemblyName asmRef = new("Microsoft.SqlServer.Types, Version=1.0.0.0"); - Assert.True(UdtAssemblyPolicy.IsAllowed(asmRef, null)); + Assert.True(IsAllowed(asmRef, null)); Assert.Equal(new Version(1, 0, 0, 0), asmRef.Version); Assert.Equal( @@ -213,14 +201,12 @@ public void IsAllowed_SqlServerTypes_WithoutVersion_StillPinsToken() /// both enforcing modes. This is the reporter's scenario: a server-supplied /// name that resolves to a DLL planted on the probing path. /// - [Theory] - [InlineData(false)] - [InlineData(true)] - public void IsAllowed_UnknownAssembly_IsDenied(bool strict) + [Fact] + public void IsAllowed_UnknownAssembly_IsDenied() { - using PolicyScope scope = new(strict: strict); + using PolicyScope scope = new(); - Assert.False(UdtAssemblyPolicy.IsAllowed(new AssemblyName(UnknownAssemblyName), null)); + Assert.False(IsAllowed(new AssemblyName(UnknownAssemblyName), null)); } /// @@ -232,55 +218,92 @@ public void IsAllowed_LegacyMode_PermitsEverything() { using PolicyScope scope = new(legacy: true); - Assert.True(UdtAssemblyPolicy.IsAllowed(new AssemblyName(UnknownAssemblyName), null)); + Assert.True(IsAllowed(new AssemblyName(UnknownAssemblyName), null)); } #endregion - #region Loaded and referenced assemblies + #region Loaded assemblies /// - /// Verifies that an assembly already loaded into the process is permitted in - /// Restricted mode but denied in Strict mode. + /// Verifies that an assembly already loaded into the process is permitted. + /// Re-resolving a loaded assembly cannot bring anything new into the + /// process, so this tier costs nothing. /// [Fact] - public void IsAllowed_LoadedAssembly_DependsOnMode() + public void Resolve_LoadedAssembly_IsPermitted() { // This test assembly is, by definition, loaded. - string loadedName = typeof(UdtAssemblyPolicyTest).Assembly.GetName().Name!; + Assembly self = typeof(UdtAssemblyPolicyTest).Assembly; - using (PolicyScope restricted = new()) - { - Assert.True(UdtAssemblyPolicy.IsAllowed(new AssemblyName(loadedName), null)); - } + using PolicyScope scope = new(); - using (PolicyScope strict = new(strict: true)) - { - Assert.False(UdtAssemblyPolicy.IsAllowed(new AssemblyName(loadedName), null)); - } + Assert.True(UdtAssemblyPolicy.TryResolve( + new AssemblyName(self.GetName().Name!), null, out Assembly? resolved)); + Assert.Same(self, resolved); } /// - /// Verifies that an assembly that is statically referenced by a loaded - /// assembly, but that may not itself be loaded yet, is permitted in - /// Restricted mode. This is what keeps lazily-loaded custom UDT assemblies - /// working. + /// Verifies that a reference permitted because the process already holds + /// that simple name resolves to the loaded instance, and that the + /// server-supplied version and public key token are discarded. /// + /// + /// Matching on the simple name and then handing the server's full reference + /// to the loader would let a server name a loaded assembly with a different + /// identity and still cause a genuinely new load, which is precisely what + /// this tier must not permit. + /// [Fact] - public void IsAllowed_ReferencedAssembly_IsPermittedWhenRestricted() + public void Resolve_LoadedAssembly_IgnoresServerSuppliedIdentity() { - AssemblyName[] references = - typeof(UdtAssemblyPolicyTest).Assembly.GetReferencedAssemblies(); - Assert.NotEmpty(references); + Assembly self = typeof(UdtAssemblyPolicyTest).Assembly; + string simpleName = self.GetName().Name!; using PolicyScope scope = new(); - foreach (AssemblyName reference in references) + AssemblyName hostile = new( + $"{simpleName}, Version=9.9.9.9, Culture=neutral, PublicKeyToken=0123456789abcdef"); + + Assert.True(UdtAssemblyPolicy.TryResolve(hostile, null, out Assembly? resolved)); + Assert.Same(self, resolved); + Assert.NotEqual(new Version(9, 9, 9, 9), resolved!.GetName().Version); + } + + /// + /// Verifies that an assembly which is merely statically referenced by a + /// loaded assembly, but is not itself loaded, is denied. + /// + /// + /// Loading a referenced-but-unloaded assembly is a genuinely new load, and + /// keeping new loads under the application's control rather than the + /// server's is the entire point of this policy. An application whose custom + /// UDT assembly is not yet loaded must name it on the allow list. + /// + [Fact] + public void Resolve_ReferencedButUnloadedAssembly_IsDenied() + { + HashSet loaded = new( + AppDomain.CurrentDomain.GetAssemblies() + .Where(a => !a.IsDynamic) + .Select(a => a.GetName().Name!), + StringComparer.OrdinalIgnoreCase); + + AssemblyName? referencedNotLoaded = typeof(UdtAssemblyPolicyTest).Assembly + .GetReferencedAssemblies() + .FirstOrDefault(r => !loaded.Contains(r.Name!)); + + if (referencedNotLoaded is null) { - Assert.True( - UdtAssemblyPolicy.IsAllowed(new AssemblyName(reference.Name!), null), - $"Expected referenced assembly '{reference.Name}' to be permitted."); + // Every referenced assembly happens to be loaded in this run, so + // there is nothing here to distinguish. The companion test + // Resolve_UnknownAssembly_IsDenied covers the general deny path. + return; } + + using PolicyScope scope = new(); + + Assert.False(IsAllowed(new AssemblyName(referencedNotLoaded.Name!), null)); } #endregion @@ -292,16 +315,14 @@ public void IsAllowed_ReferencedAssembly_IsPermittedWhenRestricted() /// every enforcing mode, and that it does so regardless of the version, /// culture, and public key token the server supplies. /// - [Theory] - [InlineData(false)] - [InlineData(true)] - public void IsAllowed_AllowListSimpleName_Permits(bool strict) + [Fact] + public void IsAllowed_AllowListSimpleName_Permits() { - using PolicyScope scope = new(strict: strict); + using PolicyScope scope = new(); PolicyScope.SetAllowList(UnknownAssemblyName); - Assert.True(UdtAssemblyPolicy.IsAllowed(new AssemblyName(UnknownAssemblyName), null)); - Assert.True(UdtAssemblyPolicy.IsAllowed(new AssemblyName( + Assert.True(IsAllowed(new AssemblyName(UnknownAssemblyName), null)); + Assert.True(IsAllowed(new AssemblyName( $"{UnknownAssemblyName}, Version=9.9.9.9, Culture=neutral, PublicKeyToken=0123456789abcdef"), null)); } @@ -312,10 +333,10 @@ public void IsAllowed_AllowListSimpleName_Permits(bool strict) [Fact] public void IsAllowed_AllowList_IgnoresCaseAndWhitespace() { - using PolicyScope scope = new(strict: true); + using PolicyScope scope = new(); PolicyScope.SetAllowList($" ; {UnknownAssemblyName.ToUpperInvariant()} ; "); - Assert.True(UdtAssemblyPolicy.IsAllowed(new AssemblyName(UnknownAssemblyName), null)); + Assert.True(IsAllowed(new AssemblyName(UnknownAssemblyName), null)); } /// @@ -326,24 +347,24 @@ public void IsAllowed_AllowList_IgnoresCaseAndWhitespace() [Fact] public void IsAllowed_AllowListFullName_MatchesAllSpecifiedComponents() { - using PolicyScope scope = new(strict: true); + using PolicyScope scope = new(); PolicyScope.SetAllowList( $"{UnknownAssemblyName}, Version=1.0.0.0, Culture=neutral, PublicKeyToken=0123456789abcdef"); // Exact match. - Assert.True(UdtAssemblyPolicy.IsAllowed(new AssemblyName( + Assert.True(IsAllowed(new AssemblyName( $"{UnknownAssemblyName}, Version=1.0.0.0, Culture=neutral, PublicKeyToken=0123456789abcdef"), null)); // Wrong version. - Assert.False(UdtAssemblyPolicy.IsAllowed(new AssemblyName( + Assert.False(IsAllowed(new AssemblyName( $"{UnknownAssemblyName}, Version=2.0.0.0, Culture=neutral, PublicKeyToken=0123456789abcdef"), null)); // Wrong public key token. - Assert.False(UdtAssemblyPolicy.IsAllowed(new AssemblyName( + Assert.False(IsAllowed(new AssemblyName( $"{UnknownAssemblyName}, Version=1.0.0.0, Culture=neutral, PublicKeyToken=fedcba9876543210"), null)); // No public key token at all. - Assert.False(UdtAssemblyPolicy.IsAllowed(new AssemblyName( + Assert.False(IsAllowed(new AssemblyName( $"{UnknownAssemblyName}, Version=1.0.0.0, Culture=neutral"), null)); } @@ -355,11 +376,11 @@ public void IsAllowed_AllowListFullName_MatchesAllSpecifiedComponents() [Fact] public void IsAllowed_MalformedAllowListEntry_IsSkipped() { - using PolicyScope scope = new(strict: true); + using PolicyScope scope = new(); PolicyScope.SetAllowList($", , Version=bogus ; {UnknownAssemblyName}"); - Assert.True(UdtAssemblyPolicy.IsAllowed(new AssemblyName(UnknownAssemblyName), null)); - Assert.False(UdtAssemblyPolicy.IsAllowed(new AssemblyName("Some.Other.Assembly"), null)); + Assert.True(IsAllowed(new AssemblyName(UnknownAssemblyName), null)); + Assert.False(IsAllowed(new AssemblyName("Some.Other.Assembly"), null)); } /// @@ -369,15 +390,15 @@ public void IsAllowed_MalformedAllowListEntry_IsSkipped() [Fact] public void IsAllowed_AllowListChange_IsObserved() { - using PolicyScope scope = new(strict: true); + using PolicyScope scope = new(); - Assert.False(UdtAssemblyPolicy.IsAllowed(new AssemblyName(UnknownAssemblyName), null)); + Assert.False(IsAllowed(new AssemblyName(UnknownAssemblyName), null)); AppDomain.CurrentDomain.SetData( UdtAssemblyPolicy.AllowListAppContextDataName, UnknownAssemblyName); - Assert.True(UdtAssemblyPolicy.IsAllowed(new AssemblyName(UnknownAssemblyName), null)); + Assert.True(IsAllowed(new AssemblyName(UnknownAssemblyName), null)); } /// @@ -389,7 +410,7 @@ public void IsAllowed_EmptySimpleName_IsDenied() { using PolicyScope scope = new(); - Assert.False(UdtAssemblyPolicy.IsAllowed(new AssemblyName(), null)); + Assert.False(IsAllowed(new AssemblyName(), null)); } #endregion From ff520613d392013fa262fd84ca18b2ed4853747c Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Mon, 24 Aug 2026 11:10:01 -0700 Subject: [PATCH 05/10] Document the UDT policy compatibility impact Records the decision to ship the single enforcing mode as-is and accept the compatibility break rather than staging the reference closure behind a deprecation release. Documents which applications are affected, why the affected shape is common (the driver materializes the value and the application never names the UDT type itself, so the driver's own Assembly.Load was previously what pulled the assembly in), and both symptom shapes. The fThrow: false path is called out specifically, because GetFieldType, GetSchemaTable and GetColumnSchema return null for a denied UDT column rather than throwing, and a caller that dereferences the result sees an unrelated NullReferenceException. Denials are always traced, so event source tracing identifies the assembly in either case. No behavior change. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60806d12-bdac-413c-bf83-f7c12e3cc12c --- .github/instructions/features.instructions.md | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/.github/instructions/features.instructions.md b/.github/instructions/features.instructions.md index 1da3e117d8..8dcf4bbae0 100644 --- a/.github/instructions/features.instructions.md +++ b/.github/instructions/features.instructions.md @@ -302,6 +302,34 @@ from the assembly, nor reading that type's custom attributes runs anything from it — a module initializer or static constructor runs on first real member access, which is what `GetUdtValue` would otherwise perform. +#### Compatibility impact + +This policy is a behavior change for applications that use **custom** UDTs. The +built-in spatial types (`SqlGeography`, `SqlGeometry`, `SqlHierarchyId`) are +unaffected, since `Microsoft.SqlServer.Types` is permitted by identity. + +An application is affected when the custom UDT's assembly is not yet loaded at +the moment the value is read. That is common whenever the *driver* materializes +the value and the application never names the type in its own code — generic data +access layers, micro-ORMs, `DataTable.Load`, and schema discovery. In those cases +the driver's own `Assembly.Load` was previously the thing that pulled the +assembly in, and it is now refused. + +The symptom depends on the API: + +| API | Symptom | +|-----|---------| +| `reader[i]`, `GetValue`, UDT output parameters | `SqlException` naming the assembly and the allow list | +| `GetFieldType`, `GetSchemaTable`, `GetColumnSchema` | Returns `null` for the UDT column's type rather than throwing | + +The second row is the harder one to diagnose, because `GetFieldType` does not +normally return `null`; a caller that dereferences the result sees an unrelated +`NullReferenceException`. A denial is always traced through +`SqlClientEventSource` regardless of which path was taken, so enabling event +source tracing will identify the assembly. + +The remedy in every case is to name the assembly on the allow list. + ### Usage Example ```csharp // Set via AppContext before opening any connection From a3d30a615dac47305665e553b979407872edaaa8 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Wed, 23 Sep 2026 14:16:01 -0700 Subject: [PATCH 06/10] Verify loaded assembly identity in the UDT policy Addresses review feedback on the UDT assembly load hardening. The central issue: on .NET the loader ignores the public key token in an AssemblyName, so pinning the reference was not an enforcement boundary. A caller could consult the policy and still be handed a same-named assembly with a different identity. SqlAuthenticationProviderManager already documents this for the Azure extension assembly and compensates with a post-load check; the UDT path now does the same. To make that impossible to forget, the load moves inside the policy. TryResolve (decide, then let the caller load) is replaced by TryLoad, which decides, loads, and verifies that the assembly the loader returned carries the identity the decision required. IsPermitted exposes the decision half for tests that use names with no file on disk. Also in the policy: - Pin the culture of Microsoft.SqlServer.Types to neutral. It was left server-controlled, so a Culture= in the reference could steer the bind. - Distinguish an omitted public key token from an explicit PublicKeyToken=null in allow list entries. AssemblyName represents the first as null and the second as a zero-length array, and treating them alike silently widened an entry written to pin an unsigned assembly into one accepting any identity. - Hold weak references to observed assemblies, and scope the already-loaded tier to the driver's own AssemblyLoadContext. Strong references would let a server force the map to be built with one denied UDT and thereby pin unrelated plugin assemblies, preventing a collectible context from unloading. The SMI path gains the SqlUserDefinedTypeAttribute gate it was missing. It applied the assembly policy but not the type check, so a bare or permitted non-UDT name could still reach ValueUtilsSmi.NullUdtInstance, which invokes the type's static Null member. It also now traces denials, so the documented claim that every denial is observable holds on both paths. Tests: - TryLoad_LoadedAssemblyWithWrongToken_IsRefused drives the real load path and demonstrates the loader ignoring the requested token. - Cover explicit PublicKeyToken=null, omitted token, and culture pinning. - Assert the policy denial specifically rather than only that nothing was loaded. Because the hostile assembly does not exist, "nothing loaded" also passed against the vulnerable implementation. - Add a positive control so the denial tests cannot pass by resolution being broken outright. - Assert the referenced-but-unloaded precondition instead of returning quietly when it cannot be established. Docs: correct the exception type to TypeLoadException, qualify the code-execution ordering claim as measured on CoreCLR rather than stated unconditionally, note that the attribute lookup is filtered and the attribute sealed so it cannot itself run foreign code, and document that trust is per process rather than per server. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60806d12-bdac-413c-bf83-f7c12e3cc12c --- .github/instructions/features.instructions.md | 70 +++- .../Data/SqlClient/Server/SmiMetaData.cs | 62 +++- .../Microsoft/Data/SqlClient/SqlConnection.cs | 35 +- .../Data/SqlClient/UdtAssemblyPolicy.cs | 327 ++++++++++++++++-- .../SqlClient/UdtAssemblyLoadHardeningTest.cs | 59 +++- .../Data/SqlClient/UdtAssemblyPolicyTest.cs | 164 ++++++++- 6 files changed, 628 insertions(+), 89 deletions(-) diff --git a/.github/instructions/features.instructions.md b/.github/instructions/features.instructions.md index 8dcf4bbae0..3888f4cf92 100644 --- a/.github/instructions/features.instructions.md +++ b/.github/instructions/features.instructions.md @@ -267,15 +267,30 @@ There is a single enforcing behavior, which permits: | Permitted | Notes | |-----------|-------| -| `Microsoft.SqlServer.Types` | Identity pinned: the version is normalized to the connection's negotiated type system version, and the public key token to the one Microsoft signs with | +| `Microsoft.SqlServer.Types` | Identity pinned: the version is normalized to the connection's negotiated type system version, the culture to neutral, and the public key token to the one Microsoft signs with | | Assemblies on the allow list | The application explicitly naming what it is willing to have loaded | -| Assemblies already loaded into the process | Resolved to the instance the process already holds; the server-supplied version and public key token are discarded | +| Assemblies already loaded into the process | Resolved to the instance the process already holds; the server-supplied version, culture and public key token are discarded | Everything else is refused. In particular, an assembly that is only *statically referenced* by a loaded assembly is **not** permitted, because loading it is a genuinely new load — precisely what this policy keeps under the application's control rather than the server's. +Normalizing the reference is necessary but not sufficient. On .NET the loader +**ignores** the public key token in an `AssemblyName`, so pinning it does not by +itself prevent a same-named assembly with a different identity from being +returned. The driver therefore verifies the identity of the assembly the loader +actually hands back, and refuses it if it does not carry the required token. +This mirrors what the driver already does for the Azure authentication extension +assembly. + +On .NET, the already-loaded tier is scoped to the `AssemblyLoadContext` that +loaded the driver, since that is the context its `Assembly.Load` calls resolve +into. An application that loads its UDT assembly into a separate (for example +collectible) context must name it on the allow list. The driver holds only weak +references to the assemblies it has observed, so this policy never prevents a +collectible context from unloading. + Setting `UseLegacyUdtAssemblyLoad` disables the policy entirely and restores the pre-policy behavior. It is a temporary compatibility escape hatch, not a supported configuration. @@ -292,15 +307,47 @@ AppDomain.CurrentDomain.SetData( Each entry is matched only on the components it specifies, so a simple name permits any version, culture, and public key token, while a fully-qualified name -must match exactly. +must match exactly. An entry that explicitly specifies `PublicKeyToken=null` +requires an unsigned assembly and is not satisfied by a signed one; this is +distinct from omitting the token, which places no constraint on it. Independently of the assembly policy, a resolved type that is not annotated with -`SqlUserDefinedTypeAttribute` is rejected before any of its code runs (except -under `UseLegacyUdtAssemblyLoad`). This is the gate that actually prevents -foreign code execution: on CoreCLR, neither `Assembly.Load`, nor resolving a type -from the assembly, nor reading that type's custom attributes runs anything from -it — a module initializer or static constructor runs on first real member access, -which is what `GetUdtValue` would otherwise perform. +`SqlUserDefinedTypeAttribute` is rejected before any member of it is accessed +(except under `UseLegacyUdtAssemblyLoad`). This is the gate that actually +prevents foreign code execution. + +On CoreCLR this has been measured directly: neither `Assembly.Load`, nor +resolving a type from the assembly, nor reading that type's custom attributes +runs anything from it. A module initializer or static constructor runs on first +real member access, which is what `GetUdtValue` would otherwise perform. The +attribute check therefore sits in front of the only step that executes code. + +Module initializer timing on .NET Framework has not been measured, and ECMA-335 +permits a runtime to run one earlier than CoreCLR does. The portable guarantee +is the one stated above — no member of the type is accessed before the attribute +check — rather than a claim about exactly when the runtime chooses to run +initializers. + +Note that the attribute check itself does not execute foreign code. +`SqlUserDefinedTypeAttribute` is `sealed`, so it cannot be subclassed by a +hostile assembly, and the lookup is filtered to that single attribute type, so +the constructors of any other attributes on the type are never invoked. + +#### Trust is per process, not per server + +The already-loaded tier makes the permitted set a property of the process rather +than of the connection. Once an assembly is loaded by any means, a UDT type +within it can be instantiated on the say-so of any server the process connects +to, whether or not that assembly was loaded for that server's benefit. The +resolved type must still carry `SqlUserDefinedTypeAttribute`, so this is +confined to types that were written to be deserialized from SQL Server, but it +is a genuine widening and is called out here deliberately. + +Relatedly, the map of loaded assemblies is built once and then maintained +incrementally. That is not only a performance choice: rebuilding it on demand +would let an assembly that was pulled in as a *dependency* of a permitted +assembly silently inherit that permission. Building it once keeps the tier +anchored to what the application had already loaded. #### Compatibility impact @@ -319,9 +366,12 @@ The symptom depends on the API: | API | Symptom | |-----|---------| -| `reader[i]`, `GetValue`, UDT output parameters | `SqlException` naming the assembly and the allow list | +| `reader[i]`, `GetValue`, UDT output parameters | `TypeLoadException` naming the assembly and the allow list | | `GetFieldType`, `GetSchemaTable`, `GetColumnSchema` | Returns `null` for the UDT column's type rather than throwing | +The exception is a `TypeLoadException` and is not wrapped in a `SqlException`, +which matches how the driver already reports a UDT type it cannot resolve. + The second row is the harder one to diagnose, because `GetFieldType` does not normally return `null`; a caller that dereferences the result sees an unrelated `NullReferenceException`. A denial is always traced through diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SmiMetaData.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SmiMetaData.cs index a6b828d295..a0317ed6fb 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SmiMetaData.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SmiMetaData.cs @@ -10,6 +10,8 @@ using System.Diagnostics.CodeAnalysis; using System.Globalization; using System.Reflection; +using Microsoft.Data.Common; +using Microsoft.Data.SqlClient.Internal; namespace Microsoft.Data.SqlClient.Server { @@ -383,20 +385,70 @@ internal Type Type // SqlConnection.ResolveTypeAssembly applies. There is no // connection context here, so no type system version is // available to pin the built-in SQL CLR types assembly to; - // its public key token is still pinned. - _clrType = Type.GetType( + // its culture and public key token are still pinned, and the + // policy verifies the identity of whatever the loader + // returns. + Type resolved = Type.GetType( typeName: _udtAssemblyQualifiedName, assemblyResolver: static asmRef => - UdtAssemblyPolicy.TryResolve(asmRef, typeSystemAssemblyVersion: null, out Assembly loaded) - ? loaded ?? Assembly.Load(asmRef) - : throw SQL.UdtAssemblyNotAllowed(asmRef.Name), + UdtAssemblyPolicy.TryLoad(asmRef, typeSystemAssemblyVersion: null, out Assembly loaded) + ? loaded + : throw UdtAssemblyDenied(asmRef), typeResolver: null, throwOnError: true); + + // A name that carries no assembly part never reaches the + // assembly resolver above, so the attribute gate is the only + // thing standing between a server-chosen type name and + // ValueUtilsSmi.NullUdtInstance invoking its static Null + // member. Apply it here as SqlConnection does on the main + // path, so this route cannot be used to run the code of a + // type that is not actually a user-defined type. + if (resolved != null && !UdtAssemblyPolicy.LegacyBehaviorEnabled && !IsUserDefinedType(resolved)) + { + SqlClientEventSource.Log.TryTraceEvent( + "SmiMetaData.Type | ERR | Type '{0}' is not annotated with SqlUserDefinedTypeAttribute and will not be used.", + _udtAssemblyQualifiedName); + + throw SQL.UdtTypeNotUserDefined(_udtAssemblyQualifiedName); + } + + _clrType = resolved; } return _clrType; } } + /// + /// Traces and builds the exception for an assembly the UDT policy + /// refused, so that a denial on this path is observable through event + /// source tracing exactly as it is on the SqlConnection path. + /// + private static Exception UdtAssemblyDenied(AssemblyName asmRef) + { + SqlClientEventSource.Log.TryTraceEvent( + "SmiMetaData.Type | ERR | UDT assembly '{0}' was not loaded because the UDT assembly load policy does not permit it.", + asmRef.Name); + + return SQL.UdtAssemblyNotAllowed(asmRef.Name); + } + + /// + /// Determines whether a resolved type is annotated as a user-defined + /// type, tolerating an attribute that cannot be read. + /// + private static bool IsUserDefinedType(Type type) + { + try + { + return SqlUdtInfo.TryGetFromType(type) != null; + } + catch (Exception e) when (ADP.IsCatchableExceptionType(e)) + { + return false; + } + } + internal bool IsMultiValued => _isMultiValued; // Returns read-only list of field metadata diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs index 3c8da27e92..a1530713fe 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlConnection.cs @@ -3047,32 +3047,25 @@ private Assembly ResolveTypeAssembly(AssemblyName asmRef, bool throwOnError) // pins the identity (version and public key token) of the built-in // SQL CLR types assembly, so that the built-in exemption cannot be // satisfied by a same-named assembly that happens to sit on the - // probing path. - if (!UdtAssemblyPolicy.TryResolve(asmRef, TypeSystemAssemblyVersion, out Assembly alreadyLoaded)) + // probing path. The policy performs the load itself so that the + // identity of whatever the loader returns is verified; on .NET the + // loader ignores the public key token in the reference, so pinning + // it above is not by itself an enforcement boundary. + try { - SqlClientEventSource.Log.TryTraceEvent("SqlConnection.ResolveTypeAssembly | ERR | UDT assembly '{0}' was not loaded because the UDT assembly load policy does not permit it.", asmRef.Name); - - if (throwOnError) + if (!UdtAssemblyPolicy.TryLoad(asmRef, TypeSystemAssemblyVersion, out Assembly resolved)) { - throw SQL.UdtAssemblyNotAllowed(asmRef.Name); - } + SqlClientEventSource.Log.TryTraceEvent("SqlConnection.ResolveTypeAssembly | ERR | UDT assembly '{0}' was not loaded because the UDT assembly load policy does not permit it.", asmRef.Name); - return null; - } + if (throwOnError) + { + throw SQL.UdtAssemblyNotAllowed(asmRef.Name); + } - // The policy permitted the reference because the process had already - // loaded an assembly of that simple name. Use that instance rather - // than binding the server-supplied version and public key token, - // which could otherwise resolve to a different assembly and cause - // the new load this policy exists to prevent. - if (alreadyLoaded != null) - { - return alreadyLoaded; - } + return null; + } - try - { - return Assembly.Load(asmRef); + return resolved; } catch (Exception e) { diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/UdtAssemblyPolicy.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/UdtAssemblyPolicy.cs index 57f1869b8e..90efbd63dd 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/UdtAssemblyPolicy.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/UdtAssemblyPolicy.cs @@ -4,7 +4,11 @@ using System; using System.Collections.Generic; +using System.Globalization; using System.Reflection; +#if NET +using System.Runtime.Loader; +#endif using Microsoft.Data.Common; using Microsoft.Data.SqlClient.Internal; @@ -83,6 +87,15 @@ internal static class UdtAssemblyPolicy private static readonly byte[] s_sqlServerTypesPublicKeyToken = { 0x89, 0x84, 0x5d, 0xcd, 0x80, 0x80, 0xcc, 0x91 }; + #if NET + /// + /// The load context that the driver's own + /// calls resolve into, which is the one that loaded the driver itself. + /// + private static readonly AssemblyLoadContext? s_driverLoadContext = + AssemblyLoadContext.GetLoadContext(typeof(UdtAssemblyPolicy).Assembly); + #endif + #endregion #region Fields @@ -110,11 +123,18 @@ internal static class UdtAssemblyPolicy /// a server name a loaded simple name with a different identity and thereby /// still trigger a new load, which is exactly what this tier must not do. /// + /// The reference is weak. A strong one would keep every assembly in the + /// map alive for the life of the process, which would prevent a collectible + /// AssemblyLoadContext from ever unloading: a server could force the + /// map to be built with a single denied UDT and thereby pin unrelated + /// plugin assemblies. A dead entry simply drops out and the reference is + /// re-evaluated as if the assembly had never been loaded. + /// /// When several assemblies share a simple name, the first one seen wins. /// All of them are already in the process, so the choice cannot widen the /// policy; at worst the subsequent type lookup fails. /// - private static Dictionary? s_loadedAssemblies; + private static Dictionary>? s_loadedAssemblies; /// /// The raw allow list string that was parsed @@ -152,14 +172,18 @@ internal static bool IsSqlServerTypesAssembly(AssemblyName asmRef) => /// /// Decides whether the driver may load the assembly named by - /// , pinning the identity of the built-in SQL - /// Server CLR types assembly as a side effect when that is what it names. + /// and, when it may, produces the assembly. /// - /// Pinning and the decision are deliberately performed by a single call so - /// that it is not possible to consult the policy without also pinning: the - /// built-in exemption is granted on the simple name alone, so an unpinned - /// reference would let an unsigned assembly that merely borrows the name - /// satisfy it. + /// The decision and the load are deliberately performed by a single call. + /// Deciding separately from loading was unsafe: on .NET the loader ignores + /// the public key token in an , so a caller that + /// consulted the policy and then called + /// itself could still be handed a same-named assembly with the wrong + /// identity. Pinning the reference is therefore not an enforcement + /// boundary; verifying what actually came back is, and doing both here + /// means no caller can forget. This mirrors what + /// SqlAuthenticationProviderManager does for the Azure extension + /// assembly. /// /// /// The server-supplied assembly reference. It is normalized in place when @@ -168,18 +192,14 @@ internal static bool IsSqlServerTypesAssembly(AssemblyName asmRef) => /// /// The type system assembly version negotiated for the connection, used to /// pin the version of the built-in SQL Server CLR types assembly. Null - /// when no connection context is available, in which case only the public - /// key token is pinned and the loader picks the version. + /// when no connection context is available, in which case the version is + /// left to the loader and only the culture and public key token are pinned. /// /// - /// On a permitted result, the assembly the caller must use, or null when - /// the caller is to load itself. A non-null value - /// means the process had already loaded an assembly with this simple name - /// and the caller must use that instance rather than binding the - /// server-supplied identity. + /// The assembly the caller must use, or null when the policy refused. /// /// True when the assembly may be used. - internal static bool TryResolve( + internal static bool TryLoad( AssemblyName asmRef, Version? typeSystemAssemblyVersion, out Assembly? assembly) @@ -188,9 +208,115 @@ internal static bool TryResolve( if (LegacyBehaviorEnabled) { + assembly = Assembly.Load(asmRef); + return true; + } + + if (!TryDecide(asmRef, typeSystemAssemblyVersion, out Decision decision)) + { + return false; + } + + // The process already holds this assembly, so there is nothing to load + // and nothing to verify: the instance is the one the application itself + // brought in, whatever identity the server claimed for it. + if (decision.Loaded is not null) + { + assembly = decision.Loaded; return true; } + Assembly loaded = Assembly.Load(asmRef); + + if (loaded is null) + { + return false; + } + + if (!SatisfiesRequiredIdentity(loaded, decision)) + { + SqlClientEventSource.Log.TryTraceEvent( + "UdtAssemblyPolicy.TryLoad | ERR | Assembly '{0}' was loaded but has an unexpected identity '{1}' and will not be used.", + asmRef.Name, + loaded.FullName); + + return false; + } + + assembly = loaded; + + return true; + } + + /// + /// The outcome of evaluating the policy: whether the reference is permitted + /// and, if so, what must be true of the assembly that the loader returns. + /// + private readonly struct Decision + { + /// + /// The assembly the process already holds, when the reference was + /// permitted on that basis. Null when the caller must load it. + /// + internal Assembly? Loaded { get; init; } + + /// + /// The public key token the loaded assembly must carry, or null when + /// the basis for permitting it placed no constraint on the token. + /// + internal byte[]? RequiredPublicKeyToken { get; init; } + + /// + /// True when the loaded assembly must carry no public key token at all, + /// because the allow list entry explicitly said PublicKeyToken=null. + /// + internal bool RequireUnsigned { get; init; } + } + + /// + /// Evaluates the policy without loading anything. This is the decision + /// half of , exposed so that tests can observe the + /// decision for assembly names that do not exist on disk. Production code + /// must use , because a decision alone does not + /// enforce the identity of what the loader returns. + /// + /// + /// The instance the process already holds, when that was the basis for + /// permitting the reference; otherwise null. + /// + /// The server-supplied assembly reference. + /// + /// The type system assembly version negotiated for the connection, or null. + /// + internal static bool IsPermitted( + AssemblyName asmRef, + Version? typeSystemAssemblyVersion, + out Assembly? alreadyLoaded) + { + if (LegacyBehaviorEnabled) + { + alreadyLoaded = null; + + return true; + } + + bool permitted = TryDecide(asmRef, typeSystemAssemblyVersion, out Decision decision); + + alreadyLoaded = decision.Loaded; + + return permitted; + } + + /// + /// Evaluates the policy without loading anything. + /// + private static bool TryDecide( + AssemblyName asmRef, + Version? typeSystemAssemblyVersion, + out Decision decision) + { + decision = default; + string? simpleName = asmRef.Name; if (string.IsNullOrEmpty(simpleName)) { @@ -203,21 +329,75 @@ internal static bool TryResolve( if (IsSqlServerTypesAssembly(asmRef)) { PinSqlServerTypesIdentity(asmRef, typeSystemAssemblyVersion); + + decision = new Decision { RequiredPublicKeyToken = s_sqlServerTypesPublicKeyToken }; + return true; } // The allow list is the application stating which assemblies it is // willing to have loaded on a server's say-so, so the reference is - // handed to the loader as given. - if (MatchesAllowList(asmRef)) + // handed to the loader as given. Whatever identity the matched entry + // specified is carried forward and enforced against the result. + if (TryMatchAllowList(asmRef, out AssemblyName? matched)) { + byte[]? allowedToken = matched!.GetPublicKeyToken(); + + decision = new Decision + { + // A null token means the entry did not mention one, an empty + // token means it explicitly required an unsigned assembly. + RequiredPublicKeyToken = allowedToken is { Length: > 0 } ? allowedToken : null, + RequireUnsigned = allowedToken is { Length: 0 }, + }; + return true; } // Otherwise the only remaining basis is that the process already holds // an assembly by this simple name, in which case that instance is used // and the server-supplied identity is discarded. - return TryGetLoadedAssembly(simpleName!, out assembly); + if (TryGetLoadedAssembly(simpleName!, out Assembly? loaded)) + { + decision = new Decision { Loaded = loaded }; + + return true; + } + + return false; + } + + /// + /// Verifies that an assembly the loader returned actually carries the + /// identity that the policy required of it. + /// + private static bool SatisfiesRequiredIdentity(Assembly loaded, Decision decision) + { + if (decision.RequiredPublicKeyToken is null && !decision.RequireUnsigned) + { + return true; + } + + byte[]? actualToken; + + try + { + actualToken = loaded.GetName().GetPublicKeyToken(); + } + catch (Exception e) when (ADP.IsCatchableExceptionType(e)) + { + // If the identity cannot be read it cannot be confirmed, so the + // assembly is refused. + return false; + } + + if (decision.RequireUnsigned) + { + return actualToken is null || actualToken.Length == 0; + } + + return actualToken is not null && + actualToken.AsSpan().SequenceEqual(decision.RequiredPublicKeyToken.AsSpan()); } /// @@ -231,6 +411,15 @@ internal static bool TryResolve( /// assembly with. Without this, a server that omits the token (or supplies /// a different one) would cause a partial-name bind that an unsigned /// same-named assembly on the probing path could satisfy. + /// + /// The culture is normalized to neutral. The shipped assembly is culture + /// neutral, so leaving the culture server-controlled would let a reference + /// carrying Culture=xx-YY steer the bind towards a satellite-shaped + /// name that the real assembly never uses. + /// + /// Normalizing the reference is necessary but not sufficient, because on + /// .NET the loader ignores the requested token. + /// verifies the identity of whatever the loader actually returns. /// /// The assembly reference to normalize, in place. /// @@ -244,6 +433,7 @@ private static void PinSqlServerTypesIdentity(AssemblyName asmRef, Version? type asmRef.Version = typeSystemAssemblyVersion; } + asmRef.CultureInfo = CultureInfo.InvariantCulture; asmRef.SetPublicKeyToken((byte[])s_sqlServerTypesPublicKeyToken.Clone()); } @@ -266,10 +456,11 @@ internal static void ResetCache() #region Helpers /// - /// Determines whether matches an entry on the - /// application-supplied allow list. + /// Finds the allow list entry that satisfies, if + /// any. The matched entry is returned so that the identity it specified + /// can be enforced against the assembly the loader returns. /// - private static bool MatchesAllowList(AssemblyName asmRef) + private static bool TryMatchAllowList(AssemblyName asmRef, out AssemblyName? matched) { List allowList = GetAllowList(); @@ -277,10 +468,14 @@ private static bool MatchesAllowList(AssemblyName asmRef) { if (Matches(allowList[i], asmRef)) { + matched = allowList[i]; + return true; } } + matched = null; + return false; } @@ -310,21 +505,27 @@ private static bool Matches(AssemblyName allowed, AssemblyName candidate) return false; } + // AssemblyName distinguishes an omitted public key token (null) from an + // explicitly unsigned one (PublicKeyToken=null, which parses to an empty + // array). Treating the latter as "unconstrained" would let an entry + // that deliberately named an unsigned assembly be satisfied by a signed + // one, so the two cases are kept apart. byte[]? allowedToken = allowed.GetPublicKeyToken(); - if (allowedToken is { Length: > 0 }) + + if (allowedToken is not null) { byte[]? candidateToken = candidate.GetPublicKeyToken(); - if (candidateToken is null || candidateToken.Length != allowedToken.Length) + + if (allowedToken.Length == 0) { - return false; + // The entry requires an unsigned assembly. + return candidateToken is null || candidateToken.Length == 0; } - for (int i = 0; i < allowedToken.Length; i++) + if (candidateToken is null || + !candidateToken.AsSpan().SequenceEqual(allowedToken.AsSpan())) { - if (allowedToken[i] != candidateToken[i]) - { - return false; - } + return false; } } @@ -390,7 +591,27 @@ private static bool TryGetLoadedAssembly(string simpleName, out Assembly? assemb { lock (s_lock) { - return GetLoadedAssemblies().TryGetValue(simpleName, out assembly); + Dictionary> loaded = GetLoadedAssemblies(); + + if (loaded.TryGetValue(simpleName, out WeakReference? reference) && + reference.TryGetTarget(out Assembly? target)) + { + assembly = target; + + return true; + } + + // The assembly has been collected, which means its load context was + // unloaded. Drop the entry so the name is no longer permitted on + // the strength of a load that no longer exists. + if (reference is not null) + { + loaded.Remove(simpleName); + } + + assembly = null; + + return false; } } @@ -402,7 +623,7 @@ private static bool TryGetLoadedAssembly(string simpleName, out Assembly? assemb /// /// Callers must hold . /// - private static Dictionary GetLoadedAssemblies() + private static Dictionary> GetLoadedAssemblies() { EnsureAssemblyLoadHandlerAttached(); @@ -411,7 +632,7 @@ private static Dictionary GetLoadedAssemblies() return s_loadedAssemblies; } - Dictionary loaded = new(StringComparer.OrdinalIgnoreCase); + Dictionary> loaded = new(StringComparer.OrdinalIgnoreCase); foreach (Assembly assembly in AppDomain.CurrentDomain.GetAssemblies()) { @@ -427,7 +648,7 @@ private static Dictionary GetLoadedAssemblies() /// Records under its simple name, keeping the /// first assembly seen for a given name. /// - private static void Remember(Dictionary loaded, Assembly assembly) + private static void Remember(Dictionary> loaded, Assembly assembly) { if (assembly.IsDynamic) { @@ -436,13 +657,24 @@ private static void Remember(Dictionary loaded, Assembly assem return; } + if (!IsInDriverLoadContext(assembly)) + { + // Only assemblies in the load context that Assembly.Load would + // resolve into can satisfy this tier. Recording one from another + // context would permit a reference that the loader would then + // resolve to a different assembly, or to none at all. + return; + } + try { string? name = assembly.GetName().Name; - if (!string.IsNullOrEmpty(name) && !loaded.ContainsKey(name!)) + if (!string.IsNullOrEmpty(name) && + (!loaded.TryGetValue(name!, out WeakReference? existing) || + !existing.TryGetTarget(out _))) { - loaded.Add(name!, assembly); + loaded[name!] = new WeakReference(assembly); } } catch (Exception e) when (ADP.IsCatchableExceptionType(e)) @@ -455,6 +687,29 @@ private static void Remember(Dictionary loaded, Assembly assem } } + /// + /// Determines whether an assembly lives in the load context that the + /// driver's own calls resolve + /// into. + /// + /// + /// On .NET, resolves against the + /// load context of the calling assembly, which is the driver's. Assemblies + /// held by other contexts are therefore not reachable by name from here, + /// and an application that loads its UDT assembly into a separate + /// collectible context must name it on the allow list. + /// + private static bool IsInDriverLoadContext(Assembly assembly) + { + #if NET + return AssemblyLoadContext.GetLoadContext(assembly) == s_driverLoadContext; + #else + // .NET Framework has a single load context per AppDomain for this + // purpose, so every loaded assembly qualifies. + return true; + #endif + } + /// /// Attaches the assembly load handler that keeps the cached map of loaded /// assemblies current, if it has not been attached already. diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyLoadHardeningTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyLoadHardeningTest.cs index 423d86aead..41ddd3eab9 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyLoadHardeningTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyLoadHardeningTest.cs @@ -18,11 +18,21 @@ namespace Microsoft.Data.SqlClient.UnitTests; /// /// /// Before the fix, handed any server-supplied -/// assembly name straight to , which -/// runs the target assembly's module initializer, and then invoked a static -/// member on the resolved type without checking that it was a user-defined type -/// at all, which runs the type's static constructor. These tests assert that -/// neither happens. +/// assembly name straight to , and then +/// invoked a static member on the resolved type without checking that it was a +/// user-defined type at all. +/// +/// The two steps are not equally dangerous, and the tests below are written to +/// reflect that. On CoreCLR the load itself runs nothing from the target +/// assembly: neither , nor resolving a +/// type from it, nor reading that type's custom attributes executes any of its +/// code. What the load grants is the ability to bring a server-chosen file into +/// the process. Code runs at the later static member invocation, which is what +/// pulls in the module initializer and the type's static constructor. +/// +/// So these tests assert two distinct things: that a denied assembly is never +/// loaded at all, and that a type which is not annotated as a user-defined type +/// never reaches the invocation that would run its code. /// [Collection(AppContextSwitchTestCollection.Name)] public class UdtAssemblyLoadHardeningTest @@ -48,6 +58,15 @@ public class UdtAssemblyLoadHardeningTest /// reaches the assembly loader, and reports a policy failure rather than /// silently succeeding. /// + /// + /// The assertion on the exception matters as much as the one on the + /// recorder. Because the hostile assembly does not exist on disk, a driver + /// with no policy at all would also fail to load it and would also raise no + /// AssemblyLoad event, so "nothing was loaded" alone would pass against the + /// vulnerable implementation too. Requiring the specific policy denial + /// distinguishes "the policy refused to ask" from "the loader looked and did + /// not find it". + /// [Fact] public void CheckGetExtendedUDTInfo_UnknownAssembly_IsNeverLoaded() { @@ -63,6 +82,36 @@ public void CheckGetExtendedUDTInfo_UnknownAssembly_IsNeverLoaded() Assert.NotNull(exception); Assert.Null(metaData.udt.Type); Assert.DoesNotContain("Contoso.Evil", recorder.LoadedNames); + + // The failure must be the policy's denial, not a loader miss. + Assert.IsType(exception); + Assert.Contains("Contoso.Evil", exception!.Message); + Assert.Contains(UdtAssemblyPolicy.AllowListAppContextDataName, exception.Message); + } + + /// + /// Verifies that a permitted UDT still resolves, so that the denial tests + /// above are not passing simply because resolution never works. + /// + /// + /// This is the positive control for the test above. Without it, a change + /// that broke UDT resolution outright would leave every "was refused" + /// assertion passing for the wrong reason. + /// + [Fact] + public void CheckGetExtendedUDTInfo_LoadedUserDefinedType_Resolves() + { + AssemblyName self = typeof(UdtAssemblyLoadHardeningTest).Assembly.GetName(); + string qualifiedName = $"{typeof(AUserDefinedType).FullName}, {self.Name}"; + + using PolicyScope scope = new(); + + SqlConnection connection = new(ConnectionString); + SqlMetaDataPriv metaData = CreateUdtMetaData(qualifiedName); + + connection.CheckGetExtendedUDTInfo(metaData, fThrow: true); + + Assert.Equal(typeof(AUserDefinedType), metaData.udt.Type); } /// diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs index 51844d9eda..14bfa7cbc1 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs @@ -35,7 +35,7 @@ public class UdtAssemblyPolicyTest /// tests care only whether the reference was permitted. /// private static bool IsAllowed(AssemblyName asmRef, Version? typeSystemAssemblyVersion) => - UdtAssemblyPolicy.TryResolve(asmRef, typeSystemAssemblyVersion, out _); + UdtAssemblyPolicy.IsPermitted(asmRef, typeSystemAssemblyVersion, out _); #region Scope @@ -61,6 +61,15 @@ public PolicyScope(bool legacy = false) SetAllowList(null); } + /// + /// Enters an enforcing scope with the given allow list already applied. + /// + public PolicyScope(string? allowList) + : this(legacy: false) + { + SetAllowList(allowList); + } + public static void SetAllowList(string? value) { AppDomain.CurrentDomain.SetData( @@ -238,7 +247,7 @@ public void Resolve_LoadedAssembly_IsPermitted() using PolicyScope scope = new(); - Assert.True(UdtAssemblyPolicy.TryResolve( + Assert.True(UdtAssemblyPolicy.IsPermitted( new AssemblyName(self.GetName().Name!), null, out Assembly? resolved)); Assert.Same(self, resolved); } @@ -265,7 +274,7 @@ public void Resolve_LoadedAssembly_IgnoresServerSuppliedIdentity() AssemblyName hostile = new( $"{simpleName}, Version=9.9.9.9, Culture=neutral, PublicKeyToken=0123456789abcdef"); - Assert.True(UdtAssemblyPolicy.TryResolve(hostile, null, out Assembly? resolved)); + Assert.True(UdtAssemblyPolicy.IsPermitted(hostile, null, out Assembly? resolved)); Assert.Same(self, resolved); Assert.NotEqual(new Version(9, 9, 9, 9), resolved!.GetName().Version); } @@ -289,21 +298,26 @@ public void Resolve_ReferencedButUnloadedAssembly_IsDenied() .Select(a => a.GetName().Name!), StringComparer.OrdinalIgnoreCase); - AssemblyName? referencedNotLoaded = typeof(UdtAssemblyPolicyTest).Assembly + // The driver references a number of assemblies that a unit test run + // never causes to be loaded (the identity and Azure stacks, for + // example), which makes this a far more reliable source of a + // referenced-but-unloaded assembly than the test assembly's own + // references. + AssemblyName? referencedNotLoaded = typeof(SqlConnection).Assembly .GetReferencedAssemblies() .FirstOrDefault(r => !loaded.Contains(r.Name!)); - if (referencedNotLoaded is null) - { - // Every referenced assembly happens to be loaded in this run, so - // there is nothing here to distinguish. The companion test - // Resolve_UnknownAssembly_IsDenied covers the general deny path. - return; - } + // Assert the precondition rather than returning quietly. If every + // referenced assembly is loaded then this test proves nothing, and that + // should be visible rather than counted as a pass. + Assert.True( + referencedNotLoaded is not null, + "Expected the driver to reference at least one assembly that is not loaded, " + + "so that the referenced-is-not-trusted rule can be exercised."); using PolicyScope scope = new(); - Assert.False(IsAllowed(new AssemblyName(referencedNotLoaded.Name!), null)); + Assert.False(IsAllowed(new AssemblyName(referencedNotLoaded!.Name!), null)); } #endregion @@ -415,6 +429,132 @@ public void IsAllowed_EmptySimpleName_IsDenied() #endregion + #region Identity enforcement + + /// + /// Verifies that an allow list entry which explicitly requires an unsigned + /// assembly (PublicKeyToken=null) is not satisfied by a signed one. + /// + /// + /// AssemblyName represents an omitted public key token as null and an + /// explicit PublicKeyToken=null as a zero-length array. Treating the + /// two alike would silently widen an entry that was written to pin an + /// unsigned assembly into one that accepts any identity. + /// + [Fact] + public void AllowList_ExplicitNullToken_DoesNotPermitSignedAssembly() + { + using PolicyScope scope = new($"{UnknownAssemblyName}, PublicKeyToken=null"); + + // The entry is satisfied by an unsigned candidate. + Assert.True(IsAllowed(new AssemblyName(UnknownAssemblyName), null)); + + // ... but not by one that carries a strong name token. + AssemblyName signed = new(UnknownAssemblyName); + signed.SetPublicKeyToken(new byte[] { 0xb0, 0x3f, 0x5f, 0x7f, 0x11, 0xd5, 0x0a, 0x3a }); + + Assert.False(IsAllowed(signed, null)); + } + + /// + /// Verifies that an allow list entry which omits the public key token still + /// permits any identity, which is the documented simple-name behavior. + /// + [Fact] + public void AllowList_OmittedToken_PermitsAnyIdentity() + { + using PolicyScope scope = new(UnknownAssemblyName); + + AssemblyName signed = new(UnknownAssemblyName); + signed.SetPublicKeyToken(new byte[] { 0xb0, 0x3f, 0x5f, 0x7f, 0x11, 0xd5, 0x0a, 0x3a }); + + Assert.True(IsAllowed(signed, null)); + Assert.True(IsAllowed(new AssemblyName(UnknownAssemblyName), null)); + } + + /// + /// Verifies that the culture of the built-in SQL CLR types assembly is + /// pinned to neutral rather than left under the server's control. + /// + /// + /// The shipped assembly is culture neutral. Leaving a server-supplied + /// Culture= in place would let the reference steer the bind towards a + /// name the real assembly never uses. + /// + [Fact] + public void SqlServerTypes_PinsCultureToNeutral() + { + using PolicyScope scope = new(); + + AssemblyName asmRef = new( + $"{UdtAssemblyPolicy.SqlServerTypesAssemblyName}, Version=14.0.0.0, Culture=en-US"); + + Assert.True(IsAllowed(asmRef, new Version(14, 0, 0, 0))); + + // AssemblyName represents the neutral culture as the empty string. + Assert.Equal(string.Empty, asmRef.CultureName); + } + + /// + /// Verifies that an assembly the loader returns with an identity that does + /// not match what the policy required is refused. + /// + /// + /// On .NET the loader ignores the public key token in an AssemblyName, so + /// pinning the reference is not an enforcement boundary on its own. This + /// drives the real load path to confirm that the returned assembly's actual + /// identity is what gates the result. The test assembly is unsigned, so + /// allow-listing it under an explicit strong name token must fail even + /// though the assembly itself resolves. + /// + [Fact] + public void TryLoad_LoadedAssemblyWithWrongToken_IsRefused() + { + AssemblyName self = typeof(UdtAssemblyPolicyTest).Assembly.GetName(); + + // Guard: the reasoning below only holds for an unsigned test assembly. + byte[]? actualToken = self.GetPublicKeyToken(); + if (actualToken is { Length: > 0 }) + { + return; + } + + const string StrongToken = "b03f5f7f11d50a3a"; + + using PolicyScope scope = new($"{self.Name}, PublicKeyToken={StrongToken}"); + + // The server names the assembly with the very token the allow list + // requires, so the entry matches and the load proceeds. This is the + // case that matters: on .NET the loader ignores the requested token and + // hands back the real, unsigned assembly of that simple name, so + // without a post-load check the wrong assembly would be accepted on the + // strength of a token it does not actually carry. + AssemblyName serverSupplied = new(self.Name!); + serverSupplied.SetPublicKeyToken(new byte[] { 0xb0, 0x3f, 0x5f, 0x7f, 0x11, 0xd5, 0x0a, 0x3a }); + + Assert.False(UdtAssemblyPolicy.TryLoad(serverSupplied, null, out Assembly? loaded)); + Assert.Null(loaded); + } + + /// + /// Verifies that a permitted, genuinely loadable assembly is returned by the + /// load path, so that the identity checks above are not simply refusing + /// everything. + /// + [Fact] + public void TryLoad_AllowListedAssembly_IsLoaded() + { + AssemblyName self = typeof(UdtAssemblyPolicyTest).Assembly.GetName(); + + using PolicyScope scope = new(self.Name!); + + Assert.True(UdtAssemblyPolicy.TryLoad(new AssemblyName(self.Name!), null, out Assembly? loaded)); + Assert.NotNull(loaded); + Assert.Equal(self.Name, loaded!.GetName().Name); + } + + #endregion + #region Helpers private static string? ToHex(byte[]? bytes) From 61f0afddca241b6232aa9133951c2f6333551a8d Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Wed, 23 Sep 2026 14:19:46 -0700 Subject: [PATCH 07/10] Document the bulk copy and TVP paths that resolve UDT types Review raised SqlCommandBuilder and several SqlBulkCopy shapes as possibly affected. Most are not, because they move UDT bytes without interpreting them, but two do materialize the type and were missing from the compatibility notes: - SqlBulkCopy from a SqlDataReader between UDT columns, which reads each value to test it for INullable. - Table-valued parameters sourced from a SqlDataReader, which build SMI metadata with throwing enabled. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60806d12-bdac-413c-bf83-f7c12e3cc12c --- .github/instructions/features.instructions.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/.github/instructions/features.instructions.md b/.github/instructions/features.instructions.md index 3888f4cf92..8f260a9aef 100644 --- a/.github/instructions/features.instructions.md +++ b/.github/instructions/features.instructions.md @@ -369,6 +369,15 @@ The symptom depends on the API: | `reader[i]`, `GetValue`, UDT output parameters | `TypeLoadException` naming the assembly and the allow list | | `GetFieldType`, `GetSchemaTable`, `GetColumnSchema` | Returns `null` for the UDT column's type rather than throwing | +Two less obvious paths also materialize the type and are therefore affected: + +- `SqlBulkCopy` **from a `SqlDataReader`** between UDT columns. The copy reads + each value so it can test it for `INullable`, which materializes the UDT. + Copying *to* a UDT column from a `DataTable`, or to `varbinary(max)`, does not + resolve the type and is unaffected. +- Table-valued parameters sourced from a `SqlDataReader`, which build SMI + metadata and resolve the UDT type with throwing enabled. + The exception is a `TypeLoadException` and is not wrapped in a `SqlException`, which matches how the driver already reports a UDT type it cannot resolve. From 36b209253733af7e9a555fac9670eeb2ab0bc18a Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Wed, 23 Sep 2026 14:52:09 -0700 Subject: [PATCH 08/10] Validate every constrained identity component after load Second round of review feedback on the UDT assembly load policy. Validate the whole identity, not just the token. The decision carried only the required public key token, so a version or culture the policy relied on was never confirmed against what the loader returned. Measured on net9.0: requesting System.Private.CoreLib at version 109.0.0.0 returns 9.0.0.0, which the old check accepted. Binding redirects on .NET Framework produce the same effect. Decision now carries the required version and culture alongside the token, and each is enforced after the load. Stop dependencies inheriting the already-loaded permission. The AssemblyLoad handler recorded every assembly that arrived after the map was built, including those pulled in by the policy's own Assembly.Load for a permitted assembly. A server could then name one of those dependencies and have it permitted as "already loaded", which is exactly the transitive trust the policy documents as denied. Policy-triggered loads are now marked with a counted thread-static guard and excluded. Cover the SMI path. The attribute gate and resolver added there had no tests; all end-to-end coverage went through CheckGetExtendedUDTInfo. Added denied-assembly, bare non-UDT, and valid-UDT cases against SmiMetaData.Type. Fix two defects in the identity test: - It returned without asserting when the test assembly is strong-name signed, so official signed builds silently lost the coverage. It now uses a framework assembly and does not depend on signing at all. - It asserted the .NET shape only. On .NET Framework the loader enforces the strong name during binding and throws rather than returning the wrong assembly, so it would have failed on net462. Both shapes are the same refusal and both are now accepted, which also keeps the post-load check from regressing unnoticed on .NET. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60806d12-bdac-413c-bf83-f7c12e3cc12c --- .github/instructions/features.instructions.md | 22 +-- .../Data/SqlClient/UdtAssemblyPolicy.cs | 130 ++++++++++++++++-- .../SqlClient/UdtAssemblyLoadHardeningTest.cs | 91 ++++++++++++ .../Data/SqlClient/UdtAssemblyPolicyTest.cs | 107 ++++++++++---- 4 files changed, 308 insertions(+), 42 deletions(-) diff --git a/.github/instructions/features.instructions.md b/.github/instructions/features.instructions.md index 8f260a9aef..213b0dc760 100644 --- a/.github/instructions/features.instructions.md +++ b/.github/instructions/features.instructions.md @@ -277,12 +277,12 @@ genuinely new load — precisely what this policy keeps under the application's control rather than the server's. Normalizing the reference is necessary but not sufficient. On .NET the loader -**ignores** the public key token in an `AssemblyName`, so pinning it does not by -itself prevent a same-named assembly with a different identity from being -returned. The driver therefore verifies the identity of the assembly the loader -actually hands back, and refuses it if it does not carry the required token. -This mirrors what the driver already does for the Azure authentication extension -assembly. +**ignores** the public key token in an `AssemblyName`, and can satisfy a request +with a different version than the one asked for, so pinning the reference does +not by itself determine what arrives. The driver therefore verifies the identity +of the assembly the loader actually hands back against every component the +decision relied on, and refuses it on any mismatch. This mirrors what the driver +already does for the Azure authentication extension assembly. On .NET, the already-loaded tier is scoped to the `AssemblyLoadContext` that loaded the driver, since that is the context its `Assembly.Load` calls resolve @@ -344,10 +344,12 @@ confined to types that were written to be deserialized from SQL Server, but it is a genuine widening and is called out here deliberately. Relatedly, the map of loaded assemblies is built once and then maintained -incrementally. That is not only a performance choice: rebuilding it on demand -would let an assembly that was pulled in as a *dependency* of a permitted -assembly silently inherit that permission. Building it once keeps the tier -anchored to what the application had already loaded. +incrementally, and loads that the policy itself triggers are excluded from it. +Neither is merely a performance choice. Rebuilding the map on demand, or +recording the dependencies that arrive alongside a permitted assembly, would let +an assembly that was pulled in as a *dependency* of a permitted assembly +silently inherit that permission. Both keep the tier anchored to what the +application loaded of its own accord. #### Compatibility impact diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/UdtAssemblyPolicy.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/UdtAssemblyPolicy.cs index 90efbd63dd..0ad2aa2cc6 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/UdtAssemblyPolicy.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/UdtAssemblyPolicy.cs @@ -147,6 +147,23 @@ internal static class UdtAssemblyPolicy /// private static List? s_allowList; + /// + /// Non-zero on a thread that is inside the policy's own call to + /// . + /// + /// Loading a permitted assembly also loads whatever that assembly needs, and + /// those dependency loads raise just as + /// an application-initiated load does. Recording them would let a server + /// name a dependency afterwards and have it permitted as "already loaded", + /// which is precisely the transitive trust this policy refuses: an assembly + /// that is merely reachable from a permitted one is documented as denied. + /// + /// Marking the window lets the handler tell the two apart, so permission + /// cannot spread from an allow-listed assembly to its closure. + /// + [ThreadStatic] + private static int t_insidePolicyLoad; + #endregion #region Properties @@ -226,7 +243,7 @@ internal static bool TryLoad( return true; } - Assembly loaded = Assembly.Load(asmRef); + Assembly loaded = LoadWithoutTrustingDependencies(asmRef); if (loaded is null) { @@ -271,6 +288,51 @@ private readonly struct Decision /// because the allow list entry explicitly said PublicKeyToken=null. /// internal bool RequireUnsigned { get; init; } + + /// + /// The version the loaded assembly must carry, or null when the basis + /// for permitting it placed no constraint on the version. + /// + /// + /// A binding redirect on .NET Framework, or a custom resolver on .NET, + /// can return a different version than the one requested, so a version + /// the policy relied on has to be confirmed after the load rather than + /// assumed from the reference. + /// + internal Version? RequiredVersion { get; init; } + + /// + /// The culture name the loaded assembly must carry, or null when the + /// basis for permitting it placed no constraint on the culture. The + /// empty string means the neutral culture. + /// + internal string? RequiredCultureName { get; init; } + } + + /// + /// Loads an assembly the policy has permitted, without letting the + /// dependencies that load alongside it inherit that permission. + /// + /// + /// The handler cannot otherwise tell a + /// dependency pulled in by this call from an assembly the application + /// loaded itself, and recording the former would quietly grant the + /// already-loaded permission to the whole reference closure. The guard is + /// per thread and counted, so a nested load (a resolver that loads + /// something in order to satisfy this one) stays covered. + /// + private static Assembly LoadWithoutTrustingDependencies(AssemblyName asmRef) + { + t_insidePolicyLoad++; + + try + { + return Assembly.Load(asmRef); + } + finally + { + t_insidePolicyLoad--; + } } /// @@ -330,7 +392,15 @@ private static bool TryDecide( { PinSqlServerTypesIdentity(asmRef, typeSystemAssemblyVersion); - decision = new Decision { RequiredPublicKeyToken = s_sqlServerTypesPublicKeyToken }; + decision = new Decision + { + RequiredPublicKeyToken = s_sqlServerTypesPublicKeyToken, + // The culture was just pinned to neutral, so require that back. + RequiredCultureName = string.Empty, + // The version is only constrained when the connection supplied + // one; otherwise the loader is free to pick. + RequiredVersion = typeSystemAssemblyVersion, + }; return true; } @@ -349,6 +419,11 @@ private static bool TryDecide( // token means it explicitly required an unsigned assembly. RequiredPublicKeyToken = allowedToken is { Length: > 0 } ? allowedToken : null, RequireUnsigned = allowedToken is { Length: 0 }, + // Only components the entry actually specified are enforced, so + // that a simple-name entry stays as permissive after the load as + // it was during matching. + RequiredVersion = matched.Version, + RequiredCultureName = matched.CultureName, }; return true; @@ -373,16 +448,19 @@ private static bool TryDecide( /// private static bool SatisfiesRequiredIdentity(Assembly loaded, Decision decision) { - if (decision.RequiredPublicKeyToken is null && !decision.RequireUnsigned) + if (decision.RequiredPublicKeyToken is null && + !decision.RequireUnsigned && + decision.RequiredVersion is null && + decision.RequiredCultureName is null) { return true; } - byte[]? actualToken; + AssemblyName actual; try { - actualToken = loaded.GetName().GetPublicKeyToken(); + actual = loaded.GetName(); } catch (Exception e) when (ADP.IsCatchableExceptionType(e)) { @@ -391,13 +469,41 @@ private static bool SatisfiesRequiredIdentity(Assembly loaded, Decision decision return false; } + byte[]? actualToken = actual.GetPublicKeyToken(); + if (decision.RequireUnsigned) { - return actualToken is null || actualToken.Length == 0; + if (actualToken is { Length: > 0 }) + { + return false; + } + } + else if (decision.RequiredPublicKeyToken is not null && + (actualToken is null || + !actualToken.AsSpan().SequenceEqual(decision.RequiredPublicKeyToken.AsSpan()))) + { + return false; } - return actualToken is not null && - actualToken.AsSpan().SequenceEqual(decision.RequiredPublicKeyToken.AsSpan()); + // A binding redirect or a custom resolver can satisfy the request with a + // different version or culture than the one asked for, so any component + // the decision relied on is confirmed against what actually arrived. + if (decision.RequiredVersion is not null && + !decision.RequiredVersion.Equals(actual.Version)) + { + return false; + } + + if (decision.RequiredCultureName is not null && + !string.Equals( + decision.RequiredCultureName, + actual.CultureName ?? string.Empty, + StringComparison.OrdinalIgnoreCase)) + { + return false; + } + + return true; } /// @@ -726,6 +832,14 @@ private static void EnsureAssemblyLoadHandlerAttached() AppDomain.CurrentDomain.AssemblyLoad += static (_, args) => { + // A load the policy itself triggered brings in the permitted + // assembly's dependencies. Those must not enter the already-loaded + // tier, or permission would spread along the reference closure. + if (t_insidePolicyLoad > 0) + { + return; + } + lock (s_lock) { // Nothing to update if the map has not been built yet; it will diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyLoadHardeningTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyLoadHardeningTest.cs index 41ddd3eab9..56845dac42 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyLoadHardeningTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyLoadHardeningTest.cs @@ -4,7 +4,10 @@ using System; using System.Collections.Generic; +using System.Data; +using System.Data.SqlTypes; using System.Reflection; +using Microsoft.Data.SqlClient.Server; using Microsoft.Data.SqlClient.Tests.Common; using Microsoft.SqlServer.Server; using Xunit; @@ -278,8 +281,96 @@ public void CheckGetExtendedUDTInfo_TypeNameWithoutAssembly_DoesNotThrowWhenNotR #endregion + #region SMI metadata path + + /// + /// Verifies that the SMI resolution path refuses an assembly the policy does + /// not permit. + /// + /// + /// SmiMetaData.Type is a second live resolution site for + /// server-supplied names, reached through table-valued parameters and + /// SqlDataReader.GetInternalSmiMetaData. It is covered separately + /// from CheckGetExtendedUDTInfo because a regression in one would not + /// be caught by tests of the other. + /// + [Fact] + public void SmiMetaData_UnknownAssembly_IsRefused() + { + using PolicyScope scope = new(); + using AssemblyLoadRecorder recorder = new(); + + SmiMetaData metaData = CreateSmiUdtMetaData(HostileAssemblyQualifiedName); + + Exception exception = Record.Exception(() => _ = metaData.Type); + + Assert.IsType(exception); + Assert.Contains("Contoso.Evil", exception!.Message); + Assert.DoesNotContain("Contoso.Evil", recorder.LoadedNames); + } + + /// + /// Verifies that the SMI path applies the user-defined type gate to a name + /// that carries no assembly part. + /// + /// + /// A bare name never reaches the assembly resolver, so the attribute check + /// is the only thing standing between a server-chosen type and + /// ValueUtilsSmi.NullUdtInstance, which invokes the type's static + /// Null member. + /// + [Fact] + public void SmiMetaData_BareNonUserDefinedTypeName_IsRefused() + { + using PolicyScope scope = new(); + + SmiMetaData metaData = CreateSmiUdtMetaData(typeof(string).FullName!); + + Exception exception = Record.Exception(() => _ = metaData.Type); + + Assert.IsType(exception); + } + + /// + /// Verifies that the SMI path still resolves a genuine user-defined type, so + /// the refusals above are not simply the path being broken. + /// + [Fact] + public void SmiMetaData_LoadedUserDefinedType_Resolves() + { + AssemblyName self = typeof(UdtAssemblyLoadHardeningTest).Assembly.GetName(); + + using PolicyScope scope = new(); + + SmiMetaData metaData = CreateSmiUdtMetaData( + $"{typeof(AUserDefinedType).FullName}, {self.Name}"); + + Assert.Equal(typeof(AUserDefinedType), metaData.Type); + } + + #endregion + #region Helpers + /// + /// Builds SMI metadata for a UDT column whose CLR type is described only by + /// a server-supplied assembly-qualified name, so that reading + /// SmiMetaData.Type exercises the fault-in resolution path. + /// + private static SmiMetaData CreateSmiUdtMetaData(string assemblyQualifiedName) => + new( + dbType: SqlDbType.Udt, + maxLength: SmiMetaData.UnlimitedMaxLengthIndicator, + precision: 0, + scale: 0, + localeId: 0, + compareOptions: SqlCompareOptions.None, + userDefinedType: null, + udtAssemblyQualifiedName: assemblyQualifiedName, + isMultiValued: false, + fieldTypes: null, + extendedProperties: null); + private static SqlMetaDataPriv CreateUdtMetaData(string assemblyQualifiedName) => new() { diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs index 14bfa7cbc1..c8980ea33e 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; +using System.IO; using System.Linq; using System.Reflection; using Microsoft.Data.SqlClient.Tests.Common; @@ -500,39 +501,58 @@ public void SqlServerTypes_PinsCultureToNeutral() /// not match what the policy required is refused. /// /// + /// The two supported runtimes refuse at different points, and this test + /// accepts either, because both are the same outcome: the assembly is not + /// used. + /// /// On .NET the loader ignores the public key token in an AssemblyName, so - /// pinning the reference is not an enforcement boundary on its own. This - /// drives the real load path to confirm that the returned assembly's actual - /// identity is what gates the result. The test assembly is unsigned, so - /// allow-listing it under an explicit strong name token must fail even - /// though the assembly itself resolves. + /// it returns the real assembly and the policy's own post-load check is what + /// rejects it. On .NET Framework the loader enforces the strong name during + /// binding and throws instead, which is the same distinction + /// SqlAuthenticationProviderManager documents. Asserting only the .NET shape + /// would fail on net462, and asserting only the net462 shape would let the + /// post-load check regress unnoticed on .NET. + /// + /// The subject is a framework assembly rather than the test assembly so the + /// test does not depend on whether the build is strong-name signed. /// [Fact] - public void TryLoad_LoadedAssemblyWithWrongToken_IsRefused() + public void TryLoad_AssemblyWithWrongToken_IsRefused() { - AssemblyName self = typeof(UdtAssemblyPolicyTest).Assembly.GetName(); + // A framework assembly is certain to exist and to carry a strong name + // token that is not the fabricated one below. + AssemblyName subject = typeof(object).Assembly.GetName(); - // Guard: the reasoning below only holds for an unsigned test assembly. - byte[]? actualToken = self.GetPublicKeyToken(); - if (actualToken is { Length: > 0 }) - { - return; - } + byte[] wrongToken = { 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef }; - const string StrongToken = "b03f5f7f11d50a3a"; + Assert.False( + wrongToken.AsSpan().SequenceEqual((subject.GetPublicKeyToken() ?? Array.Empty()).AsSpan()), + "The fabricated token must differ from the real one for this test to mean anything."); - using PolicyScope scope = new($"{self.Name}, PublicKeyToken={StrongToken}"); + using PolicyScope scope = new($"{subject.Name}, PublicKeyToken={ToHex(wrongToken)}"); // The server names the assembly with the very token the allow list - // requires, so the entry matches and the load proceeds. This is the - // case that matters: on .NET the loader ignores the requested token and - // hands back the real, unsigned assembly of that simple name, so - // without a post-load check the wrong assembly would be accepted on the - // strength of a token it does not actually carry. - AssemblyName serverSupplied = new(self.Name!); - serverSupplied.SetPublicKeyToken(new byte[] { 0xb0, 0x3f, 0x5f, 0x7f, 0x11, 0xd5, 0x0a, 0x3a }); - - Assert.False(UdtAssemblyPolicy.TryLoad(serverSupplied, null, out Assembly? loaded)); + // requires, so the entry matches and the load proceeds. Without a + // post-load check the assembly would then be accepted on the strength of + // a token it does not actually carry. + AssemblyName serverSupplied = new(subject.Name!); + serverSupplied.SetPublicKeyToken((byte[])wrongToken.Clone()); + + bool permitted; + Assembly? loaded = null; + + try + { + permitted = UdtAssemblyPolicy.TryLoad(serverSupplied, null, out loaded); + } + catch (Exception e) when (e is FileLoadException or FileNotFoundException or BadImageFormatException) + { + // .NET Framework: the loader refused the bind outright. + return; + } + + // .NET: the loader returned the real assembly and the policy rejected it. + Assert.False(permitted); Assert.Null(loaded); } @@ -553,6 +573,45 @@ public void TryLoad_AllowListedAssembly_IsLoaded() Assert.Equal(self.Name, loaded!.GetName().Name); } + /// + /// Verifies that a version constraint the policy relied on is confirmed + /// against the assembly that was actually loaded. + /// + /// + /// A binding redirect on .NET Framework, or a custom resolver on .NET, can + /// satisfy a request with a different version than the one asked for. An + /// allow list entry that pinned a version must therefore not be satisfied by + /// whatever the loader chose to substitute. + /// + [Fact] + public void TryLoad_AssemblyWithWrongVersion_IsRefused() + { + AssemblyName subject = typeof(object).Assembly.GetName(); + + Version wrongVersion = new(subject.Version!.Major + 100, 0, 0, 0); + + using PolicyScope scope = new( + $"{subject.Name}, Version={wrongVersion}, PublicKeyToken={ToHex(subject.GetPublicKeyToken())}"); + + AssemblyName serverSupplied = new(subject.Name!) { Version = wrongVersion }; + serverSupplied.SetPublicKeyToken(subject.GetPublicKeyToken()); + + bool permitted; + Assembly? loaded = null; + + try + { + permitted = UdtAssemblyPolicy.TryLoad(serverSupplied, null, out loaded); + } + catch (Exception e) when (e is FileLoadException or FileNotFoundException or BadImageFormatException) + { + return; + } + + Assert.False(permitted); + Assert.Null(loaded); + } + #endregion #region Helpers From b7dae7d17edc8f8bbaa839138ad59ef1941a2d5a Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Wed, 23 Sep 2026 15:16:27 -0700 Subject: [PATCH 09/10] Read the allow list through AppDomain.GetData so net462 compiles AppContext.GetData does not exist on .NET Framework, so the net462 leg of the build failed with CS0117 while every .NET leg compiled fine. AppDomain.CurrentDomain.GetData is the portable equivalent. It has been present since .NET Framework 1.1, and on .NET it is implemented over the same AppContext data store, so it reads values written by either AppContext.SetData or AppDomain.SetData as well as runtimeconfig.json configProperties. Verified all three on net9.0. The documented way to set the allow list is unchanged: the docs already show AppDomain.CurrentDomain.SetData, which works on both frameworks. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60806d12-bdac-413c-bf83-f7c12e3cc12c --- .../src/Microsoft/Data/SqlClient/UdtAssemblyPolicy.cs | 7 ++++++- .../Data/SqlClient/UdtAssemblyLoadHardeningTest.cs | 2 +- .../Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs | 2 +- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/UdtAssemblyPolicy.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/UdtAssemblyPolicy.cs index 0ad2aa2cc6..6fe07d8c93 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/UdtAssemblyPolicy.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/UdtAssemblyPolicy.cs @@ -644,7 +644,12 @@ private static bool Matches(AssemblyName allowed, AssemblyName candidate) /// private static List GetAllowList() { - string source = AppContext.GetData(AllowListAppContextDataName) as string ?? string.Empty; + // AppDomain.GetData rather than AppContext.GetData: the latter does not + // exist on .NET Framework, while the former is implemented over the same + // AppContext data on .NET, so it reads both AppContext.SetData values and + // runtimeconfig.json configProperties on every target framework. + string source = + AppDomain.CurrentDomain.GetData(AllowListAppContextDataName) as string ?? string.Empty; lock (s_lock) { diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyLoadHardeningTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyLoadHardeningTest.cs index 56845dac42..a2c429edf1 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyLoadHardeningTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyLoadHardeningTest.cs @@ -396,7 +396,7 @@ public PolicyScope(bool legacy = false) { _switches = new LocalAppContextSwitchesHelper(); _originalAllowList = - AppContext.GetData(UdtAssemblyPolicy.AllowListAppContextDataName); + AppDomain.CurrentDomain.GetData(UdtAssemblyPolicy.AllowListAppContextDataName); _switches.UseLegacyUdtAssemblyLoad = legacy; diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs index c8980ea33e..d4bcf90582 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs @@ -55,7 +55,7 @@ public PolicyScope(bool legacy = false) { _switches = new LocalAppContextSwitchesHelper(); _originalAllowList = - AppContext.GetData(UdtAssemblyPolicy.AllowListAppContextDataName); + AppDomain.CurrentDomain.GetData(UdtAssemblyPolicy.AllowListAppContextDataName); _switches.UseLegacyUdtAssemblyLoad = legacy; From 50dfbce7315c94f276d168b30d34c0eb5b31fa82 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Wed, 23 Sep 2026 15:25:52 -0700 Subject: [PATCH 10/10] Verify the simple name and snapshot loads before the first policy load Two further findings from review, both cases where a permission granted to one assembly could be inherited by another. Confirm the simple name after the load. Every basis for permitting a load rests on the simple name: it is what the allow list is matched on and what the built-in exemption recognizes. Nothing checked it once the loader returned, and for a simple-name allow list entry no other component was constrained either, so SatisfiesRequiredIdentity took its all-null early-out and accepted whatever arrived. A custom AssemblyResolve handler or AssemblyLoadContext resolver could answer with an unrelated assembly and have it inherit the permission. Decision now carries the matched name and it is always verified, so the early-out is gone. Snapshot the loaded set before any policy-triggered load. The provenance guard added previously only suppressed the AssemblyLoad callback, which left the first call unprotected: the map is built lazily, so a first request permitted by the allow list loaded before the map existed, and the dependencies that arrived with it were then captured by the later snapshot as though the application had brought them in. A server naming one of them was accepted as already-loaded. TryDecide now takes the snapshot up front, which also attaches the handler early enough that subsequent loads are attributed rather than absorbed. Both tests were confirmed to fail with their respective fix reverted and to pass with it applied. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 60806d12-bdac-413c-bf83-f7c12e3cc12c --- .github/instructions/features.instructions.md | 26 +-- .../Data/SqlClient/UdtAssemblyPolicy.cs | 51 +++++- .../Data/SqlClient/UdtAssemblyPolicyTest.cs | 153 ++++++++++++++++++ 3 files changed, 211 insertions(+), 19 deletions(-) diff --git a/.github/instructions/features.instructions.md b/.github/instructions/features.instructions.md index 213b0dc760..761f800425 100644 --- a/.github/instructions/features.instructions.md +++ b/.github/instructions/features.instructions.md @@ -279,10 +279,13 @@ control rather than the server's. Normalizing the reference is necessary but not sufficient. On .NET the loader **ignores** the public key token in an `AssemblyName`, and can satisfy a request with a different version than the one asked for, so pinning the reference does -not by itself determine what arrives. The driver therefore verifies the identity -of the assembly the loader actually hands back against every component the -decision relied on, and refuses it on any mismatch. This mirrors what the driver -already does for the Azure authentication extension assembly. +not by itself determine what arrives. A custom `AssemblyResolve` handler or +`AssemblyLoadContext` resolver can go further still and answer with an assembly +of an entirely different name. The driver therefore verifies the identity of the +assembly the loader actually hands back against every component the decision +relied on, including the simple name that the permission was granted to, and +refuses it on any mismatch. This mirrors what the driver already does for the +Azure authentication extension assembly. On .NET, the already-loaded tier is scoped to the `AssemblyLoadContext` that loaded the driver, since that is the context its `Assembly.Load` calls resolve @@ -343,13 +346,14 @@ resolved type must still carry `SqlUserDefinedTypeAttribute`, so this is confined to types that were written to be deserialized from SQL Server, but it is a genuine widening and is called out here deliberately. -Relatedly, the map of loaded assemblies is built once and then maintained -incrementally, and loads that the policy itself triggers are excluded from it. -Neither is merely a performance choice. Rebuilding the map on demand, or -recording the dependencies that arrive alongside a permitted assembly, would let -an assembly that was pulled in as a *dependency* of a permitted assembly -silently inherit that permission. Both keep the tier anchored to what the -application loaded of its own accord. +Relatedly, the map of loaded assemblies is snapshotted before the policy can +trigger any load of its own, and loads the policy performs are excluded from it +thereafter. Neither is merely a performance choice. Rebuilding the map on +demand, snapshotting it lazily after a permitted load had already run, or +recording the dependencies that arrive alongside a permitted assembly, would all +let an assembly that was pulled in as a *dependency* of a permitted assembly +silently inherit that permission. Together they keep the tier anchored to what +the application loaded of its own accord. #### Compatibility impact diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/UdtAssemblyPolicy.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/UdtAssemblyPolicy.cs index 6fe07d8c93..6c44210523 100644 --- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/UdtAssemblyPolicy.cs +++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/UdtAssemblyPolicy.cs @@ -277,6 +277,20 @@ private readonly struct Decision /// internal Assembly? Loaded { get; init; } + /// + /// The simple name the loaded assembly must carry. + /// + /// + /// Every basis for permitting a load rests on the simple name: it is + /// what the allow list was matched on and what the built-in exemption + /// recognizes. A custom AssemblyResolve handler or + /// AssemblyLoadContext resolver can return an assembly with an + /// entirely different name, which would inherit a permission that was + /// never granted to it, so the name is confirmed after the load like + /// every other component the decision relied on. + /// + internal string? RequiredSimpleName { get; init; } + /// /// The public key token the loaded assembly must carry, or null when /// the basis for permitting it placed no constraint on the token. @@ -385,6 +399,19 @@ private static bool TryDecide( return false; } + // Snapshot what the process already holds before any tier below can + // trigger a load. The already-loaded tier is meant to reflect only + // what the application brought in of its own accord, so if the very + // first request were permitted by the allow list, the dependencies + // that arrived with it would otherwise be captured by a later, lazier + // snapshot and silently inherit that permission. Taking the snapshot + // here also attaches the load handler, so every subsequent load is + // attributed rather than absorbed. + lock (s_lock) + { + GetLoadedAssemblies(); + } + // The built-in types assembly is always permitted, but only once its // identity has been pinned, so the exemption cannot be satisfied by an // arbitrary assembly that borrows the name. @@ -394,6 +421,7 @@ private static bool TryDecide( decision = new Decision { + RequiredSimpleName = asmRef.Name, RequiredPublicKeyToken = s_sqlServerTypesPublicKeyToken, // The culture was just pinned to neutral, so require that back. RequiredCultureName = string.Empty, @@ -415,6 +443,9 @@ private static bool TryDecide( decision = new Decision { + // The entry was matched on this name, so this is the name the + // permission was granted to. + RequiredSimpleName = simpleName, // A null token means the entry did not mention one, an empty // token means it explicitly required an unsigned assembly. RequiredPublicKeyToken = allowedToken is { Length: > 0 } ? allowedToken : null, @@ -448,14 +479,6 @@ private static bool TryDecide( /// private static bool SatisfiesRequiredIdentity(Assembly loaded, Decision decision) { - if (decision.RequiredPublicKeyToken is null && - !decision.RequireUnsigned && - decision.RequiredVersion is null && - decision.RequiredCultureName is null) - { - return true; - } - AssemblyName actual; try @@ -469,6 +492,18 @@ decision.RequiredVersion is null && return false; } + // The simple name is the one component every basis constrains, so it is + // always confirmed. Without this a resolver could answer the request + // with an unrelated assembly and have it inherit the permission. + if (decision.RequiredSimpleName is not null && + !string.Equals( + decision.RequiredSimpleName, + actual.Name, + StringComparison.OrdinalIgnoreCase)) + { + return false; + } + byte[]? actualToken = actual.GetPublicKeyToken(); if (decision.RequireUnsigned) diff --git a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs index d4bcf90582..fa167357c9 100644 --- a/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs +++ b/src/Microsoft.Data.SqlClient/tests/UnitTests/Microsoft/Data/SqlClient/UdtAssemblyPolicyTest.cs @@ -432,6 +432,159 @@ public void IsAllowed_EmptySimpleName_IsDenied() #region Identity enforcement + /// + /// Verifies that an assembly whose simple name differs from the one the + /// policy permitted is refused, even when the allow list entry constrained + /// nothing else. + /// + /// + /// Every basis for permitting a load rests on the simple name, so a + /// resolver that answers the request with an unrelated assembly would + /// otherwise have it inherit a permission that was never granted to it. + /// A simple-name entry is the weakest case: it constrains no version, + /// culture or token, so the name is the only thing left to verify. + /// + [Fact] + public void TryLoad_ResolverReturnsDifferentAssembly_IsRefused() + { + using PolicyScope scope = new(UnknownAssemblyName); + + Assembly substitute = typeof(string).Assembly; + + Assert.NotEqual( + UnknownAssemblyName, + substitute.GetName().Name, + StringComparer.OrdinalIgnoreCase); + + ResolveEventHandler handler = (_, args) => + new AssemblyName(args.Name).Name == UnknownAssemblyName ? substitute : null; + + AppDomain.CurrentDomain.AssemblyResolve += handler; + + try + { + bool loaded = UdtAssemblyPolicy.TryLoad( + new AssemblyName(UnknownAssemblyName), + null, + out Assembly? assembly); + + Assert.False(loaded); + Assert.Null(assembly); + } + finally + { + AppDomain.CurrentDomain.AssemblyResolve -= handler; + } + } + + /// + /// Verifies that an assembly which first arrives in the process during a + /// policy-triggered load is not subsequently permitted on the strength of + /// being "already loaded". + /// + /// + /// The already-loaded tier is meant to reflect only what the application + /// brought in of its own accord. The loaded-assembly map is built lazily, + /// so if the first policy call is permitted by the allow list, the load it + /// performs happens before the map exists and its dependencies would be + /// captured by the later snapshot, silently inheriting that permission. + /// This is the transitive trust the policy documents as denied. + /// + [Fact] + public void AlreadyLoaded_AssemblyArrivingDuringPolicyLoad_IsNotPermitted() + { + // An assembly that ships with the framework but is not loaded in this + // process, standing in for a dependency pulled in by a permitted load. + string? dependencyPath = FindUnloadedFrameworkAssembly(out string? dependencyName); + + Assert.NotNull(dependencyPath); + Assert.NotNull(dependencyName); + + using PolicyScope scope = new(UnknownAssemblyName); + + // Drop the map so this is the first policy call, which is the ordering + // the bug depended on. + UdtAssemblyPolicy.ResetCache(); + + ResolveEventHandler handler = (_, args) => + { + if (new AssemblyName(args.Name).Name == UnknownAssemblyName) + { + // Bring the stand-in dependency into the process during the + // policy's own load, then decline to satisfy the request. + Assembly.LoadFrom(dependencyPath!); + } + + return null; + }; + + AppDomain.CurrentDomain.AssemblyResolve += handler; + + try + { + UdtAssemblyPolicy.TryLoad(new AssemblyName(UnknownAssemblyName), null, out _); + } + catch (FileNotFoundException) + { + // Expected: the handler declines to satisfy the request, so the + // load fails. Production callers catch this the same way. What + // matters is the side effect it had on the loaded-assembly map. + } + finally + { + AppDomain.CurrentDomain.AssemblyResolve -= handler; + } + + // The dependency is now loaded, but the application never asked for it, + // so a server naming it must still be refused. + Assert.False(IsAllowed(new AssemblyName(dependencyName!), null)); + } + + /// + /// Locates a framework assembly that is present on disk but not loaded in + /// this process, to stand in for a dependency arriving during a load. + /// + private static string? FindUnloadedFrameworkAssembly(out string? simpleName) + { + HashSet loaded = new(StringComparer.OrdinalIgnoreCase); + + foreach (Assembly assembly in AppDomain.CurrentDomain.GetAssemblies()) + { + try + { + string? name = assembly.GetName().Name; + + if (name is not null) + { + loaded.Add(name); + } + } + catch + { + // An assembly whose name cannot be read cannot collide with the + // candidate either, so it is simply skipped. + } + } + + string directory = Path.GetDirectoryName(typeof(object).Assembly.Location)!; + + foreach (string path in Directory.GetFiles(directory, "System.*.dll")) + { + string candidate = Path.GetFileNameWithoutExtension(path); + + if (!loaded.Contains(candidate)) + { + simpleName = candidate; + + return path; + } + } + + simpleName = null; + + return null; + } + /// /// Verifies that an allow list entry which explicitly requires an unsigned /// assembly (PublicKeyToken=null) is not satisfied by a signed one.