Get all of the certificate data for downloads from the WinVerifyTrust result.

chrome/browser/safe_browsing/signature_util_win.cc

Index: chrome/browser/safe_browsing/signature_util_win.cc
diff --git a/chrome/browser/safe_browsing/signature_util_win.cc b/chrome/browser/safe_browsing/signature_util_win.cc
index 148e75ac36ff493f7daed5482a6a2904d291f4be..b4758bbb11e76cd9c104f905e49478487712450a 100644
--- a/chrome/browser/safe_browsing/signature_util_win.cc
+++ b/chrome/browser/safe_browsing/signature_util_win.cc
@@ -6,152 +6,25 @@
 
 #include <windows.h>
 #include <softpub.h>
-#include <wincrypt.h>
 #include <wintrust.h>
 
 #include "base/file_path.h"
 #include "base/logging.h"
-#include "base/scoped_ptr.h"
-#include "crypto/scoped_capi_types.h"
 #include "chrome/common/safe_browsing/csd.pb.h"
 
-#pragma comment(lib, "crypt32.lib")
 #pragma comment(lib, "wintrust.lib")
 
 namespace safe_browsing {
-namespace {
-using crypto::ScopedCAPIHandle;
-using crypto::CAPIDestroyer;
-using crypto::CAPIDestroyerWithFlags;
 
-// Free functors for scoped_ptr_malloc.
-class FreeConstCertContext {
- public:
-  void operator()(const CERT_CONTEXT* cert_context) const {
-    if (cert_context) {
-      CertFreeCertificateContext(cert_context);
-    }
-  }
-};
+SignatureUtil::SignatureUtil() {}
 
-class FreeConstCertChainContext {
- public:
-  void operator()(const CERT_CHAIN_CONTEXT* cert_chain_context) const {
-    if (cert_chain_context) {
-      CertFreeCertificateChain(cert_chain_context);
-    }
-  }
-};
+SignatureUtil::~SignatureUtil() {}
 
-// Tries to extract the signing certificate from |file_path|.  On success,
-// the |certificate_contents| field of |signature_info| is populated with the
-// DER-encoded X.509 certificate.
-void GetCertificateContents(
+void SignatureUtil::CheckSignature(
     const FilePath& file_path,
     ClientDownloadRequest_SignatureInfo* signature_info) {
-  // Largely based on http://support.microsoft.com/kb/323809
-  // Get message handle and store handle from the signed file.
-  typedef CAPIDestroyer<HCRYPTMSG, CryptMsgClose> CryptMsgDestroyer;
-  ScopedCAPIHandle<HCRYPTMSG, CryptMsgDestroyer> crypt_msg;
-  typedef CAPIDestroyerWithFlags<
-      HCERTSTORE, CertCloseStore, 0> CertStoreDestroyer;
-  ScopedCAPIHandle<HCERTSTORE, CertStoreDestroyer> cert_store;
-
-  VLOG(2) << "Looking for signature in: " << file_path.value();
-  if (!CryptQueryObject(CERT_QUERY_OBJECT_FILE,
-                        file_path.value().c_str(),
-                        CERT_QUERY_CONTENT_FLAG_PKCS7_SIGNED_EMBED,
-                        CERT_QUERY_FORMAT_FLAG_BINARY,
-                        0,      // flags
-                        NULL,   // encoding
-                        NULL,   // content type
-                        NULL,   // format type
-                        cert_store.receive(),
-                        crypt_msg.receive(),
-                        NULL)) {  // context
-    VLOG(2) << "No signature found.";
-    return;
-  }
-
-  // Get the signer information size.
-  DWORD signer_info_size;
-  if (!CryptMsgGetParam(crypt_msg.get(),
-                        CMSG_SIGNER_INFO_PARAM,
-                        0,  // index
-                        NULL,  // no buffer when checking the size
-                        &signer_info_size)) {
-    VLOG(2) << "Failed to get signer info size";
-    return;
-  }
-
-  // Get the signer information.
-  scoped_array<BYTE> signer_info_buffer(new BYTE[signer_info_size]);
-  CMSG_SIGNER_INFO* signer_info =
-      reinterpret_cast<CMSG_SIGNER_INFO*>(signer_info_buffer.get());
-  if (!CryptMsgGetParam(crypt_msg.get(),
-                        CMSG_SIGNER_INFO_PARAM,
-                        0,  // index
-                        signer_info,
-                        &signer_info_size)) {
-    VLOG(2) << "Failed to get signer info";
-    return;
-  }
-
-  // Search for the signer certificate in the temporary certificate store.
-  CERT_INFO cert_info;
-  cert_info.Issuer = signer_info->Issuer;
-  cert_info.SerialNumber = signer_info->SerialNumber;
-
-  scoped_ptr_malloc<const CERT_CONTEXT, FreeConstCertContext> cert_context(
-      CertFindCertificateInStore(
-          cert_store.get(),
-          X509_ASN_ENCODING | PKCS_7_ASN_ENCODING,
-          0, // flags
-          CERT_FIND_SUBJECT_CERT,
-          &cert_info,
-          NULL));  // no previous context
-  if (!cert_context.get()) {
-    VLOG(2) << "Failed to get CERT_CONTEXT";
-    return;
-  }
-
-  // Follow the chain of certificates to a trusted root.
-  CERT_CHAIN_PARA cert_chain_params;
-  memset(&cert_chain_params, 0, sizeof(cert_chain_params));
-  cert_chain_params.cbSize = sizeof(cert_chain_params);
+  VLOG(2) << "Checking signature for " << file_path.value();
 
-  const CERT_CHAIN_CONTEXT* cert_chain_context = NULL;
-  if (!CertGetCertificateChain(NULL,  // default chain engine
-                               cert_context.get(),
-                               // TODO(bryner): should this verify at the
-                               // executable's embedded timestamp?
-                               NULL,  // verify at the current time
-                               NULL,  // no additional store
-                               &cert_chain_params,
-                               0,     // default flags
-                               NULL,  // reserved parameter
-                               &cert_chain_context)) {
-    VLOG(2) << "Failed to get certificate chain.";
-    return;
-  }
-
-  typedef scoped_ptr_malloc<const CERT_CHAIN_CONTEXT,
-                            FreeConstCertChainContext> ScopedCertChainContext;
-  ScopedCertChainContext scoped_cert_chain_context(cert_chain_context);
-  for (size_t i = 0; i < cert_chain_context->cChain; ++i) {
-    CERT_SIMPLE_CHAIN* simple_chain = cert_chain_context->rgpChain[i];
-    ClientDownloadRequest_CertificateChain* chain =
-        signature_info->add_certificate_chain();
-    for (size_t j = 0; j < simple_chain->cElement; ++j) {
-      CERT_CHAIN_ELEMENT* element = simple_chain->rgpElement[j];
-      chain->add_element()->set_certificate(
-          element->pCertContext->pbCertEncoded,
-          element->pCertContext->cbCertEncoded);
-    }
-  }
-}
-
-bool CheckTrust(const FilePath& file_path) {
   WINTRUST_FILE_INFO file_info;
   file_info.cbStruct = sizeof(file_info);
   file_info.pcwszFilePath = file_path.value().c_str();
@@ -166,7 +39,7 @@ bool CheckTrust(const FilePath& file_path) {
   wintrust_data.fdwRevocationChecks = WTD_REVOKE_NONE;
   wintrust_data.dwUnionChoice = WTD_CHOICE_FILE;
   wintrust_data.pFile = &file_info;
-  wintrust_data.dwStateAction = WTD_STATEACTION_IGNORE;
+  wintrust_data.dwStateAction = WTD_STATEACTION_VERIFY;
   wintrust_data.hWVTStateData = NULL;
   wintrust_data.pwszURLReference = NULL;
   wintrust_data.dwProvFlags = 0;  // don't set any flags

[Ryan Sleevi, 2011/11/15 00:27:12]

FWIW, you could also considere adding WTD_CACHE_ONLY_URL_RETRIEVAL here to
disable AIA fetching. This should ensure that the certificate chain submitted is
only what is actually present in the executable.

This could result in false negatives, if the code signing actually failed to
include the whole chain and the user didn't already have it, but that's
generally an indication that someone is doing something 'silly' with
code-signing, rather than "As intended". After all, it'd be no different than
the user trying to verify the signature while offline.

[Brian Ryner, 2011/11/15 01:05:37]

Done, but I have a question.  The docs for this (
http://msdn.microsoft.com/en-us/library/windows/desktop/aa388205(v=vs.85).aspx )
say that this value isn't supported on Win2k and WinXP.  Any idea what the
behavior will be on those operating systems?  Will the whole WinVerifyTrust call
fail or will it just ignore the flag?

[Ryan Sleevi, 2011/11/15 01:20:02]

To be honest, I don't recall, it's been a while since I've touched code that
used WVT. I seem to recall passing it, and this was code that ran on Windows
95-Vista (95 via IE3, which first included CryptoAPI and Authenticode).

It might be safer to omit entirely, or I can try to follow-up in a few days
after digging in to XP vanilla again.

@@ -177,21 +50,35 @@ bool CheckTrust(const FilePath& file_path) {
   // sign code.
   GUID policy_guid = WINTRUST_ACTION_GENERIC_VERIFY_V2;
 
-  return (WinVerifyTrust(static_cast<HWND>(INVALID_HANDLE_VALUE),
-                         &policy_guid,
-                         &wintrust_data) == ERROR_SUCCESS);
-}
-}  // namespace
-
-SignatureUtil::SignatureUtil() {}
-
-SignatureUtil::~SignatureUtil() {}
+  signature_info->set_trusted(

[noelutz, 2011/11/15 00:29:47]

Should we not set trusted at all if the file has no signature?

Also, it might be interested to return a more nuanced answer.  According to [0]
we could know whether the certificate was explicitly distrusted for example as
opposed to not trusted.

[0]
http://msdn.microsoft.com/en-us/library/windows/desktop/aa382384(v=vs.85).aspx

[Brian Ryner, 2011/11/15 01:05:37]

Sure, done.
Interesting, but in the interest of simplicity I'd like to hold off on this
until we know for sure what we need.

[noelutz, 2011/11/15 01:12:43]

SGTM

+      WinVerifyTrust(static_cast<HWND>(INVALID_HANDLE_VALUE),
+                     &policy_guid,
+                     &wintrust_data) == ERROR_SUCCESS);
+
+  CRYPT_PROVIDER_DATA* prov_data = WTHelperProvDataFromStateData(
+      wintrust_data.hWVTStateData);
+  if (prov_data) {
+    for (size_t i = 0; i < prov_data->csSigners; ++i) {

[Ryan Sleevi, 2011/11/15 00:27:12]

nit: DWORD

[Brian Ryner, 2011/11/15 01:05:37]

Done.

+      const CERT_CHAIN_CONTEXT* cert_chain_context =
+          prov_data->pasSigners[i].pChainContext;
+      for (size_t j = 0; j < cert_chain_context->cChain; ++j) {

[Ryan Sleevi, 2011/11/15 00:27:12]

nit: DWORD

[Brian Ryner, 2011/11/15 01:05:37]

Done.

+        CERT_SIMPLE_CHAIN* simple_chain = cert_chain_context->rgpChain[j];
+        ClientDownloadRequest_CertificateChain* chain =
+            signature_info->add_certificate_chain();
+        for (size_t k = 0; k < simple_chain->cElement; ++k) {

[Ryan Sleevi, 2011/11/15 00:27:12]

nit: DWORD

[Brian Ryner, 2011/11/15 01:05:37]

Done.

+          CERT_CHAIN_ELEMENT* element = simple_chain->rgpElement[k];
+          chain->add_element()->set_certificate(
+              element->pCertContext->pbCertEncoded,
+              element->pCertContext->cbCertEncoded);
+        }
+      }
+    }
 
-void SignatureUtil::CheckSignature(
-    const FilePath& file_path,
-    ClientDownloadRequest_SignatureInfo* signature_info) {
-  GetCertificateContents(file_path, signature_info);
-  signature_info->set_trusted(CheckTrust(file_path));
+    // Free the provider data.
+    wintrust_data.dwStateAction = WTD_STATEACTION_CLOSE;
+    WinVerifyTrust(static_cast<HWND>(INVALID_HANDLE_VALUE),
+                   &policy_guid, &wintrust_data);
+  }
 }
 
 }  // namespace safe_browsing