This CL adds error tracking to SSLPolicy.

For a certificate to be allowed, it must not have any *additional* errors from when it was allowed, but for a certificate to be denied, it need only match *any* errors that caused it to be denied. Denial is checked before allowance.

More specifically:

* If you are checking CertC with ErrorC against the set of saved certificates, we will deny CertC if ErrorC has any overlap with any saved denied certificates. It does not need to be a perfect match.

* If you are checking CertC with ErrorC against the set of saved certificates, we will allow CertC if ErrorC is a perfect match to or subset of any saved allowed certificates.

BUG=263214
R=rsleevi@chromium.org
TBR=abarth@chromium.org

Review URL: https://chromiumcodereview.appspot.com/20931003

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

6 files changed, 131 insertions, 68 deletions

diff --git a/content/browser/ssl/ssl_host_state.cc b/content/browser/ssl/ssl_host_state.cc
index c2fc06d..06c6002 100644
--- a/content/browser/ssl/ssl_host_state.cc
+++ b/content/browser/ssl/ssl_host_state.cc
@@ -39,17 +39,19 @@ bool SSLHostState::DidHostRunInsecureContent(const std::string& host,
 }
 
 void SSLHostState::DenyCertForHost(net::X509Certificate* cert,
-                                   const std::string& host) {
+                                   const std::string& host,
+                                   net::CertStatus error) {
   DCHECK(CalledOnValidThread());
 
-  cert_policy_for_host_[host].Deny(cert);
+  cert_policy_for_host_[host].Deny(cert, error);
 }
 
 void SSLHostState::AllowCertForHost(net::X509Certificate* cert,
-                                    const std::string& host) {
+                                    const std::string& host,
+                                    net::CertStatus error) {
   DCHECK(CalledOnValidThread());
 
-  cert_policy_for_host_[host].Allow(cert);
+  cert_policy_for_host_[host].Allow(cert, error);
 }
 
 void SSLHostState::Clear() {
@@ -58,11 +60,12 @@ void SSLHostState::Clear() {
   cert_policy_for_host_.clear();
 }
 
-net::CertPolicy::Judgment SSLHostState::QueryPolicy(
-    net::X509Certificate* cert, const std::string& host) {
+net::CertPolicy::Judgment SSLHostState::QueryPolicy(net::X509Certificate* cert,
+                                                    const std::string& host,
+                                                    net::CertStatus error) {
   DCHECK(CalledOnValidThread());
 
-  return cert_policy_for_host_[host].Check(cert);
+  return cert_policy_for_host_[host].Check(cert, error);
 }
 
 }  // namespace content
diff --git a/content/browser/ssl/ssl_host_state.h b/content/browser/ssl/ssl_host_state.h
index b229b79..8208217 100644
--- a/content/browser/ssl/ssl_host_state.h
+++ b/content/browser/ssl/ssl_host_state.h
@@ -14,6 +14,7 @@
 #include "base/supports_user_data.h"
 #include "base/threading/non_thread_safe.h"
 #include "content/common/content_export.h"
+#include "net/cert/cert_status_flags.h"
 #include "net/cert/x509_certificate.h"
 
 namespace content {
@@ -42,18 +43,25 @@ class CONTENT_EXPORT SSLHostState
   // Returns whether the specified host ran insecure content.
   bool DidHostRunInsecureContent(const std::string& host, int pid) const;
 
-  // Records that |cert| is permitted to be used for |host| in the future.
-  void DenyCertForHost(net::X509Certificate* cert, const std::string& host);
+  // Records that |cert| is not permitted to be used for |host| in the future,
+  // for a specified |error| type..
+  void DenyCertForHost(net::X509Certificate* cert,
+                       const std::string& host,
+                       net::CertStatus error);
 
-  // Records that |cert| is not permitted to be used for |host| in the future.
-  void AllowCertForHost(net::X509Certificate* cert, const std::string& host);
+  // Records that |cert| is permitted to be used for |host| in the future, for
+  // a specified |error| type.
+  void AllowCertForHost(net::X509Certificate* cert,
+                        const std::string& host,
+                        net::CertStatus error);
 
   // Clear all allow/deny preferences.
   void Clear();
 
-  // Queries whether |cert| is allowed or denied for |host|.
-  net::CertPolicy::Judgment QueryPolicy(
-      net::X509Certificate* cert, const std::string& host);
+  // Queries whether |cert| is allowed or denied for |host| and |error|.
+  net::CertPolicy::Judgment QueryPolicy(net::X509Certificate* cert,
+                                        const std::string& host,
+                                        net::CertStatus error);
 
  private:
   // A BrokenHostEntry is a pair of (host, process_id) that indicates the host
diff --git a/content/browser/ssl/ssl_host_state_unittest.cc b/content/browser/ssl/ssl_host_state_unittest.cc
index 7ba266b..5e4366d 100644
--- a/content/browser/ssl/ssl_host_state_unittest.cc
+++ b/content/browser/ssl/ssl_host_state_unittest.cc
@@ -120,48 +120,84 @@ TEST_F(SSLHostStateTest, QueryPolicy) {
 
   SSLHostState state;
 
-  EXPECT_EQ(state.QueryPolicy(google_cert.get(), "www.google.com"),
-            net::CertPolicy::UNKNOWN);
-  EXPECT_EQ(state.QueryPolicy(google_cert.get(), "google.com"),
-            net::CertPolicy::UNKNOWN);
-  EXPECT_EQ(state.QueryPolicy(google_cert.get(), "example.com"),
-            net::CertPolicy::UNKNOWN);
-
-  state.AllowCertForHost(google_cert.get(), "www.google.com");
-
-  EXPECT_EQ(state.QueryPolicy(google_cert.get(), "www.google.com"),
-            net::CertPolicy::ALLOWED);
-  EXPECT_EQ(state.QueryPolicy(google_cert.get(), "google.com"),
-            net::CertPolicy::UNKNOWN);
-  EXPECT_EQ(state.QueryPolicy(google_cert.get(), "example.com"),
-            net::CertPolicy::UNKNOWN);
-
-  state.AllowCertForHost(google_cert.get(), "example.com");
-
-  EXPECT_EQ(state.QueryPolicy(google_cert.get(), "www.google.com"),
-            net::CertPolicy::ALLOWED);
-  EXPECT_EQ(state.QueryPolicy(google_cert.get(), "google.com"),
-            net::CertPolicy::UNKNOWN);
-  EXPECT_EQ(state.QueryPolicy(google_cert.get(), "example.com"),
-            net::CertPolicy::ALLOWED);
-
-  state.DenyCertForHost(google_cert.get(), "example.com");
-
-  EXPECT_EQ(state.QueryPolicy(google_cert.get(), "www.google.com"),
-            net::CertPolicy::ALLOWED);
-  EXPECT_EQ(state.QueryPolicy(google_cert.get(), "google.com"),
-            net::CertPolicy::UNKNOWN);
-  EXPECT_EQ(state.QueryPolicy(google_cert.get(), "example.com"),
-            net::CertPolicy::DENIED);
+  EXPECT_EQ(net::CertPolicy::UNKNOWN,
+            state.QueryPolicy(google_cert.get(),
+                              "www.google.com",
+                              net::CERT_STATUS_DATE_INVALID));
+  EXPECT_EQ(net::CertPolicy::UNKNOWN,
+            state.QueryPolicy(google_cert.get(),
+                              "google.com",
+                              net::CERT_STATUS_DATE_INVALID));
+  EXPECT_EQ(net::CertPolicy::UNKNOWN,
+            state.QueryPolicy(google_cert.get(),
+                              "example.com",
+                              net::CERT_STATUS_DATE_INVALID));
+
+  state.AllowCertForHost(google_cert.get(),
+                         "www.google.com",
+                         net::CERT_STATUS_DATE_INVALID);
+
+  EXPECT_EQ(net::CertPolicy::ALLOWED,
+           state.QueryPolicy(google_cert.get(),
+                             "www.google.com",
+                             net::CERT_STATUS_DATE_INVALID));
+  EXPECT_EQ(net::CertPolicy::UNKNOWN,
+           state.QueryPolicy(google_cert.get(),
+                             "google.com",
+                             net::CERT_STATUS_DATE_INVALID));
+  EXPECT_EQ(net::CertPolicy::UNKNOWN,
+           state.QueryPolicy(google_cert.get(),
+                             "example.com",
+                             net::CERT_STATUS_DATE_INVALID));
+
+  state.AllowCertForHost(google_cert.get(),
+                         "example.com",
+                         net::CERT_STATUS_DATE_INVALID);
+
+  EXPECT_EQ(net::CertPolicy::ALLOWED,
+          state.QueryPolicy(google_cert.get(),
+                            "www.google.com",
+                            net::CERT_STATUS_DATE_INVALID));
+  EXPECT_EQ(net::CertPolicy::UNKNOWN,
+          state.QueryPolicy(google_cert.get(),
+                            "google.com",
+                            net::CERT_STATUS_DATE_INVALID));
+  EXPECT_EQ(net::CertPolicy::ALLOWED,
+          state.QueryPolicy(google_cert.get(),
+                            "example.com",
+                            net::CERT_STATUS_DATE_INVALID));
+
+  state.DenyCertForHost(google_cert.get(),
+                        "example.com",
+                        net::CERT_STATUS_DATE_INVALID);
+
+  EXPECT_EQ(net::CertPolicy::ALLOWED,
+          state.QueryPolicy(google_cert.get(),
+                            "www.google.com",
+                            net::CERT_STATUS_DATE_INVALID));
+  EXPECT_EQ(net::CertPolicy::UNKNOWN,
+          state.QueryPolicy(google_cert.get(),
+                            "google.com",
+                            net::CERT_STATUS_DATE_INVALID));
+  EXPECT_EQ(net::CertPolicy::DENIED,
+          state.QueryPolicy(google_cert.get(),
+                            "example.com",
+                            net::CERT_STATUS_DATE_INVALID));
 
   state.Clear();
 
-  EXPECT_EQ(state.QueryPolicy(google_cert.get(), "www.google.com"),
-            net::CertPolicy::UNKNOWN);
-  EXPECT_EQ(state.QueryPolicy(google_cert.get(), "google.com"),
-            net::CertPolicy::UNKNOWN);
-  EXPECT_EQ(state.QueryPolicy(google_cert.get(), "example.com"),
-            net::CertPolicy::UNKNOWN);
+  EXPECT_EQ(net::CertPolicy::UNKNOWN,
+            state.QueryPolicy(google_cert.get(),
+                              "www.google.com",
+                              net::CERT_STATUS_DATE_INVALID));
+  EXPECT_EQ(net::CertPolicy::UNKNOWN,
+            state.QueryPolicy(google_cert.get(),
+                              "google.com",
+                              net::CERT_STATUS_DATE_INVALID));
+  EXPECT_EQ(net::CertPolicy::UNKNOWN,
+            state.QueryPolicy(google_cert.get(),
+                              "example.com",
+                              net::CERT_STATUS_DATE_INVALID));
 }
 
 }  // namespace content
diff --git a/content/browser/ssl/ssl_policy.cc b/content/browser/ssl/ssl_policy.cc
index 41156d5..02af398 100644
--- a/content/browser/ssl/ssl_policy.cc
+++ b/content/browser/ssl/ssl_policy.cc
@@ -34,7 +34,9 @@ SSLPolicy::SSLPolicy(SSLPolicyBackend* backend)
 void SSLPolicy::OnCertError(SSLCertErrorHandler* handler) {
   // First we check if we know the policy for this error.
   net::CertPolicy::Judgment judgment = backend_->QueryPolicy(
-      handler->ssl_info().cert.get(), handler->request_url().host());
+      handler->ssl_info().cert.get(),
+      handler->request_url().host(),
+      handler->cert_error());
 
   if (judgment == net::CertPolicy::ALLOWED) {
     handler->ContinueRequest();
@@ -156,7 +158,8 @@ void SSLPolicy::OnAllowCertificate(scoped_refptr<SSLCertErrorHandler> handler,
     // ContinueRequest() gets posted to a different thread. Calling
     // AllowCertForHost() first ensures deterministic ordering.
     backend_->AllowCertForHost(handler->ssl_info().cert.get(),
-                               handler->request_url().host());
+                               handler->request_url().host(),
+                               handler->cert_error());
     handler->ContinueRequest();
   } else {
     // Default behavior for rejecting a certificate.
@@ -165,7 +168,8 @@ void SSLPolicy::OnAllowCertificate(scoped_refptr<SSLCertErrorHandler> handler,
     // CancelRequest() gets posted to a different thread. Calling
     // DenyCertForHost() first ensures deterministic ordering.
     backend_->DenyCertForHost(handler->ssl_info().cert.get(),
-                              handler->request_url().host());
+                              handler->request_url().host(),
+                              handler->cert_error());
     handler->CancelRequest();
   }
 }
diff --git a/content/browser/ssl/ssl_policy_backend.cc b/content/browser/ssl/ssl_policy_backend.cc
index 8aa8bc7..57e0570 100644
--- a/content/browser/ssl/ssl_policy_backend.cc
+++ b/content/browser/ssl/ssl_policy_backend.cc
@@ -27,18 +27,22 @@ bool SSLPolicyBackend::DidHostRunInsecureContent(const std::string& host,
 }
 
 void SSLPolicyBackend::DenyCertForHost(net::X509Certificate* cert,
-                                       const std::string& host) {
-  ssl_host_state_->DenyCertForHost(cert, host);
+                                       const std::string& host,
+                                       net::CertStatus error) {
+  ssl_host_state_->DenyCertForHost(cert, host, error);
 }
 
 void SSLPolicyBackend::AllowCertForHost(net::X509Certificate* cert,
-                                        const std::string& host) {
-  ssl_host_state_->AllowCertForHost(cert, host);
+                                        const std::string& host,
+                                        net::CertStatus error) {
+  ssl_host_state_->AllowCertForHost(cert, host, error);
 }
 
 net::CertPolicy::Judgment SSLPolicyBackend::QueryPolicy(
-    net::X509Certificate* cert, const std::string& host) {
-  return ssl_host_state_->QueryPolicy(cert, host);
+    net::X509Certificate* cert,
+    const std::string& host,
+    net::CertStatus error) {
+  return ssl_host_state_->QueryPolicy(cert, host, error);
 }
 
 }  // namespace content
diff --git a/content/browser/ssl/ssl_policy_backend.h b/content/browser/ssl/ssl_policy_backend.h
index 18ec580..06ea23e 100644
--- a/content/browser/ssl/ssl_policy_backend.h
+++ b/content/browser/ssl/ssl_policy_backend.h
@@ -10,6 +10,7 @@
 
 #include "base/basictypes.h"
 #include "base/strings/string16.h"
+#include "net/cert/cert_status_flags.h"
 #include "net/cert/x509_certificate.h"
 
 namespace content {
@@ -26,15 +27,22 @@ class SSLPolicyBackend {
   // Returns whether the specified host ran insecure content.
   bool DidHostRunInsecureContent(const std::string& host, int pid) const;
 
-  // Records that |cert| is permitted to be used for |host| in the future.
-  void DenyCertForHost(net::X509Certificate* cert, const std::string& host);
+  // Records that |cert| is not permitted to be used for |host| in the future,
+  // for a specific error type.
+  void DenyCertForHost(net::X509Certificate* cert,
+                       const std::string& host,
+                       net::CertStatus error);
 
-  // Records that |cert| is not permitted to be used for |host| in the future.
-  void AllowCertForHost(net::X509Certificate* cert, const std::string& host);
+  // Records that |cert| is permitted to be used for |host| in the future, for
+  // a specific error type.
+  void AllowCertForHost(net::X509Certificate* cert,
+                        const std::string& host,
+                        net::CertStatus error);
 
   // Queries whether |cert| is allowed or denied for |host|.
-  net::CertPolicy::Judgment QueryPolicy(
-      net::X509Certificate* cert, const std::string& host);
+  net::CertPolicy::Judgment QueryPolicy(net::X509Certificate* cert,
+                                        const std::string& host,
+                                        net::CertStatus error);
 
  private:
   // SSL state specific for each host.