From 5fc987f210cb555c53e4d1696fb2205d6d70f3e9 Mon Sep 17 00:00:00 2001 From: Ryan Sleevi Date: Mon, 13 Jan 2020 22:23:34 +0100 Subject: [Backport] Security bug 1033260 Manual backport of patch: macOS 10.13+ switched to using trustd over XPC to verify certificates, but can (for unknown reasons) occasionally fail, affecting the legacy/non-built-in verifier (CertVerifyProcMac). When this happens, don't re-validate the same certificate multiple times, since trustd will keep complaining. TBR=mattm@chromium.org (cherry picked from commit a7bdc8bfd0fdf679e8667e16c489c592ca973f37) Bug: 1033260 Commit-Queue: Ryan Sleevi Cr-Original-Commit-Position: refs/heads/master@{#724437} Reviewed-by: Ryan Sleevi Cr-Commit-Position: refs/branch-heads/3945@{#1006} Cr-Branched-From: e4635fff7defbae0f9c29e798349f6fc0cce4b1b-refs/heads/master@{#706915} Change-Id: I57ad4ebeac4059f232624b6af4345021ea3fbcfb Reviewed-by: Michal Klocek --- chromium/net/cert/cert_verify_proc_mac.cc | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/chromium/net/cert/cert_verify_proc_mac.cc b/chromium/net/cert/cert_verify_proc_mac.cc index ea7407985cc..fd6902075d3 100644 --- a/chromium/net/cert/cert_verify_proc_mac.cc +++ b/chromium/net/cert/cert_verify_proc_mac.cc @@ -144,6 +144,7 @@ CertStatus CertStatusFromOSStatus(OSStatus status) { // TODO(wtc): Should we add CERT_STATUS_WRONG_USAGE? return CERT_STATUS_INVALID; + case errSecInternalError: case CSSMERR_APPLETP_CRL_BAD_URI: case CSSMERR_APPLETP_IDP_FAIL: return CERT_STATUS_INVALID; @@ -513,6 +514,10 @@ CRLSetResult CheckRevocationWithCRLSet(CFArrayRef chain, CRLSet* crl_set) { // |verified_chain|, and |chain_info| with the verification results. On // failure, no output parameters are modified. // +// WARNING: Beginning with macOS 10.13, if |trust_result| is equal to +// kSecTrustResultInvalid, any further accesses via SecTrust APIs to |trust_ref| +// may invalidate |chain_info|. +// // Note: An OK return does not mean that |cert_array| is trusted, merely that // verification was performed successfully. // @@ -614,6 +619,8 @@ int BuildAndEvaluateSecTrustRef(CFArrayRef cert_array, return NetErrorFromOSStatus(status); CFArrayRef tmp_verified_chain = NULL; CSSM_TP_APPLE_EVIDENCE_INFO* tmp_chain_info; + // WARNING: Beginning with OS X 10.13, |tmp_chain_info| may be freed by any + // other accesses via SecTrust APIs to |tmp_trust|. status = SecTrustGetResult(tmp_trust, &tmp_trust_result, &tmp_verified_chain, &tmp_chain_info); if (status) @@ -905,8 +912,21 @@ int VerifyWithGivenFlags(X509Certificate* cert, bool policy_fail_already_mapped = false; bool weak_key_or_signature_algorithm = false; + // As of macOS 10.13, if |trust_result| (from SecTrustGetResult) returns + // kSecTrustResultInvalid, subsequent invocations of SecTrust APIs may + // result in revalidating the SecTrust, invalidating the pointers such + // as |chain_info|. In releases earlier than 10.13, this call would have + // additional information, except that information is unused and + // irrelevant if the result was invalid, so the placeholder + // errSecInternalError is fine. + OSStatus cssm_result = errSecInternalError; + if (trust_result != kSecTrustResultInvalid) { + status = SecTrustGetCssmResultCode(trust_ref, &cssm_result); + if (status) + return NetErrorFromOSStatus(status); + } + // Evaluate the results - OSStatus cssm_result; switch (trust_result) { case kSecTrustResultUnspecified: case kSecTrustResultProceed: @@ -923,9 +943,6 @@ int VerifyWithGivenFlags(X509Certificate* cert, case kSecTrustResultRecoverableTrustFailure: // Certificate chain has a failure that can be overridden by the user. - status = SecTrustGetCssmResultCode(trust_ref, &cssm_result); - if (status) - return NetErrorFromOSStatus(status); if (cssm_result == CSSMERR_TP_VERIFY_ACTION_FAILED) { policy_failed = true; } else { -- cgit v1.2.1