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 166 matching lines @@
 
 // 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|.
 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;
   verify_result->has_md2 = false;
   verify_result->has_md4 = false;
   verify_result->has_md5 = false;
   verify_result->has_sha1 = false;
 
+  // If certificate verification fails unrecoverably, OS X will return an
+  // empty chain. CertVerifyResult is expected to contain the original
+  // certificate chain in this case, so leave |verify_result->verified_cert|
+  // unchanged.
+  if (CFArrayGetCount(cert_chain) == 0)
+    return;
+
   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)));
     if (i == 0) {
       verified_cert = chain_cert;
     } else {
       verified_chain.push_back(chain_cert);
     }
@@ skipping 43 matching lines @@
                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 180 matching lines @@
   status = SecTrustEvaluate(tmp_trust, &tmp_trust_result);
   if (status)
     return NetErrorFromOSStatus(status);
   CFArrayRef tmp_verified_chain = NULL;
   CSSM_TP_APPLE_EVIDENCE_INFO* tmp_chain_info;
   status = SecTrustGetResult(tmp_trust, &tmp_trust_result, &tmp_verified_chain,
                              &tmp_chain_info);
   if (status)
     return NetErrorFromOSStatus(status);
 
+  // SecTrustGetResult may return an empty verified chain on some fatal
+  // errors. Guard against any library bugs which may return it in cases where
+  // a chain is expected.
+  if (CFArrayGetCount(tmp_verified_chain) == 0 &&
+      (tmp_trust_result == kSecTrustResultUnspecified ||
+       tmp_trust_result == kSecTrustResultProceed ||
+       tmp_trust_result == kSecTrustResultRecoverableTrustFailure)) {

[Ryan Sleevi, 2015/04/18 00:54:58]

Why only these status codes?

And, from your description, |tmp_trust_result| is
kSecTrustResultFatalTrustError, which wouldn't be caught by either of these
three.

[davidben, 2015/04/22 21:57:58]

You've got it backwards. These are the status codes where we DO expect a chain.

This block was just a sanity check. It's not really needed. That's not the fix.
The fix is to never clear verified_cert up at the top, which is where the
regression came from.

+    NOTREACHED();
+    return ERR_FAILED;
+  }
+
   trust_ref->swap(scoped_tmp_trust);
   *trust_result = tmp_trust_result;
   verified_chain->reset(tmp_verified_chain);
   *chain_info = tmp_chain_info;
 
   return OK;
 }
 
 }  // namespace
 
@@ skipping 96 matching lines @@
     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);

[Ryan Sleevi, 2015/04/18 00:54:58]

The result of this is that a chain that fails (for whatever reason) has higher
priority over a weak, but untrusted chain

This is because weak_chain would be |false| for this chain, but would be true
for the others.

Should weak_chain factor in the CFArrayGetCount() of |temp_chain| ?

[davidben, 2015/04/22 21:57:58]

Hrm? A chain that fails will have untrusted = false. That gives it lower
priority over everything.

In fact, that sanity check up at the top is to prevent precisely what you're
talking about from happening. A chain that's trusted (or untrusted but
recoverable) should never be empty.

     // 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
@@ skipping 180 matching lines @@
           }
         }
       }
     }
   }
 
   return OK;
 }
 
 }  // namespace net