Account for the OS returning an empty certificate chain.

net/cert/cert_verify_proc_mac.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/cert/cert_verify_proc_mac.h"
 
 #include <CommonCrypto/CommonDigest.h>
 #include <CoreServices/CoreServices.h>
 #include <Security/Security.h>
 
@@ skipping 158 matching lines @@
       (flags & CertVerifier::VERIFY_REV_CHECKING_ENABLED_EV_ONLY),
       local_policies);
   if (status)
     return status;
 
   policies->reset(local_policies.release());
   return noErr;
 }
 
 // Stores the constructed certificate chain |cert_chain| and information about
-// the signature algorithms used into |*verify_result|. If the leaf cert in
-// |cert_chain| contains a weak (MD2, MD4, MD5, SHA-1) signature, stores that
-// in |*leaf_is_weak|.
+// the signature algorithms used into |*verify_result|. |cert_chain| must not be
+// empty.
 void GetCertChainInfo(CFArrayRef cert_chain,
                       CSSM_TP_APPLE_EVIDENCE_INFO* chain_info,
-                      CertVerifyResult* verify_result,
-                      bool* leaf_is_weak) {
-  *leaf_is_weak = false;
-  verify_result->verified_cert = nullptr;
+                      CertVerifyResult* verify_result) {
+  DCHECK_LT(0, CFArrayGetCount(cert_chain));
+
   verify_result->has_md2 = false;
   verify_result->has_md4 = false;
   verify_result->has_md5 = false;
   verify_result->has_sha1 = false;
 
   SecCertificateRef verified_cert = NULL;
   std::vector<SecCertificateRef> verified_chain;
   for (CFIndex i = 0, count = CFArrayGetCount(cert_chain); i < count; ++i) {
     SecCertificateRef chain_cert = reinterpret_cast<SecCertificateRef>(
         const_cast<void*>(CFArrayGetValueAtIndex(cert_chain, i)));
@@ skipping 27 matching lines @@
     // OS X certificate library, but based on history, it is best to play it
     // safe.
     const CSSM_X509_ALGORITHM_IDENTIFIER* sig_algorithm =
         signature_field.GetAs<CSSM_X509_ALGORITHM_IDENTIFIER>();
     if (!sig_algorithm)
       continue;
 
     const CSSM_OID* alg_oid = &sig_algorithm->algorithm;
     if (CSSMOIDEqual(alg_oid, &CSSMOID_MD2WithRSA)) {
       verify_result->has_md2 = true;
-      if (i == 0)
-        *leaf_is_weak = true;
     } else if (CSSMOIDEqual(alg_oid, &CSSMOID_MD4WithRSA)) {
       verify_result->has_md4 = true;
-      if (i == 0)
-        *leaf_is_weak = true;
     } else if (CSSMOIDEqual(alg_oid, &CSSMOID_MD5WithRSA)) {
       verify_result->has_md5 = true;
-      if (i == 0)
-        *leaf_is_weak = true;
     } else if (CSSMOIDEqual(alg_oid, &CSSMOID_SHA1WithRSA) ||
                CSSMOIDEqual(alg_oid, &CSSMOID_SHA1WithRSA_OIW) ||
                CSSMOIDEqual(alg_oid, &CSSMOID_SHA1WithDSA) ||
                CSSMOIDEqual(alg_oid, &CSSMOID_SHA1WithDSA_CMS) ||
                CSSMOIDEqual(alg_oid, &CSSMOID_SHA1WithDSA_JDK) ||
                CSSMOIDEqual(alg_oid, &CSSMOID_ECDSA_WithSHA1)) {
       verify_result->has_sha1 = true;
-      if (i == 0)
-        *leaf_is_weak = true;
     }
   }
-  if (!verified_cert)
+  if (!verified_cert) {
+    NOTREACHED();
     return;
+  }
 
   verify_result->verified_cert =
       X509Certificate::CreateFromHandle(verified_cert, verified_chain);
 }
 
 void AppendPublicKeyHashes(CFArrayRef chain,
                            HashValueVector* hashes) {
   const CFIndex n = CFArrayGetCount(chain);
   for (CFIndex i = 0; i < n; i++) {
     SecCertificateRef cert = reinterpret_cast<SecCertificateRef>(
@@ skipping 286 matching lines @@
     SecTrustResultType temp_trust_result = kSecTrustResultDeny;
     ScopedCFTypeRef<CFArrayRef> temp_chain;
     CSSM_TP_APPLE_EVIDENCE_INFO* temp_chain_info = NULL;
 
     int rv = BuildAndEvaluateSecTrustRef(cert_array, trust_policies, flags,
                                          &temp_ref, &temp_trust_result,
                                          &temp_chain, &temp_chain_info);
     if (rv != OK)
       return rv;
 
-    CertVerifyResult temp_verify_result;
-    bool leaf_is_weak = false;
-    GetCertChainInfo(temp_chain, temp_chain_info, &temp_verify_result,
-                     &leaf_is_weak);
-
     bool untrusted = (temp_trust_result != kSecTrustResultUnspecified &&
                       temp_trust_result != kSecTrustResultProceed);
-    bool weak_chain =
-        !leaf_is_weak &&
-        (temp_verify_result.has_md2 || temp_verify_result.has_md4 ||
-         temp_verify_result.has_md5 || temp_verify_result.has_sha1);
+    bool weak_chain = false;
+    if (CFArrayGetCount(cert_array) > 0) {
+      CertVerifyResult temp_verify_result;
+      GetCertChainInfo(temp_chain, temp_chain_info, &temp_verify_result);
+      weak_chain = temp_verify_result.has_md2 || temp_verify_result.has_md4 ||
+                   temp_verify_result.has_md5 || temp_verify_result.has_sha1;
+    } else {
+      // If the chain is trusted or has recoverable errors, it cannot be empty.
+      DCHECK(untrusted);
+      DCHECK_NE(kSecTrustResultRecoverableTrustFailure, temp_trust_result);
+    }
     // Set the result to the current chain if:
     // - This is the first verification attempt. This ensures that if
     //   everything is awful (e.g. it may just be an untrusted cert), that
     //   what is reported is exactly what was sent by the server
     // - If the current chain is trusted, and the old chain was not trusted,
     //   then prefer this chain. This ensures that if there is at least a
     //   valid path to a trust anchor, it's preferred over reporting an error.
     // - If the current chain is trusted, and the old chain is trusted, but
     //   the old chain contained weak algorithms while the current chain only
     //   contains strong algorithms, then prefer the current chain over the
     //   old chain.
-    //
-    // Note: If the leaf certificate itself is weak, then the only
-    // consideration is whether or not there is a trusted chain. That's
-    // because no amount of path discovery will fix a weak leaf.

[Ryan Sleevi, 2015/04/23 00:30:50]

This is a bug that we _aren't_ exiting this loop after the first pass when
leaf_is_weak, so I'd rather *not* delete all this code, and instead fix that
bug.

     if (!trust_ref || (!untrusted && (candidate_untrusted ||
                                       (candidate_weak && !weak_chain)))) {
       trust_ref = temp_ref;
       trust_result = temp_trust_result;
       completed_chain = temp_chain;
       chain_info = temp_chain_info;
 
       candidate_untrusted = untrusted;
       candidate_weak = weak_chain;
     }
     // Short-circuit when a current, trusted chain is found.
     if (!untrusted && !weak_chain)

[Ryan Sleevi, 2015/04/23 00:30:50]

This is the bug.

It should have been
if (!untrusted && (leaf_is_weak || !weak_chain))


The logic was expressed somewhat inverted, in that we didn't want to call it a
"weak chain" if the leaf was also weak, because there's no amount we can do to
fix it.

       break;
     CFArrayRemoveValueAtIndex(cert_array, CFArrayGetCount(cert_array) - 1);
   }
 
   if (flags & CertVerifier::VERIFY_REV_CHECKING_ENABLED)
     verify_result->cert_status |= CERT_STATUS_REV_CHECKING_ENABLED;
 
   if (crl_set && !CheckRevocationWithCRLSet(completed_chain, crl_set))
     verify_result->cert_status |= CERT_STATUS_REVOKED;
 
-  bool leaf_is_weak_unused = false;
-  GetCertChainInfo(completed_chain, chain_info, verify_result,
-                   &leaf_is_weak_unused);
+  if (CFArrayGetCount(completed_chain) > 0)
+    GetCertChainInfo(completed_chain, chain_info, verify_result);
 
   // As of Security Update 2012-002/OS X 10.7.4, when an RSA key < 1024 bits
   // is encountered, CSSM returns CSSMERR_TP_VERIFY_ACTION_FAILED and adds
   // CSSMERR_CSP_UNSUPPORTED_KEY_SIZE as a certificate status. Avoid mapping
   // the CSSMERR_TP_VERIFY_ACTION_FAILED to CERT_STATUS_INVALID if the only
   // error was due to an unsupported key size.
   bool policy_failed = false;
   bool weak_key_or_signature_algorithm = false;
 
   // Evaluate the results
@@ skipping 140 matching lines @@
           }
         }
       }
     }
   }
 
   return OK;
 }
 
 }  // namespace net