crypto: improve system certificate enumeration logic on macOS

src/crypto/crypto_context.cc

@@ -369,7 +369,10 @@ TrustStatus IsTrustDictionaryTrustedForPolicy(CFDictionaryRef trust_dict,
     CFStringRef policy_oid = reinterpret_cast<CFStringRef>(
         const_cast<void*>(CFDictionaryGetValue(policy_dict, kSecPolicyOid)));
 
-    if (!CFEqual(policy_oid, kSecPolicyAppleSSL)) {
+    bool matches_ssl = CFEqual(policy_oid, kSecPolicyAppleSSL);
+    CFRelease(policy_dict);
+
+    if (!matches_ssl) {
       return TrustStatus::UNSPECIFIED;
     }
   }
@@ -386,35 +389,44 @@ TrustStatus IsTrustDictionaryTrustedForPolicy(CFDictionaryRef trust_dict,
                           &trust_settings_result)) {
       return TrustStatus::UNSPECIFIED;
     }
+  }
 
-    if (trust_settings_result == kSecTrustSettingsResultDeny) {
-      return TrustStatus::DISTRUSTED;
-    }
-
-    // This is a bit of a hack: if the cert is self-issued allow either
-    // kSecTrustSettingsResultTrustRoot or kSecTrustSettingsResultTrustAsRoot on
-    // the basis that SecTrustSetTrustSettings should not allow creating an
-    // invalid trust record in the first place. (The spec is that
-    // kSecTrustSettingsResultTrustRoot can only be applied to root(self-signed)
-    // certs and kSecTrustSettingsResultTrustAsRoot is used for other certs.)
-    // This hack avoids having to check the signature on the cert which is slow
-    // if using the platform APIs, and may require supporting MD5 signature
-    // algorithms on some older OSX versions or locally added roots, which is
-    // undesirable in the built-in signature verifier.
-    if (is_self_issued) {
-      return trust_settings_result == kSecTrustSettingsResultTrustRoot ||
-                     trust_settings_result == kSecTrustSettingsResultTrustAsRoot
-                 ? TrustStatus::TRUSTED
-                 : TrustStatus::UNSPECIFIED;
-    }
-
-    // kSecTrustSettingsResultTrustAsRoot can only be applied to non-root certs.
-    return (trust_settings_result == kSecTrustSettingsResultTrustAsRoot)
+  // When kSecTrustSettingsResult is absent from the trust dict,
+  // Apple docs specify kSecTrustSettingsResultTrustRoot as the default.
+  // Refs
+  // https://github.com/apple-oss-distributions/Security/blob/db15acbe6a7f257a859ad9a3bb86097bfe0679d9/trust/headers/SecTrustSettings.h#L119-L122
+  // This is also enforced at write time for self-signed certs get TrustRoot,
+  // and non-self-signed certs cannot have an empty settings,
+  // Refs
+  // https://github.com/apple-oss-distributions/Security/blob/db15acbe6a7f257a859ad9a3bb86097bfe0679d9/OSX/sec/Security/SecTrustStore.c#L196-L207
+
+  if (trust_settings_result == kSecTrustSettingsResultDeny) {
+    return TrustStatus::DISTRUSTED;
+  }
+
+  // From
+  // https://source.chromium.org/chromium/chromium/src/+/main:net/cert/internal/trust_store_mac.cc;l=144-146
+  // This is a bit of a hack: if the cert is self-issued allow either
+  // kSecTrustSettingsResultTrustRoot or kSecTrustSettingsResultTrustAsRoot on
+  // the basis that SecTrustSetTrustSettings should not allow creating an
+  // invalid trust record in the first place. (The spec is that
+  // kSecTrustSettingsResultTrustRoot can only be applied to root(self-signed)
+  // certs and kSecTrustSettingsResultTrustAsRoot is used for other certs.)
+  // This hack avoids having to check the signature on the cert which is slow
+  // if using the platform APIs, and may require supporting MD5 signature
+  // algorithms on some older OSX versions or locally added roots, which is
+  // undesirable in the built-in signature verifier.
+  if (is_self_issued) {
+    return (trust_settings_result == kSecTrustSettingsResultTrustRoot ||
+            trust_settings_result == kSecTrustSettingsResultTrustAsRoot)
                ? TrustStatus::TRUSTED
                : TrustStatus::UNSPECIFIED;
   }
 
-  return TrustStatus::UNSPECIFIED;
+  // kSecTrustSettingsResultTrustAsRoot can only be applied to non-root certs.
+  return (trust_settings_result == kSecTrustSettingsResultTrustAsRoot)
+             ? TrustStatus::TRUSTED
+             : TrustStatus::UNSPECIFIED;
 }
 
 TrustStatus IsTrustSettingsTrustedForPolicy(CFArrayRef trust_settings,
@@ -447,6 +459,17 @@ bool IsCertificateTrustValid(SecCertificateRef ref) {
       CFArrayCreateMutable(nullptr, 1, &kCFTypeArrayCallBacks);
   CFArraySetValueAtIndex(subj_certs, 0, ref);
 
+  // SecTrustEvaluateWithError is used to check whether an individual
+  // certificate is trusted by the system — not to validate it for a
+  // specific role (server, intermediate, etc.). We just need a minimal
+  // policy that guarantees the certificate can be chained to a known
+  // trust anchor while filtering out irrelevant certificates.
+  //
+  // Refs
+  // https://github.com/apple-oss-distributions/Security/blob/db15acbe6a7f257a859ad9a3bb86097bfe0679d9/OSX/sec/Security/SecPolicy.c#L1855-L1890
+  // SecPolicyCreateSSL (both mark EKU optional):
+  //   server=true  -> BasicX509 + serverAuth + anyExtendedKeyUsage + SGC
+  //   server=false -> BasicX509 + clientAuth + anyExtendedKeyUsage
   SecPolicyRef policy = SecPolicyCreateSSL(false, nullptr);
   OSStatus ortn =
       SecTrustCreateWithCertificates(subj_certs, policy, &sec_trust);
@@ -516,6 +539,21 @@ bool IsCertificateTrustedForPolicy(X509* cert, SecCertificateRef ref) {
   return false;
 }
 
+// Checks if a certificate has expired.
+// Returns true if the certificate's notAfter date is in the past.
+static bool IsCertificateExpired(X509* cert) {
+  // X509_cmp_current_time returns:
+  // -1 if the time is in the past (expired)
+  //  0 if there was an error
+  //  1 if the time is in the future (not yet expired)
+  ASN1_TIME* not_after = X509_get_notAfter(cert);
+  if (not_after == nullptr) {
+    return false;
+  }
+  int cmp = X509_cmp_current_time(not_after);
+  return cmp < 0;
+}
+
 void ReadMacOSKeychainCertificates(
     std::vector<X509*>* system_root_certificates_X509) {
   CFTypeRef search_keys[] = {kSecClass, kSecMatchLimit, kSecReturnRef};
@@ -543,6 +581,10 @@ void ReadMacOSKeychainCertificates(
 
   CFIndex count = CFArrayGetCount(curr_anchors);
 
+  // Track seen certificates to detect duplicates (same cert in multiple
+  // keychains).
+  std::set<X509*, X509Less> seen_certs;
+
   for (int i = 0; i < count; ++i) {
     SecCertificateRef cert_ref = reinterpret_cast<SecCertificateRef>(
         const_cast<void*>(CFArrayGetValueAtIndex(curr_anchors, i)));
@@ -568,11 +610,28 @@ void ReadMacOSKeychainCertificates(
     }
 
     bool is_valid = IsCertificateTrustedForPolicy(cert, cert_ref);
-    if (is_valid) {
-      system_root_certificates_X509->emplace_back(cert);
-    } else {
+    if (!is_valid) {
+      X509_free(cert);
+      continue;
+    }
+
+    // Skip duplicate certificates.
+    auto [it, inserted] = seen_certs.insert(cert);
+    if (!inserted) {
+      X509_free(cert);
+      continue;
+    }
+
+    // Skip expired certificates.
+    if (IsCertificateExpired(cert)) {
+      per_process::Debug(DebugCategory::CRYPTO,
+                         "Skipping expired system certificate\n");
+      seen_certs.erase(it);
       X509_free(cert);
+      continue;
     }
+
+    system_root_certificates_X509->emplace_back(cert);
   }
   CFRelease(curr_anchors);
 }

test/fixtures/keys/expired-root-cert.pem

@@ -0,0 +1,20 @@
+-----BEGIN CERTIFICATE-----
+MIIDSzCCAjOgAwIBAgIUJmC4/h9L3c8Arrxyo/+O73Owt9AwDQYJKoZIhvcNAQEL
+BQAwNTEhMB8GA1UEAwwYTm9kZUpTLVRlc3QtRXhwaXJlZC1Sb290MRAwDgYDVQQK
+DAdOb2RlLmpzMB4XDTIwMDEwMTAwMDAwMFoXDTIwMTIzMTIzNTk1OVowNTEhMB8G
+A1UEAwwYTm9kZUpTLVRlc3QtRXhwaXJlZC1Sb290MRAwDgYDVQQKDAdOb2RlLmpz
+MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEArkeWKdWP/YG89KiMCTYe
+9Q3DSMwzKeYrjtK5FeVxSHqo5za01VYCYsHpmTZFTM5P6nU6c889vZ0+C5lS1QAa
+JLgkUYpMMTk7ycAO0VKYugB9vHI3mQUH3Fkk7cilq5r1KyR1RqaGx7xhe49iFyXo
+zIE7C9fAQ4/DhqkuZlCnFg9PQLzKikgRdUUZsR/kvEAK30YyCcVs5po5oTGZB1VB
+gRoNYLPqSBCUtgfHEw2EIqRNQtgVwEuJVEclwbijkgIm4NFDCFKixbVE8KQFAuxr
+SFO8msDonubXJhCXZkNeh+diBMl7lglZEQU8o6ax0lqrV8mCPselXc0ipR/eD6aG
+XQIDAQABo1MwUTAdBgNVHQ4EFgQUN+lQVSG+5zPWNm/XiMcDMk9GVnowHwYDVR0j
+BBgwFoAUN+lQVSG+5zPWNm/XiMcDMk9GVnowDwYDVR0TAQH/BAUwAwEB/zANBgkq
+hkiG9w0BAQsFAAOCAQEASj1j/atg8qr3aZEsukmFaBf7eu4c0O0WwogTwCaSAEtn
+6et9G9rbg++X3JDHBv2PKiYdhSKTz2Kdsaau0fw0fmDkrFB5fwvzi6JU+LZ0Q8Ur
+jPlcks8sIdByX9mAbmf0Hur1qkPBoOm4BQNHrhEA+ExD8jbTAUukAOwH0mZV9ZzC
+B/7qVXwV6JmvdLXKDeinScnu1AIcYJVPUFEO2ZSkl+4XsdFogN+t4ryisosgFCtL
+AWkxA2/InBPBncDOUiceagJQdkGiyhsySxj/niLe8XUUo0p6/bYgqV7loJV6qkDA
+6KPH1RyigZMJ0SxNvj5oW1wcS3N2i/uX5csq9/w2nA==
+-----END CERTIFICATE-----

test/fixtures/keys/expired-root-key.pem

@@ -0,0 +1,28 @@
+-----BEGIN PRIVATE KEY-----
+MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQCuR5Yp1Y/9gbz0
+qIwJNh71DcNIzDMp5iuO0rkV5XFIeqjnNrTVVgJiwemZNkVMzk/qdTpzzz29nT4L
+mVLVABokuCRRikwxOTvJwA7RUpi6AH28cjeZBQfcWSTtyKWrmvUrJHVGpobHvGF7
+j2IXJejMgTsL18BDj8OGqS5mUKcWD09AvMqKSBF1RRmxH+S8QArfRjIJxWzmmjmh
+MZkHVUGBGg1gs+pIEJS2B8cTDYQipE1C2BXAS4lURyXBuKOSAibg0UMIUqLFtUTw
+pAUC7GtIU7yawOie5tcmEJdmQ16H52IEyXuWCVkRBTyjprHSWqtXyYI+x6VdzSKl
+H94PpoZdAgMBAAECggEAAoFI6UUGktBAlQuvJ5q9iywteGhm+90xFxZ0TppDrJUG
+xHwG0WIxGpZK80bSbC4y+92/f1alPop6D9SeWi1sMsbqzrk8KyD1eQrnq56ST2oe
+ZI0Hu41U9ZfabgiKSRMrHvmhLejK2ygcBpijAk4rMHVTEfKB8vaoCtF3t0TFgg2k
+MkliPU9hCkrr17aYZSw8R2HiTM8hqixm7GrTihZzGFUa+N4NHZEbVkLQpCnifBly
+1ZyNL03FDSBClYRGvp2W6qO8uUi3cQuXyfXhoeHZx5AW+pVsmmZsxhNnyeJwCDcU
++WgxU9uNeHWEYwVg3fTP3LLgRhnVBr3UuYI4GtWBiQKBgQDhRC9cTlOG9SrgHPZT
+reZNDQ9hhrcb1kbo1yJBIHFTihv0UIS6S36PmN+OgshbF5+G8Y8fvJJ8vuzMpDfC
+GNR53F6DHH/bngv3lgws/6F0X41TrzOA6+MkA4D6s5uGscSODCGuxkwjuZuKQzwo
+zAhx8t6RX/w1k4uXFPfYgX5vBwKBgQDGDppVCyRGpijdAdXVmmiQz/7q9M6YdFDX
+53pt8CIdCfYiT3+iOwPqHAddQdwVewC3WrEZ7S3PdxpKgDDpYkJ0+NtHhlYdGLR9
+Ti6Cvmfc+r8yYnmEAqIXAeac5A4MYYTh+FadgtzqVPhp8Q61xkXFwajo/tV1EcSt
+/A+rC0XiewKBgQCqniBZA6JUJ8FvucAApUg3t9qcfZKW7PcMSFXTiiULpyGBLLM6
+/w8+6AT7RadHB192r+M9oHA7N8jXPtJUmsXj/rs/Bwj4aH6b6fQS6RN6txyt85dI
+4GFL17OLLxpvLJm5FQs1+0+UB3L9h+s64z7KP6+/4DmAwt4JcoI+Y+ZFZQKBgHbE
+YwAEgmhrU63UX+qLgZD1aaRz0T/S4HfYM66hhZNsWdERYzRht2M4E6J00AmBjVhm
+ZjVp6UKz5WwvmyUY60lBwh0ODa29Ft7ddz6n95ioNOd97eifu5uYZDZI+7Oo9wqa
+5TXnN5q+AYlmKLAQid6g1y2BQ3fEg/DhanPjerDpAoGBAM7Sm4E4UMOCzHFCHehh
+bTzfz8oNGpkWI+5UctXZOkzQ3YVtTwB/+b5nuG7D5EI8mRGHmhXerkM2uAmShmWl
+a9Xyo9nswhDjanBcji+ZWIXFqOodclK4/3VE+VnbzlZMqpGnxUl88wWeBXEualZ0
+Z3lq9DNf/Fywlcl69thCNXQs
+-----END PRIVATE KEY-----

test/fixtures/keys/selfsigned-no-result-root-cert.pem

@@ -0,0 +1,20 @@
+-----BEGIN CERTIFICATE-----
+MIIDUTCCAjmgAwIBAgIUapTwHjAOdsC1qQUcKRKprxW7GvswDQYJKoZIhvcNAQEL
+BQAwNzEjMCEGA1UEAwwaTm9kZUpTLVRlc3QtTm8tUmVzdWx0LVJvb3QxEDAOBgNV
+BAoMB05vZGUuanMwIBcNMjYwNDA0MTgzMjUyWhgPMjEyNjAzMTExODMyNTJaMDcx
+IzAhBgNVBAMMGk5vZGVKUy1UZXN0LU5vLVJlc3VsdC1Sb290MRAwDgYDVQQKDAdO
+b2RlLmpzMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAomij2nGU2Lg0
+sD9OitlQlIAC6QEm3ziS8c3yX+wIhV/sZ/mRFt1cvFfIX7LFYl6zzDMI0Wgho422
+H9T4I5RRZ2Ms6IZ1e6+ERlqWDm3pS2nFglOR3AA1QUDt0pSfrtQjIc2yxeTMP7EO
+/LRZuGp1VOrR2kDP9JZC0+WLBBvtz/yFUICdrfDnm2d9b632Gl3CU/GiotMvt+oZ
+8J3WEh4y0vVdIwzMLV/jhoIgi0e9sZzSkB4ZmHCBOwl4asj8Yj8PS4q4AvXH2jms
+HmrsxUezsRrw9seccisexcC+akrJ4s1cqGXmi6MxqP3FO6yhSBdge6fMSNAETZ70
+qKL/Kk1t6QIDAQABo1MwUTAdBgNVHQ4EFgQULJNqn+YwH/flAsr8460+8ej1XQkw
+HwYDVR0jBBgwFoAULJNqn+YwH/flAsr8460+8ej1XQkwDwYDVR0TAQH/BAUwAwEB
+/zANBgkqhkiG9w0BAQsFAAOCAQEAMl8+Y1xRagBBKOluPcvTyV5BTleXLtF95wsj
+6SCTsSzmJiYQtBZA/pSoTS8+gEQb9hjN6dbYRDVY2pGyLrEVI/YE4zr4+Ug8vBXw
+UnZx4a76bUiT8iC7rBsqSeui7R56lbPQlxYjEKyX3oZgW9WzZ9NT3z9/u3mfTrW/
+TculuifSQHAi1X5r9IXFFABMyD8gDtQSfG+0e1E9KLyCMO3p8H/snz1hXIAUFnBD
+2b5QYiQRxUN2aO4PqJakxhDN844Vv6O+vQX618Fn+MqaL6qyPzJRo9qJe70/5xIu
+4Xs3ajp8Y3c5bb9vGtgLWsb3eUJ+AcZ2kttaBaIgiKqk4hTrDA==
+-----END CERTIFICATE-----

test/fixtures/keys/selfsigned-no-result-root-key.pem

@@ -0,0 +1,28 @@
+-----BEGIN PRIVATE KEY-----
+MIIEvAIBADANBgkqhkiG9w0BAQEFAASCBKYwggSiAgEAAoIBAQCiaKPacZTYuDSw
+P06K2VCUgALpASbfOJLxzfJf7AiFX+xn+ZEW3Vy8V8hfssViXrPMMwjRaCGjjbYf
+1PgjlFFnYyzohnV7r4RGWpYObelLacWCU5HcADVBQO3SlJ+u1CMhzbLF5Mw/sQ78
+tFm4anVU6tHaQM/0lkLT5YsEG+3P/IVQgJ2t8OebZ31vrfYaXcJT8aKi0y+36hnw
+ndYSHjLS9V0jDMwtX+OGgiCLR72xnNKQHhmYcIE7CXhqyPxiPw9LirgC9cfaOawe
+auzFR7OxGvD2x5xyKx7FwL5qSsnizVyoZeaLozGo/cU7rKFIF2B7p8xI0ARNnvSo
+ov8qTW3pAgMBAAECggEAFFg1W7zqCh1GhGQ5y4zzhNa7BpiIAmMiN+Y2RtaDhBRY
+bKHyZJcwRxZcBG5EPvwMBo1yEla6uAlILO6kcu3hJf63dnK5hC1KzaFgC3NbS9Yh
+JsqfNUcAD3+PCy0RCnk1OXEnbut9ZpEghn7Kf8kzj4Km7RySBa/5CSBHwikEKQ85
+jO4dvtx0wDYbbvTrIcCKS74KoM10hioR3478m/6sx3sYau1bOUAUgvBBQc4WmdwJ
+Mxxz2ZqTQp4IyTGLIIS7vhyJLG9dcb1fapuMXjRYUn9i2ku9mbB+p/n514RLS+2W
+iS9IokYY2NwYv5Ue2cxvtmE5uVqx7Hkm4FEeFQJCHQKBgQDWLaT5mWRZaIMGyoas
+lmqqYM0gLlb9E9mxyPMc/BV9flV/8dTzSVarvGDJtefVQB4LHBOCXQYj9ZiqUsND
+5b7ZH/sVBNBfm/FR6rebKYXc0HOzhmCsoiuVSMRmpLZsWVpeX2BzCVJzES5ejm0o
+t3nGFzzLyOwWxrvp5zcGLwsNWwKBgQDCHyMHQ052QBujBs6PICd6pQ+VtcTSmXbX
+TUcUFptzdRRIZdnuM2r2ZhUcBV1RSrHFfpzeyMuTW1+sU4I+ggOeM15ui1XRJjHI
+FId6Ahj35QR/Jqrlyhde5g/xElY7/4Vlvw27DlrqrSa8QzsM32J8RoQB3SppuPlz
+lnZoB5uBCwKBgBuYbfUq6l8KtDcfyRJbnwqsxkErN1IMSLQ7a/eEE1DEAkgl5IYk
+IOKntuDGa0RyqmxMBcd6LNxdPHpVh4ssAtb+497la+OluAYR8+4t/21f/khXPAWC
+L5NgeM2w00BKkvYt28N2pATnZc4RE8d3PF1liRPIo4KbwIJ2pARL82SZAoGAW8cu
+33sx+HR83IoWVNLl93VctfJ3eP53knmF1niNzHuZOFV3QMhslMxUxKfAo/OFsxMW
+hbo3jZbQ1/+vf3Am17//sJIN49GEDc2u879UILfVdWxJtlTi0cpB1T9PKBS59A3t
+Jvg1geiVfMLog0CGJq2MMfln2Q5MWhrUJoEaQ1sCgYBGIrbT0Tpb/09w1qZ21kDk
+4iBAnKiy3KAeXlq3bVpfV+u/Ah6K7nvKIFtende/FoKFhiIu7YAFkOhIWRvBsRZ0
+KWt2rlHfjlk+iZN9NqaWQqfmUvemiuBxie7ddFmMJwK2YYmmSI1hnTQlVLHuqMs6
+fDfGqsm1Edj00rLA7f6ylg==
+-----END PRIVATE KEY-----

test/system-ca/README.md

@@ -18,6 +18,34 @@ security add-certificates \
 security add-certificates \
   -k /Users/$USER/Library/Keychains/login.keychain-db \
   test/fixtures/keys/non-trusted-intermediate-ca.pem
+security add-trusted-cert \
+  -k /Users/$USER/Library/Keychains/login.keychain-db \
+  test/fixtures/keys/expired-root-cert.pem
+# Self-signed cert with trust settings that lack kSecTrustSettingsResult.
+security add-trusted-cert \
+  -k /Users/$USER/Library/Keychains/login.keychain-db \
+  test/fixtures/keys/selfsigned-no-result-root-cert.pem
+security trust-settings-export /tmp/node-trust-settings.plist
+CERT_SHA1=$(openssl x509 \
+  -in test/fixtures/keys/selfsigned-no-result-root-cert.pem \
+  -fingerprint -sha1 -noout | sed 's/.*=//;s/://g')
+/usr/libexec/PlistBuddy \
+  -c "Delete :trustList:${CERT_SHA1}:trustSettings" \
+  /tmp/node-trust-settings.plist
+/usr/libexec/PlistBuddy \
+  -c "Add :trustList:${CERT_SHA1}:trustSettings array" \
+  /tmp/node-trust-settings.plist
+/usr/libexec/PlistBuddy \
+  -c "Add :trustList:${CERT_SHA1}:trustSettings:0 dict" \
+  /tmp/node-trust-settings.plist
+security trust-settings-import /tmp/node-trust-settings.plist
+rm /tmp/node-trust-settings.plist
+# Duplicate cert in a second keychain
+security create-keychain -p "test" /tmp/node-test-dup.keychain
+security add-certificates \
+  -k /tmp/node-test-dup.keychain \
+  test/fixtures/keys/fake-startcom-root-cert.pem
+security list-keychains -d user -s login.keychain-db /tmp/node-test-dup.keychain
 ```
 
 **Removing the certificate**
@@ -29,6 +57,12 @@ security delete-certificate -c 'NodeJS-Test-Intermediate-CA' \
   -t /Users/$USER/Library/Keychains/login.keychain-db
 security delete-certificate -c 'NodeJS-Non-Trusted-Test-Intermediate-CA' \
   -t /Users/$USER/Library/Keychains/login.keychain-db
+security delete-certificate -c 'NodeJS-Test-Expired-Root' \
+  -t /Users/$USER/Library/Keychains/login.keychain-db
+security delete-certificate -c 'NodeJS-Test-No-Result-Root' \
+  -t /Users/$USER/Library/Keychains/login.keychain-db
+security list-keychains -d user -s login.keychain-db
+security delete-keychain /tmp/node-test-dup.keychain
 ```
 
 ## Windows