Skip to content

Commit 3ae29ba

Browse files
committed
crypto: fix leak and improve cert enumeration
1. Fix CFRelease leak in IsTrustDictionaryTrustedForPolicy: the CFDictionaryRef returned by SecPolicyCopyProperties(policy_ref) was not released when the policy OID matched kSecPolicyAppleSSL. 2. Deduplicate certificates: SecItemCopyMatching can return the same certificate from multiple keychains. 3. Filter expired certificates.
1 parent 3637f40 commit 3ae29ba

File tree

1 file changed

+43
-4
lines changed

1 file changed

+43
-4
lines changed

src/crypto/crypto_context.cc

Lines changed: 43 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -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
}
@@ -522,6 +525,21 @@ bool IsCertificateTrustedForPolicy(X509* cert, SecCertificateRef ref) {
522525
return false;
523526
}
524527

528+
// Checks if a certificate has expired.
529+
// Returns true if the certificate's notAfter date is in the past.
530+
static bool IsCertificateExpired(X509* cert) {
531+
// X509_cmp_current_time returns:
532+
// -1 if the time is in the past (expired)
533+
// 0 if there was an error
534+
// 1 if the time is in the future (not yet expired)
535+
ASN1_TIME* not_after = X509_get_notAfter(cert);
536+
if (not_after == nullptr) {
537+
return false;
538+
}
539+
int cmp = X509_cmp_current_time(not_after);
540+
return cmp < 0;
541+
}
542+
525543
void ReadMacOSKeychainCertificates(
526544
std::vector<X509*>* system_root_certificates_X509) {
527545
CFTypeRef search_keys[] = {kSecClass, kSecMatchLimit, kSecReturnRef};
@@ -549,6 +567,10 @@ void ReadMacOSKeychainCertificates(
549567

550568
CFIndex count = CFArrayGetCount(curr_anchors);
551569

570+
// Track seen certificates to detect duplicates (same cert in multiple
571+
// keychains).
572+
std::set<X509*, X509Less> seen_certs;
573+
552574
for (int i = 0; i < count; ++i) {
553575
SecCertificateRef cert_ref = reinterpret_cast<SecCertificateRef>(
554576
const_cast<void*>(CFArrayGetValueAtIndex(curr_anchors, i)));
@@ -574,11 +596,28 @@ void ReadMacOSKeychainCertificates(
574596
}
575597

576598
bool is_valid = IsCertificateTrustedForPolicy(cert, cert_ref);
577-
if (is_valid) {
578-
system_root_certificates_X509->emplace_back(cert);
579-
} else {
599+
if (!is_valid) {
600+
X509_free(cert);
601+
continue;
602+
}
603+
604+
// Skip duplicate certificates.
605+
auto [it, inserted] = seen_certs.insert(cert);
606+
if (!inserted) {
580607
X509_free(cert);
608+
continue;
581609
}
610+
611+
// Skip expired certificates.
612+
if (IsCertificateExpired(cert)) {
613+
per_process::Debug(DebugCategory::CRYPTO,
614+
"Skipping expired system certificate\n");
615+
seen_certs.erase(it);
616+
X509_free(cert);
617+
continue;
618+
}
619+
620+
system_root_certificates_X509->emplace_back(cert);
582621
}
583622
CFRelease(curr_anchors);
584623
}

0 commit comments

Comments
 (0)