Look up trusted certs consistently on windows (#56701)

* Search for trusted certificates consistently on Windows 1. Don't use thumbprint so we don't get flagged for using SHA-1 2. Make TrustCertificateCore and RemoveCertificateFromTrustedRoots consistent * Add a note about our usage of Thumbprint on macOS * Clean up assumptions about root store * FindBySubjectName expects a string * Search by serial number to avoid having to parse subject name * Fix typo Co-authored-by: Martin Costello <[email protected]> * Call DisposeCertificates more consistently --------- Co-authored-by: Martin Costello <[email protected]>
dotnet · Jul 22, 2024 · ed7ea40 · ed7ea40
1 parent 9503913
commit ed7ea40
Show file tree

Hide file tree

Showing 3 changed files with 57 additions and 16 deletions.
diff --git a/src/Shared/CertificateGeneration/CertificateManager.cs b/src/Shared/CertificateGeneration/CertificateManager.cs
@@ -812,6 +812,50 @@ internal static string ToCertificateDescription(IEnumerable<X509Certificate2> ce
     internal static string GetDescription(X509Certificate2 c) =>
         $"{c.Thumbprint} - {c.Subject} - Valid from {c.NotBefore:u} to {c.NotAfter:u} - IsHttpsDevelopmentCertificate: {IsHttpsDevelopmentCertificate(c).ToString().ToLowerInvariant()} - IsExportable: {Instance.IsExportable(c).ToString().ToLowerInvariant()}";
 
+    /// <remarks>
+    /// <see cref="X509Certificate.Equals(X509Certificate?)"/> is not adequate for security purposes.
+    /// </remarks>
+    internal static bool AreCertificatesEqual(X509Certificate2 cert1, X509Certificate2 cert2)
+    {
+        return cert1.RawDataMemory.Span.SequenceEqual(cert2.RawDataMemory.Span);
+    }
+
+    /// <summary>
+    /// Given a certificate, usually from the <see cref="StoreName.My"/> store, try to find the
+    /// corresponding certificate in <paramref name="store"/> (usually the <see cref="StoreName.Root"/> store)."/>
+    /// </summary>
+    /// <param name="store">An open <see cref="X509Store"/>.</param>
+    /// <param name="certificate">A certificate to search for.</param>
+    /// <param name="foundCertificate">The certificate, if any, corresponding to <paramref name="certificate"/> in <paramref name="store"/>.</param>
+    /// <returns>True if a corresponding certificate was found.</returns>
+    /// <remarks><see cref="ListCertificates"/> has richer filtering and a lot of debugging output that's unhelpful here.</remarks>
+    internal static bool TryFindCertificateInStore(X509Store store, X509Certificate2 certificate, [NotNullWhen(true)] out X509Certificate2? foundCertificate)
+    {
+        foundCertificate = null;
+
+        // We specifically don't search by thumbprint to avoid being flagged for using a SHA-1 hash.
+        var certificatesWithSubjectName = store.Certificates.Find(X509FindType.FindBySerialNumber, certificate.SerialNumber, validOnly: false);
+        if (certificatesWithSubjectName.Count == 0)
+        {
+            return false;
+        }
+
+        var certificatesToDispose = new List<X509Certificate2>();
+        foreach (var candidate in certificatesWithSubjectName.OfType<X509Certificate2>())
+        {
+            if (foundCertificate is null && AreCertificatesEqual(candidate, certificate))
+            {
+                foundCertificate = candidate;
+            }
+            else
+            {
+                certificatesToDispose.Add(candidate);
+            }
+        }
+        DisposeCertificates(certificatesToDispose);
+        return foundCertificate is not null;
+    }
+
     [EventSource(Name = "Dotnet-dev-certs")]
     public sealed class CertificateManagerEventSource : EventSource
     {

diff --git a/src/Shared/CertificateGeneration/MacOSCertificateManager.cs b/src/Shared/CertificateGeneration/MacOSCertificateManager.cs
@@ -11,6 +11,11 @@
 
 namespace Microsoft.AspNetCore.Certificates.Generation;
 
+/// <remarks>
+/// Normally, we avoid the use of <see cref="X509Certificate2.Thumbprint"/> because it's a SHA-1 hash and, therefore,
+/// not adequate for security applications.  However, the MacOS security tool uses SHA-1 hashes for certificate
+/// identification, so we're stuck.
+/// </remarks>
 internal sealed class MacOSCertificateManager : CertificateManager
 {
     // User keychain. Guard with quotes when using in command lines since users may have set

diff --git a/src/Shared/CertificateGeneration/WindowsCertificateManager.cs b/src/Shared/CertificateGeneration/WindowsCertificateManager.cs
@@ -71,26 +71,22 @@ protected override X509Certificate2 SaveCertificateCore(X509Certificate2 certifi
 
     protected override void TrustCertificateCore(X509Certificate2 certificate)
     {
-        using var publicCertificate = new X509Certificate2(certificate.Export(X509ContentType.Cert));
-
-        publicCertificate.FriendlyName = certificate.FriendlyName;
-
         using var store = new X509Store(StoreName.Root, StoreLocation.CurrentUser);
-
         store.Open(OpenFlags.ReadWrite);
-        var existing = store.Certificates.Find(X509FindType.FindByThumbprint, publicCertificate.Thumbprint, validOnly: false);
-        if (existing.Count > 0)
+
+        if (TryFindCertificateInStore(store, certificate, out _))
         {
             Log.WindowsCertificateAlreadyTrusted();
-            DisposeCertificates(existing.OfType<X509Certificate2>());
             return;
         }
 
         try
         {
             Log.WindowsAddCertificateToRootStore();
+
+            using var publicCertificate = X509CertificateLoader.LoadCertificate(certificate.Export(X509ContentType.Cert));
+            publicCertificate.FriendlyName = certificate.FriendlyName;
             store.Add(publicCertificate);
-            store.Close();
         }
         catch (CryptographicException exception) when (exception.HResult == UserCancelledErrorCode)
         {
@@ -102,14 +98,11 @@ protected override void TrustCertificateCore(X509Certificate2 certificate)
     protected override void RemoveCertificateFromTrustedRoots(X509Certificate2 certificate)
     {
         Log.WindowsRemoveCertificateFromRootStoreStart();
-        using var store = new X509Store(StoreName.Root, StoreLocation.CurrentUser);
 
+        using var store = new X509Store(StoreName.Root, StoreLocation.CurrentUser);
         store.Open(OpenFlags.ReadWrite);
-        var matching = store.Certificates
-            .OfType<X509Certificate2>()
-            .SingleOrDefault(c => c.SerialNumber == certificate.SerialNumber);
 
-        if (matching != null)
+        if (TryFindCertificateInStore(store, certificate, out var matching))
         {
             store.Remove(matching);
         }
@@ -118,14 +111,13 @@ protected override void RemoveCertificateFromTrustedRoots(X509Certificate2 certi
             Log.WindowsRemoveCertificateFromRootStoreNotFound();
         }
 
-        store.Close();
         Log.WindowsRemoveCertificateFromRootStoreEnd();
     }
 
     public override bool IsTrusted(X509Certificate2 certificate)
     {
         return ListCertificates(StoreName.Root, StoreLocation.CurrentUser, isValid: true, requireExportable: false)
-            .Any(c => c.Thumbprint == certificate.Thumbprint);
+            .Any(c => AreCertificatesEqual(c, certificate));
     }
 
     protected override IList<X509Certificate2> GetCertificatesToRemove(StoreName storeName, StoreLocation storeLocation)