Include the full certificate chain in the download pingback.

BUG=102540
TEST=SignatureUtilWinTest

Review URL: http://codereview.chromium.org/8536035

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@109807 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/safe_browsing/download_protection_service.cc

@@ -405,7 +405,7 @@ class DownloadProtectionService::CheckClientDownloadRequest
   void ExtractFileFeatures() {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
     signature_util_->CheckSignature(info_.local_file, &signature_info_);
-    bool is_signed = signature_info_.has_certificate_contents();
+    bool is_signed = (signature_info_.certificate_chain_size() > 0);
     if (is_signed) {
       VLOG(2) << "Downloaded a signed binary: " << info_.local_file.value();
     } else {
@@ -454,7 +454,8 @@ class DownloadProtectionService::CheckClientDownloadRequest
           sb_service_->MatchDownloadWhitelistUrl(info_.referrer_url)) {
         reason = REASON_WHITELISTED_REFERRER;
       }
-      if (reason != REASON_MAX || signature_info_.has_certificate_contents()) {
+      if (reason != REASON_MAX ||
+          signature_info_.certificate_chain_size() > 0) {
         UMA_HISTOGRAM_COUNTS("SBClientDownload.SignedOrWhitelistedDownload", 1);
       }
     }

chrome/browser/safe_browsing/download_protection_service_unittest.cc

@@ -63,7 +63,7 @@ class MockSignatureUtil : public SignatureUtil {
 }  // namespace
 
 ACTION_P(SetCertificateContents, contents) {
-  arg1->set_certificate_contents(contents);
+  arg1->add_certificate_chain()->add_element()->set_certificate(contents);
 }
 
 // We can't call OnSafeBrowsingResult directly because SafeBrowsingCheck does
@@ -384,8 +384,11 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadValidateRequest) {
                                       "http://www.google.com/bla.exe",
                                       info.referrer_url.spec()));
   EXPECT_TRUE(request.has_signature());
-  EXPECT_TRUE(request.signature().has_certificate_contents());
-  EXPECT_EQ("dummy cert data", request.signature().certificate_contents());
+  ASSERT_EQ(1, request.signature().certificate_chain_size());
+  const ClientDownloadRequest_CertificateChain& chain =
+      request.signature().certificate_chain(0);
+  ASSERT_EQ(1, chain.element_size());
+  EXPECT_EQ("dummy cert data", chain.element(0).certificate());
 
   // Simulate the request finishing.
   MessageLoop::current()->PostTask(
@@ -437,7 +440,7 @@ TEST_F(DownloadProtectionServiceTest,
                                       "http://www.google.com/bla.exe",
                                       info.referrer_url.spec()));
   EXPECT_TRUE(request.has_signature());
-  EXPECT_FALSE(request.signature().has_certificate_contents());
+  EXPECT_EQ(0, request.signature().certificate_chain_size());
 
   // Simulate the request finishing.
   MessageLoop::current()->PostTask(

chrome/browser/safe_browsing/signature_util_win.cc

@@ -24,12 +24,23 @@ using crypto::ScopedCAPIHandle;
 using crypto::CAPIDestroyer;
 using crypto::CAPIDestroyerWithFlags;
 
-// Free functor for scoped_ptr_malloc.
+// Free functors for scoped_ptr_malloc.
 class FreeConstCertContext {
  public:
   void operator()(const CERT_CONTEXT* cert_context) const {
+    if (cert_context) {
       CertFreeCertificateContext(cert_context);
     }
+  }
+};
+
+class FreeConstCertChainContext {
+ public:
+  void operator()(const CERT_CHAIN_CONTEXT* cert_chain_context) const {
+    if (cert_chain_context) {
+      CertFreeCertificateChain(cert_chain_context);
+    }
+  }
 };
 
 // Tries to extract the signing certificate from |file_path|.  On success,
@@ -104,9 +115,40 @@ void GetCertificateContents(
     return;
   }
 
-  signature_info->set_certificate_contents(cert_context->pbCertEncoded,
-                                           cert_context->cbCertEncoded);
-  VLOG(2) << "Successfully extracted cert";
+  // 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);
+
+  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) {

chrome/browser/safe_browsing/signature_util_win_unittest.cc

@@ -5,6 +5,8 @@
 #include "chrome/browser/safe_browsing/signature_util.h"
 
 #include <string>
+#include <vector>
+
 #include "base/base_paths.h"
 #include "base/file_path.h"
 #include "base/path_service.h"
@@ -16,56 +18,87 @@
 
 namespace safe_browsing {
 
-TEST(SignatureUtilWinTest, CheckSignature) {
+class SignatureUtilWinTest : public testing::Test {
+ protected:
+  virtual void SetUp() {
     FilePath source_path;
     ASSERT_TRUE(PathService::Get(base::DIR_SOURCE_ROOT, &source_path));
-
-  FilePath testdata_path = source_path
+    testdata_path_ = source_path
         .AppendASCII("chrome")
         .AppendASCII("test")
         .AppendASCII("data")
         .AppendASCII("safe_browsing")
         .AppendASCII("download_protection");
+  }
+
+  // Given a certificate chain protobuf, parse it into X509Certificates.
+  void ParseCertificateChain(
+      const ClientDownloadRequest_CertificateChain& chain,
+      std::vector<scoped_refptr<net::X509Certificate> >* certs) {
+    for (int i = 0; i < chain.element_size(); ++i) {
+      certs->push_back(
+          net::X509Certificate::CreateFromBytes(
+              chain.element(i).certificate().data(),
+              chain.element(i).certificate().size()));
+    }
+  }
+
+  FilePath testdata_path_;
+};
 
-  // signed.exe is signed with a self-signed certificate.  The certificate
-  // should be returned, but it is not trusted.
+TEST_F(SignatureUtilWinTest, UntrustedSignedBinary) {
+  // signed.exe is signed by an untrusted root CA.
   scoped_refptr<SignatureUtil> signature_util(new SignatureUtil());
   ClientDownloadRequest_SignatureInfo signature_info;
-  signature_util->CheckSignature(testdata_path.Append(L"signed.exe"),
+  signature_util->CheckSignature(testdata_path_.Append(L"signed.exe"),
                                  &signature_info);
-  EXPECT_FALSE(signature_info.certificate_contents().empty());
-  scoped_refptr<net::X509Certificate> cert(
-      net::X509Certificate::CreateFromBytes(
-          signature_info.certificate_contents().data(),
-          signature_info.certificate_contents().size()));
-  ASSERT_TRUE(cert.get());
-  EXPECT_EQ("Joe's-Software-Emporium", cert->subject().common_name);
+  ASSERT_EQ(1, signature_info.certificate_chain_size());
+  std::vector<scoped_refptr<net::X509Certificate> > certs;
+  ParseCertificateChain(signature_info.certificate_chain(0), &certs);
+  ASSERT_EQ(2, certs.size());
+  EXPECT_EQ("Joe's-Software-Emporium", certs[0]->subject().common_name);
+  EXPECT_EQ("Root Agency", certs[1]->subject().common_name);
+
   EXPECT_FALSE(signature_info.trusted());
+}
 
+TEST_F(SignatureUtilWinTest, TrustedBinary) {
   // wow_helper.exe is signed using Google's signing certifiacte.
-  signature_info.Clear();
-  signature_util->CheckSignature(testdata_path.Append(L"wow_helper.exe"),
+  scoped_refptr<SignatureUtil> signature_util(new SignatureUtil());
+  ClientDownloadRequest_SignatureInfo signature_info;
+  signature_util->CheckSignature(testdata_path_.Append(L"wow_helper.exe"),
                                  &signature_info);
-  EXPECT_TRUE(signature_info.has_certificate_contents());
-  cert = net::X509Certificate::CreateFromBytes(
-      signature_info.certificate_contents().data(),
-      signature_info.certificate_contents().size());
-  ASSERT_TRUE(cert.get());
-  EXPECT_EQ("Google Inc", cert->subject().common_name);
+  ASSERT_EQ(1, signature_info.certificate_chain_size());
+  std::vector<scoped_refptr<net::X509Certificate> > certs;
+  ParseCertificateChain(signature_info.certificate_chain(0), &certs);
+  ASSERT_EQ(3, certs.size());
+
+  EXPECT_EQ("Google Inc", certs[0]->subject().common_name);
+  EXPECT_EQ("VeriSign Class 3 Code Signing 2009-2 CA",
+            certs[1]->subject().common_name);
+  EXPECT_EQ("Class 3 Public Primary Certification Authority",
+            certs[2]->subject().organization_unit_names[0]);
+
   EXPECT_TRUE(signature_info.trusted());
+}
 
+TEST_F(SignatureUtilWinTest, UnsignedBinary) {
   // unsigned.exe has no signature information.
-  signature_info.Clear();
-  signature_util->CheckSignature(testdata_path.Append(L"unsigned.exe"),
+  scoped_refptr<SignatureUtil> signature_util(new SignatureUtil());
+  ClientDownloadRequest_SignatureInfo signature_info;
+  signature_util->CheckSignature(testdata_path_.Append(L"unsigned.exe"),
                                  &signature_info);
-  EXPECT_FALSE(signature_info.has_certificate_contents());
+  EXPECT_EQ(0, signature_info.certificate_chain_size());
   EXPECT_FALSE(signature_info.trusted());
+}
 
+TEST_F(SignatureUtilWinTest, NonExistentBinary) {
   // Test a file that doesn't exist.
-  signature_info.Clear();
-  signature_util->CheckSignature(testdata_path.Append(L"doesnotexist.exe"),
+  scoped_refptr<SignatureUtil> signature_util(new SignatureUtil());
+  ClientDownloadRequest_SignatureInfo signature_info;
+  signature_util->CheckSignature(testdata_path_.Append(L"doesnotexist.exe"),
                                  &signature_info);
-  EXPECT_FALSE(signature_info.has_certificate_contents());
+  EXPECT_EQ(0, signature_info.certificate_chain_size());
   EXPECT_FALSE(signature_info.trusted());
 }
 

chrome/common/safe_browsing/csd.proto

@@ -124,10 +124,24 @@ message ClientDownloadRequest {
   // triggered the download) as well as for the download URL itself.
   repeated Resource resources = 4;
 
+  // A trust chain of certificates.  Each chain begins with the signing
+  // certificate of the binary, and ends with a self-signed certificate,
+  // typically from a trusted root CA.  This structure is analogous to
+  // CERT_CHAIN_CONTEXT on Windows.
+  message CertificateChain {
+    // A single link in the chain.
+    message Element {
+      // DER-encoded X.509 representation of the certificate.
+      optional bytes certificate = 1;
+    }
+    repeated Element element = 1;
+  }
+
   message SignatureInfo {
-    // The full DER-encoded X.509 certificate extracted from the binary.
-    // If this field is not present, it means the binary was unsigned.
-    optional bytes certificate_contents = 1;
+    // All of the certificate chains for the binary's signing certificate.
+    // If no chains are present, the binary is not signed.  Multiple chains
+    // may be present if any certificate has multiple signers.
+    repeated CertificateChain certificate_chain = 1;
 
     // True if the signature was trusted on the client.
     optional bool trusted = 2;