Implement a whitelist for code-signing certificates.

With this change, only downloads that match a whitelisted URL or certificate will be exempt from the download protection pingback.  The certificate whitelist format allows matching on the certificate issuer along with the CN, O, or OU attributes of the certificate.

BUG=102540
TEST=DownloadProtectionServiceTest


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

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

chrome/browser/safe_browsing/download_protection_service.cc

@@ -24,6 +24,8 @@
 #include "content/public/common/url_fetcher.h"
 #include "content/public/common/url_fetcher_delegate.h"
 #include "net/base/load_flags.h"
+#include "net/base/x509_cert_types.h"
+#include "net/base/x509_certificate.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "net/url_request/url_request_status.h"
 
@@ -503,12 +505,15 @@ class DownloadProtectionService::CheckClientDownloadRequest
       for (size_t i = 0; i < info_.download_url_chain.size(); ++i) {
         const GURL& url = info_.download_url_chain[i];
         if (url.is_valid() && sb_service_->MatchDownloadWhitelistUrl(url)) {
+          VLOG(2) << url << " is on the download whitelist.";
           reason = REASON_WHITELISTED_URL;
           break;
         }
       }
       if (info_.referrer_url.is_valid() && reason == REASON_MAX &&
           sb_service_->MatchDownloadWhitelistUrl(info_.referrer_url)) {
+        VLOG(2) << "Referrer url " << info_.referrer_url
+                << " is on the download whitelist.";
         reason = REASON_WHITELISTED_REFERRER;
       }
       if (reason != REASON_MAX || signature_info_.trusted()) {
@@ -516,9 +521,13 @@ class DownloadProtectionService::CheckClientDownloadRequest
       }
     }
     if (reason == REASON_MAX && signature_info_.trusted()) {
-      // TODO(noelutz): implement a certificate whitelist and only whitelist
-      // binaries whose certificate match the whitelist.
-      reason = REASON_TRUSTED_EXECUTABLE;
+      for (int i = 0; i < signature_info_.certificate_chain_size(); ++i) {
+        if (CertificateChainIsWhitelisted(
+                signature_info_.certificate_chain(i))) {
+          reason = REASON_TRUSTED_EXECUTABLE;
+          break;
+        }
+      }
     }
     if (reason != REASON_MAX) {
       RecordImprovedProtectionStats(reason);
@@ -622,6 +631,46 @@ class DownloadProtectionService::CheckClientDownloadRequest
                               REASON_MAX);
   }
 
+  bool CertificateChainIsWhitelisted(
+      const ClientDownloadRequest_CertificateChain& chain) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    if (chain.element_size() < 2) {
+      // We need to have both a signing certificate and its issuer certificate
+      // present to construct a whitelist entry.
+      return false;
+    }
+    scoped_refptr<net::X509Certificate> cert =
+        net::X509Certificate::CreateFromBytes(
+            chain.element(0).certificate().data(),
+            chain.element(0).certificate().size());
+    if (!cert.get()) {
+      return false;
+    }
+
+    for (int i = 1; i < chain.element_size(); ++i) {
+      scoped_refptr<net::X509Certificate> issuer =
+          net::X509Certificate::CreateFromBytes(
+              chain.element(i).certificate().data(),
+              chain.element(i).certificate().size());
+      if (!issuer.get()) {
+        return false;
+      }
+      std::vector<std::string> whitelist_strings;
+      DownloadProtectionService::GetCertificateWhitelistStrings(
+          *cert, *issuer, &whitelist_strings);
+      for (size_t j = 0; j < whitelist_strings.size(); ++j) {
+        if (sb_service_->MatchDownloadWhitelistString(whitelist_strings[j])) {
+          VLOG(2) << "Certificate matched whitelist, cert="
+                  << cert->subject().GetDisplayName()
+                  << " issuer=" << issuer->subject().GetDisplayName();
+          return true;
+        }
+      }
+      cert = issuer;
+    }
+    return false;
+  }
+
   DownloadInfo info_;
   ClientDownloadRequest_SignatureInfo signature_info_;
   CheckDownloadCallback callback_;
@@ -712,4 +761,80 @@ void DownloadProtectionService::RequestFinished(
   DCHECK(it != download_requests_.end());
   download_requests_.erase(*it);
 }
+
+namespace {
+// Escapes a certificate attribute so that it can be used in a whitelist
+// entry.  Currently, we only escape slashes, since they are used as a
+// separator between attributes.
+std::string EscapeCertAttribute(const std::string& attribute) {
+  std::string escaped;
+  for (size_t i = 0; i < attribute.size(); ++i) {
+    if (attribute[i] == '%') {
+      escaped.append("%25");
+    } else if (attribute[i] == '/') {
+      escaped.append("%2F");
+    } else {
+      escaped.push_back(attribute[i]);
+    }
+  }
+  return escaped;
+}
+}  // namespace
+
+// static
+void DownloadProtectionService::GetCertificateWhitelistStrings(
+    const net::X509Certificate& certificate,
+    const net::X509Certificate& issuer,
+    std::vector<std::string>* whitelist_strings) {
+  // The whitelist paths are in the format:
+  // cert/<ascii issuer fingerprint>[/CN=common_name][/O=org][/OU=unit]
+  //
+  // Any of CN, O, or OU may be omitted from the whitelist entry, in which
+  // case they match anything.  However, the attributes that do appear will
+  // always be in the order shown above.  At least one attribute will always
+  // be present.
+
+  const net::CertPrincipal& subject = certificate.subject();
+  std::vector<std::string> ou_tokens;
+  for (size_t i = 0; i < subject.organization_unit_names.size(); ++i) {
+    ou_tokens.push_back(
+        "/OU=" + EscapeCertAttribute(subject.organization_unit_names[i]));
+  }
+
+  std::vector<std::string> o_tokens;
+  for (size_t i = 0; i < subject.organization_names.size(); ++i) {
+    o_tokens.push_back(
+        "/O=" + EscapeCertAttribute(subject.organization_names[i]));
+  }
+
+  std::string cn_token;
+  if (!subject.common_name.empty()) {
+    cn_token = "/CN=" + EscapeCertAttribute(subject.common_name);
+  }
+
+  std::set<std::string> paths_to_check;
+  if (!cn_token.empty()) {
+    paths_to_check.insert(cn_token);
+  }
+  for (size_t i = 0; i < o_tokens.size(); ++i) {
+    paths_to_check.insert(cn_token + o_tokens[i]);
+    paths_to_check.insert(o_tokens[i]);
+    for (size_t j = 0; j < ou_tokens.size(); ++j) {
+      paths_to_check.insert(cn_token + o_tokens[i] + ou_tokens[j]);
+      paths_to_check.insert(o_tokens[i] + ou_tokens[j]);
+    }
+  }
+  for (size_t i = 0; i < ou_tokens.size(); ++i) {
+    paths_to_check.insert(cn_token + ou_tokens[i]);
+    paths_to_check.insert(ou_tokens[i]);
+  }
+
+  std::string issuer_fp = base::HexEncode(issuer.fingerprint().data,
+                                          sizeof(issuer.fingerprint().data));
+  for (std::set<std::string>::iterator it = paths_to_check.begin();
+       it != paths_to_check.end(); ++it) {
+    whitelist_strings->push_back("cert/" + issuer_fp + *it);
+  }
+}
+
 }  // namespace safe_browsing

chrome/browser/safe_browsing/download_protection_service.h

@@ -25,6 +25,7 @@ class SafeBrowsingService;
 
 namespace net {
 class URLRequestContextGetter;
+class X509Certificate;
 }  // namespace net
 
 namespace safe_browsing {
@@ -140,7 +141,6 @@ class DownloadProtectionService {
                            CheckClientDownloadFetchFailed);
   FRIEND_TEST_ALL_PREFIXES(DownloadProtectionServiceTest,
                            TestDownloadRequestTimeout);
-
   static const char kDownloadRequestUrl[];
 
   // Cancels all requests in |download_requests_|, and empties it, releasing
@@ -154,6 +154,14 @@ class DownloadProtectionService {
   static void FillDownloadInfo(const DownloadItem& item,
                                DownloadInfo* download_info);
 
+  // Given a certificate and its immediate issuer certificate, generates the
+  // list of strings that need to be checked against the download whitelist to
+  // determine whether the certificate is whitelisted.
+  static void GetCertificateWhitelistStrings(
+      const net::X509Certificate& certificate,
+      const net::X509Certificate& issuer,
+      std::vector<std::string>* whitelist_strings);
+
   // This pointer may be NULL if SafeBrowsing is disabled. The
   // SafeBrowsingService owns us, so we don't need to hold a reference to it.
   SafeBrowsingService* sb_service_;

chrome/browser/safe_browsing/download_protection_service_unittest.cc

@@ -7,12 +7,16 @@
 #include <map>
 #include <string>
 
+#include "base/base_paths.h"
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/file_path.h"
+#include "base/file_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop.h"
+#include "base/path_service.h"
+#include "base/string_number_conversions.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/browser/safe_browsing/signature_util.h"
 #include "chrome/common/safe_browsing/csd.pb.h"
@@ -20,12 +24,15 @@
 #include "content/public/common/url_fetcher_delegate.h"
 #include "content/test/test_browser_thread.h"
 #include "content/test/test_url_fetcher_factory.h"
+#include "crypto/rsa_private_key.h"
 #include "googleurl/src/gurl.h"
+#include "net/base/x509_certificate.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using ::testing::ContainerEq;
 using ::testing::DoAll;
+using ::testing::ElementsAre;
 using ::testing::Mock;
 using ::testing::NotNull;
 using ::testing::Return;
@@ -42,6 +49,7 @@ class MockSafeBrowsingService : public SafeBrowsingService {
   virtual ~MockSafeBrowsingService() {}
 
   MOCK_METHOD1(MatchDownloadWhitelistUrl, bool(const GURL&));
+  MOCK_METHOD1(MatchDownloadWhitelistString, bool(const std::string&));
   MOCK_METHOD2(CheckDownloadUrl, bool(const std::vector<GURL>& url_chain,
                                       Client* client));
   MOCK_METHOD2(CheckDownloadHash, bool(const std::string&, Client* client));
@@ -66,8 +74,16 @@ ACTION_P(SetCertificateContents, contents) {
   arg1->add_certificate_chain()->add_element()->set_certificate(contents);
 }
 
-ACTION(TrustSignature) {
+ACTION_P(TrustSignature, certificate_file) {
   arg1->set_trusted(true);
+  // Add a certificate chain.  Note that we add the certificate twice so that
+  // it appears as its own issuer.
+  std::string cert_data;
+  ASSERT_TRUE(file_util::ReadFileToString(certificate_file, &cert_data));
+  ClientDownloadRequest_CertificateChain* chain =
+      arg1->add_certificate_chain();
+  chain->add_element()->set_certificate(cert_data);
+  chain->add_element()->set_certificate(cert_data);
 }
 
 // We can't call OnSafeBrowsingResult directly because SafeBrowsingCheck does
@@ -122,6 +138,15 @@ class DownloadProtectionServiceTest : public testing::Test {
     download_service_->signature_util_ = signature_util_;
     download_service_->SetEnabled(true);
     msg_loop_.RunAllPending();
+
+    FilePath source_path;
+    ASSERT_TRUE(PathService::Get(base::DIR_SOURCE_ROOT, &source_path));
+    testdata_path_ = source_path
+        .AppendASCII("chrome")
+        .AppendASCII("test")
+        .AppendASCII("data")
+        .AppendASCII("safe_browsing")
+        .AppendASCII("download_protection");
   }
 
   virtual void TearDown() {
@@ -155,6 +180,15 @@ class DownloadProtectionServiceTest : public testing::Test {
     msg_loop_.RunAllPending();
   }
 
+  // Proxy for private method.
+  static void GetCertificateWhitelistStrings(
+      const net::X509Certificate& certificate,
+      const net::X509Certificate& issuer,
+      std::vector<std::string>* whitelist_strings) {
+    DownloadProtectionService::GetCertificateWhitelistStrings(
+        certificate, issuer, whitelist_strings);
+  }
+
  private:
   // Helper functions for FlushThreadMessageLoops.
   void RunAllPendingAndQuitUI() {
@@ -212,6 +246,7 @@ class DownloadProtectionServiceTest : public testing::Test {
   scoped_ptr<content::TestBrowserThread> io_thread_;
   scoped_ptr<content::TestBrowserThread> file_thread_;
   scoped_ptr<content::TestBrowserThread> ui_thread_;
+  FilePath testdata_path_;
 };
 
 TEST_F(DownloadProtectionServiceTest, CheckClientDownloadInvalidUrl) {
@@ -602,7 +637,13 @@ TEST_F(DownloadProtectionServiceTest, CheckClientDownloadDigestList) {
   EXPECT_CALL(*sb_service_, MatchDownloadWhitelistUrl(_))
       .WillRepeatedly(Return(false));
   EXPECT_CALL(*signature_util_, CheckSignature(info.local_file, _))
-      .WillOnce(TrustSignature());
+      .WillOnce(TrustSignature(
+          testdata_path_.AppendASCII("signature_util_test.cer")));
+  EXPECT_CALL(*sb_service_,
+              MatchDownloadWhitelistString(
+                  "cert/58AFF702772EB67BDD412571BA40AAC07F0D936C/"
+                  "CN=Joe's-Software-Emporium"))
+      .WillOnce(Return(true));
   EXPECT_CALL(*sb_service_,
               CheckDownloadHash(info.sha256_hash, NotNull()))
       .WillOnce(DoAll(CheckDownloadHashDone(SafeBrowsingService::SAFE),
@@ -701,4 +742,119 @@ TEST_F(DownloadProtectionServiceTest, TestDownloadRequestTimeout) {
   msg_loop_.Run();
   ExpectResult(DownloadProtectionService::SAFE);
 }
+
+TEST_F(DownloadProtectionServiceTest,
+       GetCertificateWhitelistStrings_TestCert) {
+  std::string cert_data;
+  ASSERT_TRUE(file_util::ReadFileToString(testdata_path_.AppendASCII(
+      "signature_util_test.cer"), &cert_data));
+
+  scoped_refptr<net::X509Certificate> cert(
+      net::X509Certificate::CreateFromBytes(cert_data.data(),
+                                            cert_data.size()));
+  ASSERT_TRUE(cert.get());
+
+  std::vector<std::string> whitelist_strings;
+  GetCertificateWhitelistStrings(*cert, *cert, &whitelist_strings);
+
+  EXPECT_THAT(whitelist_strings, ElementsAre(
+      "cert/58AFF702772EB67BDD412571BA40AAC07F0D936C"
+      "/CN=Joe's-Software-Emporium"));
+}
+
+// Only some implementations have the ability to generate self-signed certs.
+#if defined(USE_NSS) || defined(OS_WIN) || defined(OS_MACOSX)
+TEST_F(DownloadProtectionServiceTest,
+       GetCertificateWhitelistStrings_SelfSigned) {
+  scoped_ptr<crypto::RSAPrivateKey> private_key(
+      crypto::RSAPrivateKey::Create(1024));
+  // We'll pass this cert in as the "issuer", even though it isn't really
+  // used to sign the certs below.  GetCertificateWhitelistStirngs doesn't care
+  // about this.
+  scoped_refptr<net::X509Certificate> issuer_cert =
+      net::X509Certificate::CreateSelfSigned(
+          private_key.get(), "CN=issuer", 1, base::TimeDelta::FromDays(1));
+  ASSERT_TRUE(issuer_cert.get());
+  std::string cert_base = "cert/" + base::HexEncode(
+      issuer_cert->fingerprint().data,
+      sizeof(issuer_cert->fingerprint().data));
+
+  scoped_refptr<net::X509Certificate> cert =
+      net::X509Certificate::CreateSelfSigned(
+          private_key.get(), "CN=subject/%1", 1, base::TimeDelta::FromDays(1));
+  ASSERT_TRUE(cert.get());
+  std::vector<std::string> whitelist_strings;
+  GetCertificateWhitelistStrings(*cert, *issuer_cert, &whitelist_strings);
+  // This also tests escaping of characters in the certificate attributes.
+  EXPECT_THAT(whitelist_strings, ElementsAre(
+      cert_base + "/CN=subject%2F%251"));
+
+  cert = net::X509Certificate::CreateSelfSigned(
+      private_key.get(), "CN=subject,O=org", 1, base::TimeDelta::FromDays(1));
+  ASSERT_TRUE(cert.get());
+  whitelist_strings.clear();
+  GetCertificateWhitelistStrings(*cert, *issuer_cert, &whitelist_strings);
+  EXPECT_THAT(whitelist_strings, ElementsAre(
+      cert_base + "/CN=subject",
+      cert_base + "/CN=subject/O=org",
+      cert_base + "/O=org"));
+
+  cert = net::X509Certificate::CreateSelfSigned(
+      private_key.get(), "CN=subject,O=org,OU=unit", 1,
+      base::TimeDelta::FromDays(1));
+  ASSERT_TRUE(cert.get());
+  whitelist_strings.clear();
+  GetCertificateWhitelistStrings(*cert, *issuer_cert, &whitelist_strings);
+  EXPECT_THAT(whitelist_strings, ElementsAre(
+      cert_base + "/CN=subject",
+      cert_base + "/CN=subject/O=org",
+      cert_base + "/CN=subject/O=org/OU=unit",
+      cert_base + "/CN=subject/OU=unit",
+      cert_base + "/O=org",
+      cert_base + "/O=org/OU=unit",
+      cert_base + "/OU=unit"));
+
+  cert = net::X509Certificate::CreateSelfSigned(
+      private_key.get(), "CN=subject,OU=unit", 1,
+      base::TimeDelta::FromDays(1));
+  ASSERT_TRUE(cert.get());
+  whitelist_strings.clear();
+  GetCertificateWhitelistStrings(*cert, *issuer_cert, &whitelist_strings);
+  EXPECT_THAT(whitelist_strings, ElementsAre(
+      cert_base + "/CN=subject",
+      cert_base + "/CN=subject/OU=unit",
+      cert_base + "/OU=unit"));
+
+  cert = net::X509Certificate::CreateSelfSigned(
+      private_key.get(), "O=org,", 1, base::TimeDelta::FromDays(1));
+  ASSERT_TRUE(cert.get());
+  whitelist_strings.clear();
+  GetCertificateWhitelistStrings(*cert, *issuer_cert, &whitelist_strings);
+  EXPECT_THAT(whitelist_strings, ElementsAre(cert_base + "/O=org"));
+
+  cert = net::X509Certificate::CreateSelfSigned(
+      private_key.get(), "O=org,OU=unit", 1, base::TimeDelta::FromDays(1));
+  ASSERT_TRUE(cert.get());
+  whitelist_strings.clear();
+  GetCertificateWhitelistStrings(*cert, *issuer_cert, &whitelist_strings);
+  EXPECT_THAT(whitelist_strings, ElementsAre(
+      cert_base + "/O=org",
+      cert_base + "/O=org/OU=unit",
+      cert_base + "/OU=unit"));
+
+  cert = net::X509Certificate::CreateSelfSigned(
+      private_key.get(), "OU=unit", 1, base::TimeDelta::FromDays(1));
+  ASSERT_TRUE(cert.get());
+  whitelist_strings.clear();
+  GetCertificateWhitelistStrings(*cert, *issuer_cert, &whitelist_strings);
+  EXPECT_THAT(whitelist_strings, ElementsAre(cert_base + "/OU=unit"));
+
+  cert = net::X509Certificate::CreateSelfSigned(
+      private_key.get(), "C=US", 1, base::TimeDelta::FromDays(1));
+  ASSERT_TRUE(cert.get());
+  whitelist_strings.clear();
+  GetCertificateWhitelistStrings(*cert, *issuer_cert, &whitelist_strings);
+  EXPECT_THAT(whitelist_strings, ElementsAre());
+}
+#endif
 }  // namespace safe_browsing

chrome/browser/safe_browsing/safe_browsing_service.cc

@@ -304,6 +304,15 @@ bool SafeBrowsingService::MatchDownloadWhitelistUrl(const GURL& url) {
   return database_->ContainsDownloadWhitelistedUrl(url);
 }
 
+bool SafeBrowsingService::MatchDownloadWhitelistString(
+    const std::string& str) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  if (!enabled_ || !enable_download_whitelist_ || !MakeDatabaseAvailable()) {
+    return true;
+  }
+  return database_->ContainsDownloadWhitelistedString(str);
+}
+
 bool SafeBrowsingService::CheckBrowseUrl(const GURL& url,
                                          Client* client) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));

chrome/browser/safe_browsing/safe_browsing_service.h

@@ -208,6 +208,12 @@ class SafeBrowsingService
   // This method is expected to be called on the IO thread.
   virtual bool MatchDownloadWhitelistUrl(const GURL& url);
 
+  // Check if |str| matches any of the full-length hashes from the download
+  // whitelist.  Returns true if there was a match and false otherwise.
+  // To make sure we are conservative we will return true if an error occurs.
+  // This method is expected to be called on the IO thread.
+  virtual bool MatchDownloadWhitelistString(const std::string& str);
+
   // Called on the IO thread to cancel a pending check if the result is no
   // longer needed.
   void CancelCheck(Client* client);