From c429b2f6f23849d3df6883f87aaeb66a25c02d23 Mon Sep 17 00:00:00 2001 From: Cheena Malhotra Date: Tue, 28 Jul 2026 13:50:36 -0700 Subject: [PATCH 1/5] 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 2/5] 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 3/5] 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 4/5] 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 5/5] 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