Commit 7eaf815
src: handle DER decoding errors from system certificates
When decoding certificates from the system store, it's not actually
guaranteed to succeed. In case the system returns a certificate
that cannot be decoded (might be related to SSL implementation issues),
skip them.
PR-URL: #60787
Refs: microsoft/vscode#277064
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Aditi Singh <aditisingh1400@gmail.com>
Reviewed-By: Tim Perry <pimterry@gmail.com>1 parent c2e79aa commit 7eaf815
1 file changed
Lines changed: 26 additions & 3 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -507,7 +507,11 @@ void ReadMacOSKeychainCertificates( | |||
| 507 | 507 | CFRelease(search); | |
| 508 | 508 | ||
| 509 | 509 | if (ortn) { | |
| 510 | - fprintf(stderr, "ERROR: SecItemCopyMatching failed %d\n", ortn); | ||
| 510 | + per_process::Debug(DebugCategory::CRYPTO, | ||
| 511 | + "Cannot read certificates from system because " | ||
| 512 | + "SecItemCopyMatching failed %d\n", | ||
| 513 | + ortn); | ||
| 514 | + return; | ||
| 511 | 515 | } | |
| 512 | 516 | ||
| 513 | 517 | CFIndex count = CFArrayGetCount(curr_anchors); | |
@@ -518,17 +522,29 @@ void ReadMacOSKeychainCertificates( | |||
| 518 | 522 | ||
| 519 | 523 | CFDataRef der_data = SecCertificateCopyData(cert_ref); | |
| 520 | 524 | if (!der_data) { | |
| 521 | - fprintf(stderr, "ERROR: SecCertificateCopyData failed\n"); | ||
| 525 | + per_process::Debug(DebugCategory::CRYPTO, | ||
| 526 | + "Skipping read of a system certificate " | ||
| 527 | + "because SecCertificateCopyData failed\n"); | ||
| 522 | 528 | continue; | |
| 523 | 529 | } | |
| 524 | 530 | auto data_buffer_pointer = CFDataGetBytePtr(der_data); | |
| 525 | 531 | ||
| 526 | 532 | X509* cert = | |
| 527 | 533 | d2i_X509(nullptr, &data_buffer_pointer, CFDataGetLength(der_data)); | |
| 528 | 534 | CFRelease(der_data); | |
| 535 | + | ||
| 536 | + if (cert == nullptr) { | ||
| 537 | + per_process::Debug(DebugCategory::CRYPTO, | ||
| 538 | + "Skipping read of a system certificate " | ||
| 539 | + "because decoding failed\n"); | ||
| 540 | + continue; | ||
| 541 | + } | ||
| 542 | + | ||
| 529 | 543 | bool is_valid = IsCertificateTrustedForPolicy(cert, cert_ref); | |
| 530 | 544 | if (is_valid) { | |
| 531 | 545 | system_root_certificates_X509->emplace_back(cert); | |
| 546 | + } else { | ||
| 547 | + X509_free(cert); | ||
| 532 | 548 | } | |
| 533 | 549 | } | |
| 534 | 550 | CFRelease(curr_anchors); | |
@@ -638,7 +654,14 @@ void GatherCertsForLocation(std::vector<X509*>* vector, | |||
| 638 | 654 | reinterpret_cast<const unsigned char*>(cert_from_store->pbCertEncoded); | |
| 639 | 655 | const size_t cert_size = cert_from_store->cbCertEncoded; | |
| 640 | 656 | ||
| 641 | - vector->emplace_back(d2i_X509(nullptr, &cert_data, cert_size)); | ||
| 657 | + X509* x509 = d2i_X509(nullptr, &cert_data, cert_size); | ||
| 658 | + if (x509 == nullptr) { | ||
| 659 | + per_process::Debug(DebugCategory::CRYPTO, | ||
| 660 | + "Skipping read of a system certificate " | ||
| 661 | + "because decoding failed\n"); | ||
| 662 | + } else { | ||
| 663 | + vector->emplace_back(x509); | ||
| 664 | + } | ||
| 642 | 665 | } | |
| 643 | 666 | } | |
| 644 | 667 | ||
0 commit comments