Attach a SecurityStyle to each request in ResourceLoader

content/common/ssl_status_serialization.cc

Index: content/common/ssl_status_serialization.cc
diff --git a/content/common/ssl_status_serialization.cc b/content/common/ssl_status_serialization.cc
index 364dba0538a4cdf6bdc17263e1fca8a663c74b69..d0f4f88f8693b6482396ede74136e22adf1556f6 100644
--- a/content/common/ssl_status_serialization.cc
+++ b/content/common/ssl_status_serialization.cc
@@ -7,16 +7,39 @@
 #include "base/logging.h"
 #include "base/pickle.h"
 
+namespace {
+
+// Checks that an integer |security_style| is a valid SecurityStyle enum
+// value. Returns true if valid, false otherwise.
+bool CheckSecurityStyle(int security_style) {
+  switch (security_style) {
+    case content::SECURITY_STYLE_UNKNOWN:
+      return true;

[davidben, 2015/07/22 20:56:58]

Can a renderer ever legally send this one now? I guess tightening that is a
sufficiently more violent change that probably it should be separate.

[estark, 2015/07/22 22:56:55]

Agree on both counts. I added that to crbug.com/508232 and can do it in a
follow-up CL. The next CL in this series allows the embedder to override the
security style for a resource, and I think I should tighten that to not allow
the embedder to set UNKNOWN as well.

+    case content::SECURITY_STYLE_UNAUTHENTICATED:
+      return true;
+    case content::SECURITY_STYLE_AUTHENTICATION_BROKEN:
+      return true;
+    case content::SECURITY_STYLE_WARNING:
+      return true;
+    case content::SECURITY_STYLE_AUTHENTICATED:

[palmer, 2015/07/22 22:30:00]

Nit: Maybe just implement these as fall-throughs?

[estark, 2015/07/22 22:56:55]

Done (I think -- can you please check that I did what you meant?)

+      return true;
+  }
+  return false;
+}
+
+}  // namespace
+
 namespace content {
 
-std::string SerializeSecurityInfo(
-    int cert_id,
-    net::CertStatus cert_status,
-    int security_bits,
-    int ssl_connection_status,
-    const SignedCertificateTimestampIDStatusList&
-        signed_certificate_timestamp_ids) {
+std::string SerializeSecurityInfo(SecurityStyle security_style,
+                                  int cert_id,
+                                  net::CertStatus cert_status,
+                                  int security_bits,
+                                  int ssl_connection_status,
+                                  const SignedCertificateTimestampIDStatusList&
+                                      signed_certificate_timestamp_ids) {
   base::Pickle pickle;
+  pickle.WriteInt(security_style);
   pickle.WriteInt(cert_id);
   pickle.WriteUInt32(cert_status);
   pickle.WriteInt(security_bits);
@@ -41,8 +64,9 @@ bool DeserializeSecurityInfo(const std::string& state, SSLStatus* ssl_status) {
 
   base::Pickle pickle(state.data(), static_cast<int>(state.size()));
   base::PickleIterator iter(pickle);
+  int security_style;
   int num_scts_to_read;
-  if (!iter.ReadInt(&ssl_status->cert_id) ||
+  if (!iter.ReadInt(&security_style) || !iter.ReadInt(&ssl_status->cert_id) ||
       !iter.ReadUInt32(&ssl_status->cert_status) ||
       !iter.ReadInt(&ssl_status->security_bits) ||
       !iter.ReadInt(&ssl_status->connection_status) ||
@@ -51,6 +75,13 @@ bool DeserializeSecurityInfo(const std::string& state, SSLStatus* ssl_status) {
     return false;
   }
 
+  if (!CheckSecurityStyle(security_style)) {
+    *ssl_status = SSLStatus();
+    return false;
+  }
+
+  ssl_status->security_style = static_cast<SecurityStyle>(security_style);

[davidben, 2015/07/22 20:56:58]

NOTE: We are now trusting the renderer to set security_style, which means the
browser process can't completely explode if security_style is inconsistent with
the rest of the SSLStatus. (This is probably fine?)

[estark, 2015/07/22 22:56:55]

I took a quick look through and this seems fine. I added a comment to the header
file to document it. There are definitely places where the browser would end up
somewhat confused (e.g. displaying confused security UI), but I don't think
that's fundamentally worse off than we currently are where the renderer can set
the rest of the fields. (As you said, the thing to worry about is probably the
browser exploding, which it doesn't look like it will.)

crbug.com/508232 is the general fix for this.

+
   // Sanity check |security_bits|: the only allowed negative value is -1.
   if (ssl_status->security_bits < -1) {
     *ssl_status = SSLStatus();