Check X509Certificate::CreateFromHandle result.

net/cert/cert_verify_proc_win.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_win.h"
 
 #include <memory>
 #include <string>
 #include <vector>
 
@@ skipping 311 matching lines @@
   }
   UMA_HISTOGRAM_ENUMERATION("Net.SSL_AuthRootConsistency", status,
                             BUILT_IN_MAX_VALUE);
 
   return is_builtin;
 }
 
 // Saves some information about the certificate chain |chain_context| in
 // |*verify_result|. The caller MUST initialize |*verify_result| before
 // calling this function.
-void GetCertChainInfo(PCCERT_CHAIN_CONTEXT chain_context,
+bool GetCertChainInfo(PCCERT_CHAIN_CONTEXT chain_context,
                       CertVerifyResult* verify_result) {
   if (chain_context->cChain == 0)
-    return;
+    return true;

[eroman, 2017/03/23 23:41:31]

Should this be false? (more discussion on this below).

[mattm, 2017/03/24 21:49:35]

I guess the problem is I'm not really sure either.

It *seems* like this is something that either shouldn't happen or that there
should already be some other error code set if it did, but I'm not really sure.
But maybe it's better to set an error code on any case like this, just to be
safe? I dunno.

[eroman, 2017/03/24 22:07:23]

Either approach sounds fine to me. 

[mattm, 2017/03/27 23:24:37]

Ok, I'll just leave this one as is for now.

 
   PCERT_SIMPLE_CHAIN first_chain = chain_context->rgpChain[0];
   DWORD num_elements = first_chain->cElement;
   PCERT_CHAIN_ELEMENT* element = first_chain->rgpElement;
 
   PCCERT_CONTEXT verified_cert = NULL;
   std::vector<PCCERT_CONTEXT> verified_chain;
 
   bool has_root_ca = num_elements > 1 &&
       !(chain_context->TrustStatus.dwErrorStatus &
@@ skipping 16 matching lines @@
       verified_cert = cert;
     } else {
       verified_chain.push_back(cert);
     }
   }
 
   if (verified_cert) {
     // Add the root certificate, if present, as it was not added above.
     if (has_root_ca)
       verified_chain.push_back(element[num_elements]->pCertContext);
-    verify_result->verified_cert =
-          X509Certificate::CreateFromHandle(verified_cert, verified_chain);
+    scoped_refptr<X509Certificate> verified_cert_with_chain =
+        X509Certificate::CreateFromHandle(verified_cert, verified_chain);
+    if (!verified_cert_with_chain)
+      return false;
+    verify_result->verified_cert = std::move(verified_cert_with_chain);
   }
+  return true;

[eroman, 2017/03/23 23:41:31]

Not clear what true/false means from this function.

From the code it looks like you false basically just means CreateFromHandle()
failed, and all the other short-circuits are true. From reading the signature,
my expectation may have been something different (that true means
verify_result->verified_cert was successfully assigned).

Could you clarify this with documentation, or a different code pattern?

One suggestion would be to keep the void return type, and set
verify_result->cert_status |= CERT_STATUS_INVALID internally in this function.
As in:

if (verified_cert_with_chain)
  verify_result->verified_cert = std::move(...);
else
  verify_result->cert_status |= CERT_STATUS_INVALID;

[mattm, 2017/03/24 21:49:35]

I like the idea of just setting the status directly instead of returning a
value. Done.

 }
 
 // Decodes the cert's certificatePolicies extension into a CERT_POLICIES_INFO
 // structure and stores it in *output.
 void GetCertPoliciesInfo(
     PCCERT_CONTEXT cert,
     std::unique_ptr<CERT_POLICIES_INFO, base::FreeDeleter>* output) {
   PCERT_EXTENSION extension = CertFindExtension(szOID_CERT_POLICIES,
                                                 cert->pCertInfo->cExtension,
                                                 cert->pCertInfo->rgExtension);
@@ skipping 722 matching lines @@
              &chain_para,
              chain_flags,
              NULL,  // reserved
              &chain_context)) {
       verify_result->cert_status |= CERT_STATUS_INVALID;
       return MapSecurityError(GetLastError());
     }
   }
 
   CertVerifyResult temp_verify_result = *verify_result;
-  GetCertChainInfo(chain_context, verify_result);
+  if (!GetCertChainInfo(chain_context, verify_result))
+    verify_result->cert_status |= CERT_STATUS_INVALID;
   if (!verify_result->is_issued_by_known_root &&
       (flags & CertVerifier::VERIFY_REV_CHECKING_REQUIRED_LOCAL_ANCHORS)) {
     *verify_result = temp_verify_result;
 
     rev_checking_enabled = true;
     verify_result->cert_status |= CERT_STATUS_REV_CHECKING_ENABLED;
     chain_flags &= ~CERT_CHAIN_REVOCATION_CHECK_CACHE_ONLY;
 
     CertFreeCertificateChain(chain_context);
     if (!CertGetCertificateChain(
              chain_engine,
              cert_list.get(),
              NULL,  // current system time
              cert_list->hCertStore,
              &chain_para,
              chain_flags,
              NULL,  // reserved
              &chain_context)) {
       verify_result->cert_status |= CERT_STATUS_INVALID;
       return MapSecurityError(GetLastError());
     }
-    GetCertChainInfo(chain_context, verify_result);
+    if (!GetCertChainInfo(chain_context, verify_result))
+      verify_result->cert_status |= CERT_STATUS_INVALID;
 
     if (chain_context->TrustStatus.dwErrorStatus &
         CERT_TRUST_IS_OFFLINE_REVOCATION) {
       verify_result->cert_status |= CERT_STATUS_REVOKED;
     }
   }
 
   ScopedPCCERT_CHAIN_CONTEXT scoped_chain_context(chain_context);
 
   verify_result->cert_status |= MapCertChainErrorStatusToCertStatus(
@@ skipping 58 matching lines @@
     return MapCertStatusToNetError(verify_result->cert_status);
 
   if (ev_policy_oid &&
       CheckEV(chain_context, rev_checking_enabled, ev_policy_oid)) {
     verify_result->cert_status |= CERT_STATUS_IS_EV;
   }
   return OK;
 }
 
 }  // namespace net