Attach a SecurityStyle to each request in ResourceLoader

content/common/ssl_status_serialization.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/common/ssl_status_serialization.h"
 
 #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);
   pickle.WriteInt(ssl_connection_status);
   pickle.WriteInt(signed_certificate_timestamp_ids.size());
   for (SignedCertificateTimestampIDStatusList::const_iterator iter =
            signed_certificate_timestamp_ids.begin();
        iter != signed_certificate_timestamp_ids.end(); ++iter) {
     pickle.WriteInt(iter->id);
     pickle.WriteUInt16(iter->status);
   }
   return std::string(static_cast<const char*>(pickle.data()), pickle.size());
 }
 
 bool DeserializeSecurityInfo(const std::string& state, SSLStatus* ssl_status) {
   *ssl_status = SSLStatus();
 
   if (state.empty()) {
     // No SSL used.
     return true;
   }
 
   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) ||
       !iter.ReadInt(&num_scts_to_read)) {
     *ssl_status = SSLStatus();
     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();
     return false;
   }
 
   for (; num_scts_to_read > 0; --num_scts_to_read) {
     int id;
     uint16 status;
     if (!iter.ReadInt(&id) || !iter.ReadUInt16(&status)) {
       *ssl_status = SSLStatus();
       return false;
     }
 
     ssl_status->signed_certificate_timestamp_ids.push_back(
         SignedCertificateTimestampIDAndStatus(
             id, static_cast<net::ct::SCTVerifyStatus>(status)));
   }
 
   return true;
 }
 
 }  // namespace content