Merging internal commits for release/6.0

src/libraries/Common/tests/TestUtilities/System/AssertExtensions.cs

@@ -351,6 +351,20 @@ public static void GreaterThanOrEqualTo<T>(T actual, T greaterThanOrEqualTo, str
                 throw new XunitException(AddOptionalUserMessage($"Expected: {actual} to be greater than or equal to {greaterThanOrEqualTo}", userMessage));
         }
 
+        /// <summary>
+        /// Validate that a given enum value has the expected flag set.
+        /// </summary>
+        /// <typeparam name="T">The enum type.</typeparam>
+        /// <param name="expected">The flag which should be present in <paramref name="actual"/>.</param>
+        /// <param name="actual">The value which should contain the flag <paramref name="expected"/>.</param>
+        public static void HasFlag<T>(T expected, T actual, string userMessage = null) where T : Enum
+        {
+            if (!actual.HasFlag(expected))
+            {
+                throw new XunitException(AddOptionalUserMessage($"Expected: Value {actual} (of enum type {typeof(T).FullName}) to have the flag {expected} set.", userMessage));
+            }
+        }
+
         // NOTE: Consider using SequenceEqual below instead, as it will give more useful information about what
         // the actual differences are, especially for large arrays/spans.
         /// <summary>

src/libraries/System.Security.Cryptography.Encoding/src/Internal/Cryptography/OidLookup.cs

@@ -14,8 +14,8 @@ internal static partial class OidLookup
         private static readonly ConcurrentDictionary<string, string> s_lateBoundOidToFriendlyName =
             new ConcurrentDictionary<string, string>();
 
-        private static readonly ConcurrentDictionary<string, string?> s_lateBoundFriendlyNameToOid =
-            new ConcurrentDictionary<string, string?>(StringComparer.OrdinalIgnoreCase);
+        private static readonly ConcurrentDictionary<string, string> s_lateBoundFriendlyNameToOid =
+            new ConcurrentDictionary<string, string>(StringComparer.OrdinalIgnoreCase);
 
         //
         // Attempts to map a friendly name to an OID. Returns null if not a known name.
@@ -80,19 +80,13 @@ internal static partial class OidLookup
 
             mappedOid = NativeFriendlyNameToOid(friendlyName, oidGroup, fallBackToAllGroups);
 
-            if (shouldUseCache)
+            if (shouldUseCache && mappedOid != null)
             {
                 s_lateBoundFriendlyNameToOid.TryAdd(friendlyName, mappedOid);
 
                 // Don't add the reverse here.  Friendly Name => OID is a case insensitive search,
                 // so the casing provided as input here may not be the 'correct' one.  Just let
                 // ToFriendlyName capture the response and cache it itself.
-
-                // Also, mappedOid could be null here if the lookup failed.  Allowing storing null
-                // means we're able to cache that a lookup failed so we don't repeat it.  It's
-                // theoretically possible, however, the failure could have been intermittent, e.g.
-                // if the call was forced to follow an AD fallback path and the relevant servers
-                // were offline.
             }
 
             return mappedOid;

src/libraries/System.Security.Cryptography.Pkcs/tests/SignedCms/SignedCmsTests.netcoreapp.cs

@@ -399,7 +399,10 @@ public static void AddSigner_RSA_EphemeralKey()
                 {
                     ContentInfo content = new ContentInfo(new byte[] { 1, 2, 3 });
                     SignedCms cms = new SignedCms(content, false);
-                    CmsSigner signer = new CmsSigner(certWithEphemeralKey);
+                    CmsSigner signer = new CmsSigner(certWithEphemeralKey)
+                    {
+                        IncludeOption = X509IncludeOption.EndCertOnly
+                    };
                     cms.ComputeSignature(signer);
                 }
             }
@@ -429,7 +432,8 @@ public static void AddSigner_DSA_EphemeralKey()
                     SignedCms cms = new SignedCms(content, false);
                     CmsSigner signer = new CmsSigner(certWithEphemeralKey)
                     {
-                        DigestAlgorithm = new Oid(Oids.Sha1, Oids.Sha1)
+                        DigestAlgorithm = new Oid(Oids.Sha1, Oids.Sha1),
+                        IncludeOption = X509IncludeOption.EndCertOnly
                     };
                     cms.ComputeSignature(signer);
                 }
@@ -458,7 +462,10 @@ public static void AddSigner_ECDSA_EphemeralKey()
                 {
                     ContentInfo content = new ContentInfo(new byte[] { 1, 2, 3 });
                     SignedCms cms = new SignedCms(content, false);
-                    CmsSigner signer = new CmsSigner(certWithEphemeralKey);
+                    CmsSigner signer = new CmsSigner(certWithEphemeralKey)
+                    {
+                        IncludeOption = X509IncludeOption.EndCertOnly
+                    };
                     cms.ComputeSignature(signer);
                 }
             }

src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/UnixPkcs12Reader.cs

@@ -25,7 +25,7 @@ internal abstract class UnixPkcs12Reader : IDisposable
         private ContentInfoAsn[]? _safeContentsValues;
         private CertAndKey[]? _certs;
         private int _certCount;
-        private PointerMemoryManager<byte>? _tmpManager;
+        private Memory<byte> _tmpMemory;
         private bool _allowDoubleBind;
 
         protected abstract ICertificatePalCore ReadX509Der(ReadOnlyMemory<byte> data);
@@ -40,19 +40,13 @@ protected void ParsePkcs12(ReadOnlySpan<byte> data)
 
                 // Windows compatibility: Ignore trailing data.
                 ReadOnlySpan<byte> encodedData = reader.PeekEncodedValue();
+                byte[] dataWithoutTrailing = GC.AllocateUninitializedArray<byte>(encodedData.Length, pinned: true);
+                encodedData.CopyTo(dataWithoutTrailing);
+                _tmpMemory = MemoryMarshal.CreateFromPinnedArray(dataWithoutTrailing, 0, dataWithoutTrailing.Length);
 
-                unsafe
-                {
-                    IntPtr tmpPtr = Marshal.AllocHGlobal(encodedData.Length);
-                    Span<byte> tmpSpan = new Span<byte>((byte*)tmpPtr, encodedData.Length);
-                    encodedData.CopyTo(tmpSpan);
-                    _tmpManager = new PointerMemoryManager<byte>((void*)tmpPtr, encodedData.Length);
-                }
-
-                ReadOnlyMemory<byte> tmpMemory = _tmpManager.Memory;
-                reader = new AsnValueReader(tmpMemory.Span, AsnEncodingRules.BER);
+                reader = new AsnValueReader(_tmpMemory.Span, AsnEncodingRules.BER);
 
-                PfxAsn.Decode(ref reader, tmpMemory, out PfxAsn pfxAsn);
+                PfxAsn.Decode(ref reader, _tmpMemory, out PfxAsn pfxAsn);
 
                 if (pfxAsn.AuthSafe.ContentType != Oids.Pkcs7Data)
                 {
@@ -113,26 +107,8 @@ internal IEnumerable<CertAndKey> EnumerateAll()
 
         public void Dispose()
         {
-            // Generally, having a MemoryManager cleaned up in a Dispose is a bad practice.
-            // In this case, the UnixPkcs12Reader is only ever created in a using statement,
-            // never accessed by a second thread, and there isn't a manual call to Dispose
-            // mixed in anywhere.
-            if (_tmpManager != null)
-            {
-                unsafe
-                {
-                    Span<byte> tmp = _tmpManager.GetSpan();
-                    CryptographicOperations.ZeroMemory(tmp);
-
-                    fixed (byte* ptr = tmp)
-                    {
-                        Marshal.FreeHGlobal((IntPtr)ptr);
-                    }
-                }
-
-                ((IDisposable)_tmpManager).Dispose();
-                _tmpManager = null;
-            }
+            CryptographicOperations.ZeroMemory(_tmpMemory.Span);
+            _tmpMemory = Memory<byte>.Empty;
 
             ContentInfoAsn[]? rentedContents = Interlocked.Exchange(ref _safeContentsValues, null);
             CertAndKey[]? rentedCerts = Interlocked.Exchange(ref _certs, null);

src/libraries/System.Security.Cryptography.X509Certificates/src/Resources/Strings.resx

@@ -493,4 +493,7 @@
   <data name="Cryptography_X509_PfxWithoutPassword" xml:space="preserve">
     <value>PKCS12 (PFX) without a supplied password has exceeded maximum allowed iterations. See https://go.microsoft.com/fwlink/?linkid=2233907 for more information.</value>
   </data>
+  <data name="Cryptography_X509_ChainBuildingFailed" xml:space="preserve">
+    <value>An unknown chain building error occurred.</value>
+  </data>
 </root>

src/libraries/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/LocalAppContextSwitches.cs

@@ -9,6 +9,8 @@ internal static partial class LocalAppContextSwitches
 
         internal static long Pkcs12UnspecifiedPasswordIterationLimit { get; } = InitializePkcs12UnspecifiedPasswordIterationLimit();
 
+        internal static bool X509ChainBuildThrowOnInternalError { get; } = InitializeX509ChainBuildThrowOnInternalError();
+
         private static long InitializePkcs12UnspecifiedPasswordIterationLimit()
         {
             object? data = AppContext.GetData("System.Security.Cryptography.Pkcs12UnspecifiedPasswordIterationLimit");
@@ -27,5 +29,12 @@ private static long InitializePkcs12UnspecifiedPasswordIterationLimit()
                 return DefaultPkcs12UnspecifiedPasswordIterationLimit;
             }
         }
+
+        private static bool InitializeX509ChainBuildThrowOnInternalError()
+        {
+            // n.b. the switch defaults to TRUE if it has not been explicitly set.
+            return AppContext.TryGetSwitch("System.Security.Cryptography.ThrowOnX509ChainBuildInternalError", out bool isEnabled)
+                ? isEnabled : true;
+        }
     }
 }

src/libraries/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/X509Chain.cs

@@ -145,26 +145,62 @@ internal bool Build(X509Certificate2 certificate, bool throwOnException)
                     chainPolicy.UrlRetrievalTimeout,
                     chainPolicy.DisableCertificateDownloads);
 
-                if (_pal == null)
-                    return false;
-
-                _chainElements = new X509ChainElementCollection(_pal.ChainElements!);
-
-                Exception? verificationException;
-                bool? verified = _pal.Verify(chainPolicy.VerificationFlags, out verificationException);
-                if (!verified.HasValue)
+                bool success = false;
+                if (_pal is not null)
                 {
-                    if (throwOnException)
-                    {
-                        throw verificationException!;
-                    }
-                    else
+                    _chainElements = new X509ChainElementCollection(_pal.ChainElements!);
+
+                    Exception? verificationException;
+                    bool? verified = _pal.Verify(chainPolicy.VerificationFlags, out verificationException);
+                    if (!verified.HasValue)
                     {
-                        verified = false;
+                        if (throwOnException)
+                        {
+                            throw verificationException!;
+                        }
+                        else
+                        {
+                            verified = false;
+                        }
                     }
+
+                    success = verified.Value;
+                }
+
+                // There are two reasons success might be false here.
+                //
+                // The most common reason is that we built the chain but the chain appears to run
+                // afoul of policy. This is represented by BuildChain returning a non-null object
+                // and storing potential policy violations in the chain structure. The public Build
+                // method returns false to the caller, and the caller can inspect the ChainStatus
+                // and ChainElements properties and evaluate the failure reason against app-level
+                // policies. If the caller does not care about these policy violations, they can
+                // choose to ignore them and to treat chain building as successful.
+                //
+                // The other type of failure is that BuildChain simply can't build the chain at all.
+                // Perhaps something within the certificate is not valid or is unsupported, or perhaps
+                // there's an internal failure within the OS layer we're invoking, etc. Whatever the
+                // reason, we're not meaningfully able to initialize the ChainStatus property, which
+                // means callers may observe a non-empty list of policy violations. Depending on the
+                // caller's logic, they might incorrectly interpret this as there being no policy
+                // violations at all, which means they might treat this condition as success.
+                //
+                // To avoid callers misinterpeting this latter condition as success, we'll throw an
+                // exception, which matches general .NET API behavior when a method cannot complete
+                // its objective. A compat switch is provided to normalize this back to a 'false'
+                // return value for callers who cannot handle an exception here. If throwOnException
+                // is false, it means the caller explicitly wants to suppress exceptions and normalize
+                // them to a false return value.
+
+                if (!success
+                    && throwOnException
+                    && _pal?.ChainStatus is not { Length: > 0 }
+                    && LocalAppContextSwitches.X509ChainBuildThrowOnInternalError)
+                {
+                    throw new CryptographicException(SR.Cryptography_X509_ChainBuildingFailed);
                 }
 
-                return verified.Value;
+                return success;
             }
         }
 

src/libraries/System.Security.Cryptography.X509Certificates/tests/ChainTests.cs

@@ -1248,6 +1248,58 @@ public static void BuildChainForCertificateWithMD5Signature()
             }
         }
 
+        [Fact]
+        public static void BuildChainForSelfSignedCertificate_WithSha256RsaSignature()
+        {
+            using (ChainHolder chainHolder = new ChainHolder())
+            using (X509Certificate2 cert = new X509Certificate2(TestData.SelfSignedCertSha256RsaBytes))
+            {
+                X509Chain chain = chainHolder.Chain;
+                chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck;
+                chain.ChainPolicy.VerificationTime = cert.NotBefore.AddHours(2);
+
+                // No custom root of trust store means that this self-signed cert will at
+                // minimum be marked UntrustedRoot.
+
+                Assert.False(chain.Build(cert));
+                AssertExtensions.HasFlag(X509ChainStatusFlags.UntrustedRoot, chain.AllStatusFlags());
+            }
+        }
+
+        [Fact]
+        public static void BuildChainForSelfSignedCertificate_WithUnknownOidSignature()
+        {
+            using (ChainHolder chainHolder = new ChainHolder())
+            using (X509Certificate2 cert = new X509Certificate2(TestData.SelfSignedCertDummyOidBytes))
+            {
+                X509Chain chain = chainHolder.Chain;
+                chain.ChainPolicy.RevocationMode = X509RevocationMode.NoCheck;
+                chain.ChainPolicy.VerificationTime = cert.NotBefore.AddHours(2);
+
+                // This tests a self-signed cert whose signature block contains a garbage signing alg OID.
+                // Some platforms return NotSignatureValid to indicate that they cannot understand the
+                // signature block. Other platforms return PartialChain to indicate that they think the
+                // bad signature block might correspond to some unknown, untrusted signer. Yet other
+                // platforms simply fail the operation; e.g., Windows's CertGetCertificateChain API returns
+                // NTE_BAD_ALGID, which we bubble up as CryptographicException.
+
+                if (PlatformDetection.UsesAppleCrypto)
+                {
+                    Assert.False(chain.Build(cert));
+                    AssertExtensions.HasFlag(X509ChainStatusFlags.PartialChain, chain.AllStatusFlags());
+                }
+                else if (PlatformDetection.IsOpenSslSupported)
+                {
+                    Assert.False(chain.Build(cert));
+                    AssertExtensions.HasFlag(X509ChainStatusFlags.NotSignatureValid, chain.AllStatusFlags());
+                }
+                else
+                {
+                    Assert.ThrowsAny<CryptographicException>(() => chain.Build(cert));
+                }
+            }
+        }
+
         internal static X509ChainStatusFlags AllStatusFlags(this X509Chain chain)
         {
             return chain.ChainStatus.Aggregate(