Mixed content and SHA1 should override SECURE_WITH_POLICY_INSTALLED_CERT

On ChromeOS, any prior observation of a policy-installed cert is an
indicator of a MITM being present (the enterprise). Previously, the
SECURE_WITH_POLICY_INSTALLED_CERT security level was returned even if
the page contained mixed content or used a SHA-1 certificate. Instead,
such problems should impact the security level instead of being ignored.

Bug: 756639
Change-Id: I4e87fc6039eb76ff8b5b87612c5d0dc004ddd867
Reviewed-on: https://chromium-review.googlesource.com/779743Reviewed-by: Emily Stark <estark@chromium.org>
Commit-Queue: Eric Lawrence <elawrence@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518374}

components/security_state/core/security_state.cc

@@ -155,14 +155,6 @@ SecurityLevel GetSecurityLevelForRequest(
     return kRanInsecureContentLevel;
   }
 
-  // Report if there is a policy cert first, before reporting any other
-  // authenticated-but-with-errors cases. A policy cert is a strong
-  // indicator of a MITM being present (the enterprise), while the
-  // other authenticated-but-with-errors indicate something may
-  // be wrong, or may be wrong in the future, but is unclear now.
-  if (used_policy_installed_certificate)
-    return SECURE_WITH_POLICY_INSTALLED_CERT;
-
   // In most cases, SHA1 use is treated as a certificate error, in which case
   // DANGEROUS will have been returned above. If SHA1 was permitted by policy,
   // downgrade the security level to Neutral.
@@ -189,6 +181,12 @@ SecurityLevel GetSecurityLevelForRequest(
     return NONE;
   }
 
+  // Any prior observation of a policy-installed cert is a strong indicator
+  // of a MITM being present (the enterprise), so a "secure-but-inspected"
+  // security level is returned.
+  if (used_policy_installed_certificate)
+    return SECURE_WITH_POLICY_INSTALLED_CERT;
+
   if ((visible_security_state.cert_status & net::CERT_STATUS_IS_EV) &&
       visible_security_state.certificate) {
     return EV_SECURE;

components/security_state/core/security_state.h

@@ -57,10 +57,11 @@ enum SecurityLevel {
   // HTTPS (non-EV) with valid cert.
   SECURE,
 
-  // HTTPS, but the certificate verification chain is anchored on a
-  // certificate that was installed by the system administrator.
+  // HTTPS, but a certificate chain anchored to a root certificate installed
+  // by the system administrator has been observed in this profile, suggesting
+  // a MITM was present.
   //
-  // Currently used only on ChromeOS.
+  // Used only on ChromeOS, this status is unreached on other platforms.
   SECURE_WITH_POLICY_INSTALLED_CERT,
 
   // Attempted HTTPS and failed, page not authenticated, HTTPS with

components/security_state/core/security_state_unittest.cc

@@ -58,8 +58,8 @@ class TestSecurityStateHelper {
         malicious_content_status_(MALICIOUS_CONTENT_STATUS_NONE),
         is_incognito_(false),
         is_error_page_(false),
-        is_view_source_(false) {}
-
+        is_view_source_(false),
+        has_policy_certificate_(false) {}
   virtual ~TestSecurityStateHelper() {}
 
   void SetCertificate(scoped_refptr<net::X509Certificate> cert) {
@@ -107,7 +107,9 @@ class TestSecurityStateHelper {
   void set_insecure_field_edit(bool insecure_field_edit) {
     insecure_input_events_.insecure_field_edited = insecure_field_edit;
   }
-
+  void set_has_policy_certificate(bool has_policy_cert) {
+    has_policy_certificate_ = has_policy_cert;
+  }
   void SetUrl(const GURL& url) { url_ = url; }
 
   std::unique_ptr<VisibleSecurityState> GetVisibleSecurityState() const {
@@ -130,10 +132,9 @@ class TestSecurityStateHelper {
   }
 
   void GetSecurityInfo(SecurityInfo* security_info) const {
-    security_state::GetSecurityInfo(
-        GetVisibleSecurityState(),
-        false /* used policy installed certificate */,
-        base::Bind(&IsOriginSecure), security_info);
+    security_state::GetSecurityInfo(GetVisibleSecurityState(),
+                                    has_policy_certificate_,
+                                    base::Bind(&IsOriginSecure), security_info);
   }
 
  private:
@@ -148,6 +149,7 @@ class TestSecurityStateHelper {
   bool is_incognito_;
   bool is_error_page_;
   bool is_view_source_;
+  bool has_policy_certificate_;
   InsecureInputEventData insecure_input_events_;
 };
 
@@ -163,6 +165,13 @@ TEST(SecurityStateTest, SHA1Blocked) {
   helper.GetSecurityInfo(&security_info);
   EXPECT_TRUE(security_info.sha1_in_chain);
   EXPECT_EQ(DANGEROUS, security_info.security_level);
+
+  // Ensure that policy-installed certificates do not interfere.
+  helper.set_has_policy_certificate(true);
+  SecurityInfo policy_cert_security_info;
+  helper.GetSecurityInfo(&policy_cert_security_info);
+  EXPECT_TRUE(policy_cert_security_info.sha1_in_chain);
+  EXPECT_EQ(DANGEROUS, policy_cert_security_info.security_level);
 }
 
 // Tests that SHA1-signed certificates, when allowed by policy, downgrade the
@@ -173,6 +182,13 @@ TEST(SecurityStateTest, SHA1Warning) {
   helper.GetSecurityInfo(&security_info);
   EXPECT_TRUE(security_info.sha1_in_chain);
   EXPECT_EQ(NONE, security_info.security_level);
+
+  // Ensure that policy-installed certificates do not interfere.
+  helper.set_has_policy_certificate(true);
+  SecurityInfo policy_cert_security_info;
+  helper.GetSecurityInfo(&policy_cert_security_info);
+  EXPECT_TRUE(policy_cert_security_info.sha1_in_chain);
+  EXPECT_EQ(NONE, policy_cert_security_info.security_level);
 }
 
 // Tests that SHA1-signed certificates, when allowed by policy, don't interfere
@@ -547,12 +563,14 @@ TEST(SecurityStateTest, MixedForm) {
 
   helper.set_contained_mixed_form(true);
 
+  // Verify that a mixed form downgrades the security level.
   SecurityInfo mixed_form_security_info;
   helper.GetSecurityInfo(&mixed_form_security_info);
   EXPECT_TRUE(mixed_form_security_info.contained_mixed_form);
   EXPECT_EQ(CONTENT_STATUS_NONE, mixed_form_security_info.mixed_content_status);
   EXPECT_EQ(NONE, mixed_form_security_info.security_level);
 
+  // Ensure that active mixed content trumps the mixed form warning.
   helper.set_ran_mixed_content(true);
   SecurityInfo mixed_form_and_active_security_info;
   helper.GetSecurityInfo(&mixed_form_and_active_security_info);
@@ -562,6 +580,61 @@ TEST(SecurityStateTest, MixedForm) {
   EXPECT_EQ(DANGEROUS, mixed_form_and_active_security_info.security_level);
 }
 
+// Tests that policy-installed-certificates do not interfere with mixed content
+// notifications.
+TEST(SecurityStateTest, MixedContentWithPolicyCertificate) {
+  TestSecurityStateHelper helper;
+
+  helper.set_has_policy_certificate(true);
+  helper.set_cert_status(0);
+
+  {
+    // Verify that if no mixed content is present, the policy-installed
+    // certificate is recorded.
+    SecurityInfo no_mixed_content_security_info;
+    helper.GetSecurityInfo(&no_mixed_content_security_info);
+    EXPECT_FALSE(no_mixed_content_security_info.contained_mixed_form);
+    EXPECT_EQ(CONTENT_STATUS_NONE,
+              no_mixed_content_security_info.mixed_content_status);
+    EXPECT_EQ(SECURE_WITH_POLICY_INSTALLED_CERT,
+              no_mixed_content_security_info.security_level);
+  }
+
+  {
+    // Verify that a mixed form downgrades the security level.
+    SecurityInfo mixed_form_security_info;
+    helper.set_contained_mixed_form(true);
+    helper.GetSecurityInfo(&mixed_form_security_info);
+    EXPECT_TRUE(mixed_form_security_info.contained_mixed_form);
+    EXPECT_EQ(CONTENT_STATUS_NONE,
+              mixed_form_security_info.mixed_content_status);
+    EXPECT_EQ(NONE, mixed_form_security_info.security_level);
+  }
+
+  {
+    // Verify that passive mixed content downgrades the security level.
+    helper.set_contained_mixed_form(false);
+    helper.set_displayed_mixed_content(true);
+    SecurityInfo passive_mixed_security_info;
+    helper.GetSecurityInfo(&passive_mixed_security_info);
+    EXPECT_EQ(CONTENT_STATUS_DISPLAYED,
+              passive_mixed_security_info.mixed_content_status);
+    EXPECT_EQ(NONE, passive_mixed_security_info.security_level);
+  }
+
+  {
+    // Ensure that active mixed content downgrades the security level.
+    helper.set_contained_mixed_form(false);
+    helper.set_displayed_mixed_content(false);
+    helper.set_ran_mixed_content(true);
+    SecurityInfo active_mixed_security_info;
+    helper.GetSecurityInfo(&active_mixed_security_info);
+    EXPECT_EQ(CONTENT_STATUS_RAN,
+              active_mixed_security_info.mixed_content_status);
+    EXPECT_EQ(DANGEROUS, active_mixed_security_info.security_level);
+  }
+}
+
 // Tests that a field edit is reflected in the SecurityInfo.
 TEST(SecurityStateTest, FieldEdit) {
   TestSecurityStateHelper helper;