Check X509Certificate::CreateFromHandle result.

net/cert/cert_verify_proc_win.cc

Index: net/cert/cert_verify_proc_win.cc
diff --git a/net/cert/cert_verify_proc_win.cc b/net/cert/cert_verify_proc_win.cc
index ef334da0ae464bfa88bfdd804d702aa02b06e2c3..86ca6c61dc03c198ac64188d8e571e5f929cef35 100644
--- a/net/cert/cert_verify_proc_win.cc
+++ b/net/cert/cert_verify_proc_win.cc
@@ -329,10 +329,10 @@ bool IsIssuedByKnownRoot(PCCERT_CHAIN_CONTEXT chain_context) {
 // 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;
@@ -369,9 +369,13 @@ void GetCertChainInfo(PCCERT_CHAIN_CONTEXT chain_context,
     // 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
@@ -1114,7 +1118,8 @@ int CertVerifyProcWin::VerifyInternal(
   }
 
   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;
@@ -1136,7 +1141,8 @@ int CertVerifyProcWin::VerifyInternal(
       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) {