@@ -342,7 +342,10 @@ TrustStatus IsTrustDictionaryTrustedForPolicy(CFDictionaryRef trust_dict,
342342 CFStringRef policy_oid = reinterpret_cast <CFStringRef>(
343343 const_cast <void *>(CFDictionaryGetValue (policy_dict, kSecPolicyOid )));
344344
345- if (!CFEqual (policy_oid, kSecPolicyAppleSSL )) {
345+ bool matches_ssl = CFEqual (policy_oid, kSecPolicyAppleSSL );
346+ CFRelease (policy_dict);
347+
348+ if (!matches_ssl) {
346349 return TrustStatus::UNSPECIFIED ;
347350 }
348351 }
@@ -359,35 +362,44 @@ TrustStatus IsTrustDictionaryTrustedForPolicy(CFDictionaryRef trust_dict,
359362 &trust_settings_result)) {
360363 return TrustStatus::UNSPECIFIED ;
361364 }
365+ }
362366
363- if (trust_settings_result == kSecTrustSettingsResultDeny ) {
364- return TrustStatus::DISTRUSTED ;
365- }
366-
367- // This is a bit of a hack: if the cert is self-issued allow either
368- // kSecTrustSettingsResultTrustRoot or kSecTrustSettingsResultTrustAsRoot on
369- // the basis that SecTrustSetTrustSettings should not allow creating an
370- // invalid trust record in the first place. (The spec is that
371- // kSecTrustSettingsResultTrustRoot can only be applied to root(self-signed)
372- // certs and kSecTrustSettingsResultTrustAsRoot is used for other certs.)
373- // This hack avoids having to check the signature on the cert which is slow
374- // if using the platform APIs, and may require supporting MD5 signature
375- // algorithms on some older OSX versions or locally added roots, which is
376- // undesirable in the built-in signature verifier.
377- if (is_self_issued) {
378- return trust_settings_result == kSecTrustSettingsResultTrustRoot ||
379- trust_settings_result == kSecTrustSettingsResultTrustAsRoot
380- ? TrustStatus::TRUSTED
381- : TrustStatus::UNSPECIFIED ;
382- }
383-
384- // kSecTrustSettingsResultTrustAsRoot can only be applied to non-root certs.
385- return (trust_settings_result == kSecTrustSettingsResultTrustAsRoot )
367+ // When kSecTrustSettingsResult is absent from the trust dict,
368+ // Apple docs specify kSecTrustSettingsResultTrustRoot as the default.
369+ // Refs
370+ // https://github.com/apple-oss-distributions/Security/blob/db15acbe6a7f257a859ad9a3bb86097bfe0679d9/trust/headers/SecTrustSettings.h#L119-L122
371+ // This is also enforced at write time for self-signed certs get TrustRoot,
372+ // and non-self-signed certs cannot have an empty settings,
373+ // Refs
374+ // https://github.com/apple-oss-distributions/Security/blob/db15acbe6a7f257a859ad9a3bb86097bfe0679d9/OSX/sec/Security/SecTrustStore.c#L196-L207
375+
376+ if (trust_settings_result == kSecTrustSettingsResultDeny ) {
377+ return TrustStatus::DISTRUSTED ;
378+ }
379+
380+ // From
381+ // https://source.chromium.org/chromium/chromium/src/+/main:net/cert/internal/trust_store_mac.cc;l=144-146
382+ // This is a bit of a hack: if the cert is self-issued allow either
383+ // kSecTrustSettingsResultTrustRoot or kSecTrustSettingsResultTrustAsRoot on
384+ // the basis that SecTrustSetTrustSettings should not allow creating an
385+ // invalid trust record in the first place. (The spec is that
386+ // kSecTrustSettingsResultTrustRoot can only be applied to root(self-signed)
387+ // certs and kSecTrustSettingsResultTrustAsRoot is used for other certs.)
388+ // This hack avoids having to check the signature on the cert which is slow
389+ // if using the platform APIs, and may require supporting MD5 signature
390+ // algorithms on some older OSX versions or locally added roots, which is
391+ // undesirable in the built-in signature verifier.
392+ if (is_self_issued) {
393+ return (trust_settings_result == kSecTrustSettingsResultTrustRoot ||
394+ trust_settings_result == kSecTrustSettingsResultTrustAsRoot )
386395 ? TrustStatus::TRUSTED
387396 : TrustStatus::UNSPECIFIED ;
388397 }
389398
390- return TrustStatus::UNSPECIFIED ;
399+ // kSecTrustSettingsResultTrustAsRoot can only be applied to non-root certs.
400+ return (trust_settings_result == kSecTrustSettingsResultTrustAsRoot )
401+ ? TrustStatus::TRUSTED
402+ : TrustStatus::UNSPECIFIED ;
391403}
392404
393405TrustStatus IsTrustSettingsTrustedForPolicy (CFArrayRef trust_settings,
@@ -420,6 +432,17 @@ bool IsCertificateTrustValid(SecCertificateRef ref) {
420432 CFArrayCreateMutable (nullptr , 1 , &kCFTypeArrayCallBacks );
421433 CFArraySetValueAtIndex (subj_certs, 0 , ref);
422434
435+ // SecTrustEvaluateWithError is used to check whether an individual
436+ // certificate is trusted by the system — not to validate it for a
437+ // specific role (server, intermediate, etc.). We just need a minimal
438+ // policy that guarantees the certificate can be chained to a known
439+ // trust anchor while filtering out irrelevant certificates.
440+ //
441+ // Refs
442+ // https://github.com/apple-oss-distributions/Security/blob/db15acbe6a7f257a859ad9a3bb86097bfe0679d9/OSX/sec/Security/SecPolicy.c#L1855-L1890
443+ // SecPolicyCreateSSL (both mark EKU optional):
444+ // server=true -> BasicX509 + serverAuth + anyExtendedKeyUsage + SGC
445+ // server=false -> BasicX509 + clientAuth + anyExtendedKeyUsage
423446 SecPolicyRef policy = SecPolicyCreateSSL (false , nullptr );
424447 OSStatus ortn =
425448 SecTrustCreateWithCertificates (subj_certs, policy, &sec_trust);
@@ -489,6 +512,21 @@ bool IsCertificateTrustedForPolicy(X509* cert, SecCertificateRef ref) {
489512 return false ;
490513}
491514
515+ // Checks if a certificate has expired.
516+ // Returns true if the certificate's notAfter date is in the past.
517+ static bool IsCertificateExpired (X509 * cert) {
518+ // X509_cmp_current_time returns:
519+ // -1 if the time is in the past (expired)
520+ // 0 if there was an error
521+ // 1 if the time is in the future (not yet expired)
522+ ASN1_TIME * not_after = X509_get_notAfter (cert);
523+ if (not_after == nullptr ) {
524+ return false ;
525+ }
526+ int cmp = X509_cmp_current_time (not_after);
527+ return cmp < 0 ;
528+ }
529+
492530void ReadMacOSKeychainCertificates (
493531 std::vector<X509 *>* system_root_certificates_X509) {
494532 CFTypeRef search_keys[] = {kSecClass , kSecMatchLimit , kSecReturnRef };
@@ -516,6 +554,10 @@ void ReadMacOSKeychainCertificates(
516554
517555 CFIndex count = CFArrayGetCount (curr_anchors);
518556
557+ // Track seen certificates to detect duplicates (same cert in multiple
558+ // keychains).
559+ std::set<X509 *, X509Less> seen_certs;
560+
519561 for (int i = 0 ; i < count; ++i) {
520562 SecCertificateRef cert_ref = reinterpret_cast <SecCertificateRef>(
521563 const_cast <void *>(CFArrayGetValueAtIndex (curr_anchors, i)));
@@ -541,11 +583,28 @@ void ReadMacOSKeychainCertificates(
541583 }
542584
543585 bool is_valid = IsCertificateTrustedForPolicy (cert, cert_ref);
544- if (is_valid) {
545- system_root_certificates_X509->emplace_back (cert);
546- } else {
586+ if (!is_valid) {
587+ X509_free (cert);
588+ continue ;
589+ }
590+
591+ // Skip duplicate certificates.
592+ auto [it, inserted] = seen_certs.insert (cert);
593+ if (!inserted) {
594+ X509_free (cert);
595+ continue ;
596+ }
597+
598+ // Skip expired certificates.
599+ if (IsCertificateExpired (cert)) {
600+ per_process::Debug (DebugCategory::CRYPTO ,
601+ " Skipping expired system certificate\n "
602+ seen_certs.erase (it);
547603 X509_free (cert);
604+ continue ;
548605 }
606+
607+ system_root_certificates_X509->emplace_back (cert);
549608 }
550609 CFRelease (curr_anchors);
551610}
0 commit comments