Check X509Certificate::CreateFromHandle result.

net/cert/cert_verify_proc_ios.cc

 // Copyright (c) 2016 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_ios.h"
 
 #include <CommonCrypto/CommonDigest.h>
 
 #include "base/logging.h"
 #include "base/mac/scoped_cftyperef.h"
@@ skipping 83 matching lines @@
     SecCertificateRef chain_cert = SecTrustGetCertificateAtIndex(tmp_trust, i);
     CFArrayAppendValue(tmp_verified_chain, chain_cert);
   }
 
   trust_ref->swap(scoped_tmp_trust);
   *trust_result = tmp_trust_result;
   verified_chain->reset(tmp_verified_chain.release());
   return OK;
 }
 
-void GetCertChainInfo(CFArrayRef cert_chain, CertVerifyResult* verify_result) {
+bool GetCertChainInfo(CFArrayRef cert_chain, CertVerifyResult* verify_result) {
   DCHECK_LT(0, CFArrayGetCount(cert_chain));
 
   SecCertificateRef verified_cert = nullptr;
   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);
     }
 
     std::string der_bytes;
     if (!X509Certificate::GetDEREncoded(chain_cert, &der_bytes))
-      return;
+      return false;
 
     base::StringPiece spki_bytes;
     if (!asn1::ExtractSPKIFromDERCert(der_bytes, &spki_bytes))
       continue;

[eroman, 2017/03/22 22:17:52]

What makes this a non-fatal failure, whereas GetDEREncoded() now is fatal?

[mattm, 2017/03/23 22:59:02]

Good point, seems this should be fatal as well.

 
     HashValue sha1(HASH_VALUE_SHA1);
     CC_SHA1(spki_bytes.data(), spki_bytes.size(), sha1.data());
     verify_result->public_key_hashes.push_back(sha1);
 
     HashValue sha256(HASH_VALUE_SHA256);
     CC_SHA256(spki_bytes.data(), spki_bytes.size(), sha256.data());
     verify_result->public_key_hashes.push_back(sha256);
 
     // Ignore the signature algorithm for the trust anchor.
     if ((verify_result->cert_status & CERT_STATUS_AUTHORITY_INVALID) == 0 &&
         i == count - 1) {
       continue;
     }
   }
   if (!verified_cert) {
     NOTREACHED();
-    return;
+    return false;
   }
 
   verify_result->verified_cert =
       X509Certificate::CreateFromHandle(verified_cert, verified_chain);
+  return !!verify_result->verified_cert;
 }
 
 }  // namespace
 
 CertVerifyProcIOS::CertVerifyProcIOS() {}
 
 // The iOS APIs don't expose an API-stable set of reasons for certificate
 // validation failures. However, internally, the reason is tracked, and it's
 // converted to user-facing localized strings.
 //
@@ skipping 100 matching lines @@
     case kSecTrustResultUnspecified:
     case kSecTrustResultProceed:
       break;
     case kSecTrustResultDeny:
       verify_result->cert_status |= CERT_STATUS_AUTHORITY_INVALID;
       break;
     default:
       verify_result->cert_status |= GetCertFailureStatusFromTrust(trust_ref);
   }
 
-  GetCertChainInfo(final_chain, verify_result);
+  if (!GetCertChainInfo(final_chain, verify_result))
+    return ERR_CERT_INVALID;

[eroman, 2017/03/22 22:17:52]

Is it necessary to set cert_status too?

[mattm, 2017/03/23 22:59:02]

Hm, I guess setting cert_status and letting that propagate to the return value
is preferable. That way if CertVerifyProc (non-platform-specific code) adds more
error statuses and recomputes the net error code, it doesn't get lost.

 
   // iOS lacks the ability to distinguish built-in versus non-built-in roots,
   // so opt to 'fail open' of any restrictive policies that apply to built-in
   // roots.
   verify_result->is_issued_by_known_root = false;
 
   if (IsCertStatusError(verify_result->cert_status))
     return MapCertStatusToNetError(verify_result->cert_status);
 
   return OK;
 }
 
 }  // namespace net