@@ -369,7 +369,10 @@ TrustStatus IsTrustDictionaryTrustedForPolicy(CFDictionaryRef trust_dict,
369369 CFStringRef policy_oid = reinterpret_cast <CFStringRef>(
370370 const_cast <void *>(CFDictionaryGetValue (policy_dict, kSecPolicyOid )));
371371
372- if (!CFEqual (policy_oid, kSecPolicyAppleSSL )) {
372+ bool matches_ssl = CFEqual (policy_oid, kSecPolicyAppleSSL );
373+ CFRelease (policy_dict);
374+
375+ if (!matches_ssl) {
373376 return TrustStatus::UNSPECIFIED;
374377 }
375378 }
@@ -386,35 +389,44 @@ TrustStatus IsTrustDictionaryTrustedForPolicy(CFDictionaryRef trust_dict,
386389 &trust_settings_result)) {
387390 return TrustStatus::UNSPECIFIED;
388391 }
392+ }
389393
390- if (trust_settings_result == kSecTrustSettingsResultDeny ) {
391- return TrustStatus::DISTRUSTED;
392- }
393-
394- // This is a bit of a hack: if the cert is self-issued allow either
395- // kSecTrustSettingsResultTrustRoot or kSecTrustSettingsResultTrustAsRoot on
396- // the basis that SecTrustSetTrustSettings should not allow creating an
397- // invalid trust record in the first place. (The spec is that
398- // kSecTrustSettingsResultTrustRoot can only be applied to root(self-signed)
399- // certs and kSecTrustSettingsResultTrustAsRoot is used for other certs.)
400- // This hack avoids having to check the signature on the cert which is slow
401- // if using the platform APIs, and may require supporting MD5 signature
402- // algorithms on some older OSX versions or locally added roots, which is
403- // undesirable in the built-in signature verifier.
404- if (is_self_issued) {
405- return trust_settings_result == kSecTrustSettingsResultTrustRoot ||
406- trust_settings_result == kSecTrustSettingsResultTrustAsRoot
407- ? TrustStatus::TRUSTED
408- : TrustStatus::UNSPECIFIED;
409- }
410-
411- // kSecTrustSettingsResultTrustAsRoot can only be applied to non-root certs.
412- return (trust_settings_result == kSecTrustSettingsResultTrustAsRoot )
394+ // When kSecTrustSettingsResult is absent from the trust dict,
395+ // Apple docs specify kSecTrustSettingsResultTrustRoot as the default.
396+ // Refs
397+ // https://github.com/apple-oss-distributions/Security/blob/db15acbe6a7f257a859ad9a3bb86097bfe0679d9/trust/headers/SecTrustSettings.h#L119-L122
398+ // This is also enforced at write time for self-signed certs get TrustRoot,
399+ // and non-self-signed certs cannot have an empty settings,
400+ // Refs
401+ // https://github.com/apple-oss-distributions/Security/blob/db15acbe6a7f257a859ad9a3bb86097bfe0679d9/OSX/sec/Security/SecTrustStore.c#L196-L207
402+
403+ if (trust_settings_result == kSecTrustSettingsResultDeny ) {
404+ return TrustStatus::DISTRUSTED;
405+ }
406+
407+ // From
408+ // https://source.chromium.org/chromium/chromium/src/+/main:net/cert/internal/trust_store_mac.cc;l=144-146
409+ // This is a bit of a hack: if the cert is self-issued allow either
410+ // kSecTrustSettingsResultTrustRoot or kSecTrustSettingsResultTrustAsRoot on
411+ // the basis that SecTrustSetTrustSettings should not allow creating an
412+ // invalid trust record in the first place. (The spec is that
413+ // kSecTrustSettingsResultTrustRoot can only be applied to root(self-signed)
414+ // certs and kSecTrustSettingsResultTrustAsRoot is used for other certs.)
415+ // This hack avoids having to check the signature on the cert which is slow
416+ // if using the platform APIs, and may require supporting MD5 signature
417+ // algorithms on some older OSX versions or locally added roots, which is
418+ // undesirable in the built-in signature verifier.
419+ if (is_self_issued) {
420+ return (trust_settings_result == kSecTrustSettingsResultTrustRoot ||
421+ trust_settings_result == kSecTrustSettingsResultTrustAsRoot )
413422 ? TrustStatus::TRUSTED
414423 : TrustStatus::UNSPECIFIED;
415424 }
416425
417- return TrustStatus::UNSPECIFIED;
426+ // kSecTrustSettingsResultTrustAsRoot can only be applied to non-root certs.
427+ return (trust_settings_result == kSecTrustSettingsResultTrustAsRoot )
428+ ? TrustStatus::TRUSTED
429+ : TrustStatus::UNSPECIFIED;
418430}
419431
420432TrustStatus IsTrustSettingsTrustedForPolicy (CFArrayRef trust_settings,
@@ -447,6 +459,17 @@ bool IsCertificateTrustValid(SecCertificateRef ref) {
447459 CFArrayCreateMutable (nullptr , 1 , &kCFTypeArrayCallBacks );
448460 CFArraySetValueAtIndex (subj_certs, 0 , ref);
449461
462+ // SecTrustEvaluateWithError is used to check whether an individual
463+ // certificate is trusted by the system — not to validate it for a
464+ // specific role (server, intermediate, etc.). We just need a minimal
465+ // policy that guarantees the certificate can be chained to a known
466+ // trust anchor while filtering out irrelevant certificates.
467+ //
468+ // Refs
469+ // https://github.com/apple-oss-distributions/Security/blob/db15acbe6a7f257a859ad9a3bb86097bfe0679d9/OSX/sec/Security/SecPolicy.c#L1855-L1890
470+ // SecPolicyCreateSSL (both mark EKU optional):
471+ // server=true -> BasicX509 + serverAuth + anyExtendedKeyUsage + SGC
472+ // server=false -> BasicX509 + clientAuth + anyExtendedKeyUsage
450473 SecPolicyRef policy = SecPolicyCreateSSL (false , nullptr );
451474 OSStatus ortn =
452475 SecTrustCreateWithCertificates (subj_certs, policy, &sec_trust);
@@ -516,6 +539,21 @@ bool IsCertificateTrustedForPolicy(X509* cert, SecCertificateRef ref) {
516539 return false ;
517540}
518541
542+ // Checks if a certificate has expired.
543+ // Returns true if the certificate's notAfter date is in the past.
544+ static bool IsCertificateExpired (X509* cert) {
545+ // X509_cmp_current_time returns:
546+ // -1 if the time is in the past (expired)
547+ // 0 if there was an error
548+ // 1 if the time is in the future (not yet expired)
549+ ASN1_TIME* not_after = X509_get_notAfter (cert);
550+ if (not_after == nullptr ) {
551+ return false ;
552+ }
553+ int cmp = X509_cmp_current_time (not_after);
554+ return cmp < 0 ;
555+ }
556+
519557void ReadMacOSKeychainCertificates (
520558 std::vector<X509*>* system_root_certificates_X509) {
521559 CFTypeRef search_keys[] = {kSecClass , kSecMatchLimit , kSecReturnRef };
@@ -543,6 +581,10 @@ void ReadMacOSKeychainCertificates(
543581
544582 CFIndex count = CFArrayGetCount (curr_anchors);
545583
584+ // Track seen certificates to detect duplicates (same cert in multiple
585+ // keychains).
586+ std::set<X509*, X509Less> seen_certs;
587+
546588 for (int i = 0 ; i < count; ++i) {
547589 SecCertificateRef cert_ref = reinterpret_cast <SecCertificateRef>(
548590 const_cast <void *>(CFArrayGetValueAtIndex (curr_anchors, i)));
@@ -568,11 +610,28 @@ void ReadMacOSKeychainCertificates(
568610 }
569611
570612 bool is_valid = IsCertificateTrustedForPolicy (cert, cert_ref);
571- if (is_valid) {
572- system_root_certificates_X509->emplace_back (cert);
573- } else {
613+ if (!is_valid) {
614+ X509_free (cert);
615+ continue ;
616+ }
617+
618+ // Skip duplicate certificates.
619+ auto [it, inserted] = seen_certs.insert (cert);
620+ if (!inserted) {
621+ X509_free (cert);
622+ continue ;
623+ }
624+
625+ // Skip expired certificates.
626+ if (IsCertificateExpired (cert)) {
627+ per_process::Debug (DebugCategory::CRYPTO,
628+ " Skipping expired system certificate\n "
629+ seen_certs.erase (it);
574630 X509_free (cert);
631+ continue ;
575632 }
633+
634+ system_root_certificates_X509->emplace_back (cert);
576635 }
577636 CFRelease (curr_anchors);
578637}
0 commit comments