Separate http_security_headers from transport_security_state

chrome/browser/ui/webui/net_internals/net_internals_ui.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 "chrome/browser/ui/webui/net_internals/net_internals_ui.h"
 
 #include <list>
 #include <string>
 #include <utility>
 #include <vector>
@@ skipping 88 matching lines @@
 // encounters a new version.  This should be incremented when significant
 // changes are made that will invalidate the old loading code.
 const int kLogFormatVersion = 1;
 
 // Returns the HostCache for |context|'s primary HostResolver, or NULL if
 // there is none.
 net::HostCache* GetHostResolverCache(net::URLRequestContext* context) {
   return context->host_resolver()->GetHostCache();
 }
 
+std::string HashesToBase64String(const net::HashValueVector& hashes) {
+  std::string str;
+  for (size_t i = 0; i != hashes.size(); ++i) {
+    if (i != 0)
+      str += ",";
+    str += hashes[i].ToString();
+  }
+  return str;
+}
+
+bool Base64StringToHashes(const std::string& hashes_str,
+                          net::HashValueVector* hashes) {
+  hashes->clear();
+  if (hashes_str.empty())
+    return true;
+  std::vector<std::string> vector_hash_str;
+  base::SplitString(hashes_str, ',', &vector_hash_str);
+
+  for (size_t i = 0; i != vector_hash_str.size(); ++i) {
+    std::string hash_str;
+    RemoveChars(vector_hash_str[i], " \t\r\n", &hash_str);
+    net::HashValue hash;
+    // Skip past unrecognized hash algos
+    if (hash_str.substr(0, 4) != "sha1" && hash_str.substr(0, 6) != "sha256")
+      continue;
+    if (!hash.FromString(hash_str))
+      return false;
+    hashes->push_back(hash);
+  }
+  return true;
+}
+
 // Returns the disk cache backend for |context| if there is one, or NULL.
 disk_cache::Backend* GetDiskCacheBackend(net::URLRequestContext* context) {
   if (!context->http_transaction_factory())
     return NULL;
 
   net::HttpCache* http_cache = context->http_transaction_factory()->GetCache();
   if (!http_cache)
     return NULL;
 
   return http_cache->GetCurrentBackend();
@@ skipping 974 matching lines @@
   // For example, turn "www.google.com" into "http://www.google.com".
   GURL url(URLFixerUpper::FixupURL(UTF16ToUTF8(url_str), std::string()));
 
   connection_tester_.reset(new ConnectionTester(
       this,
       io_thread_->globals()->proxy_script_fetcher_context.get(),
       net_log()));
   connection_tester_->RunAllTests(url);
 }
 
-void SPKIHashesToString(const net::HashValueVector& hashes,
-                        std::string* string) {
-  for (net::HashValueVector::const_iterator
-       i = hashes.begin(); i != hashes.end(); ++i) {
-    base::StringPiece hash_str(reinterpret_cast<const char*>(i->data()),
-                               i->size());
-    std::string encoded;
-    base::Base64Encode(hash_str, &encoded);
-
-    if (i != hashes.begin())
-      *string += ",";
-    *string += net::TransportSecurityState::HashValueLabel(*i) + encoded;
-  }
-}
-
 void NetInternalsMessageHandler::IOThreadImpl::OnHSTSQuery(
     const ListValue* list) {
   // |list| should be: [<domain to query>].
   std::string domain;
   CHECK(list->GetString(0, &domain));
   DictionaryValue* result = new DictionaryValue();
 
   if (!IsStringASCII(domain)) {
     result->SetString("error", "non-ASCII domain name");
   } else {
     net::TransportSecurityState* transport_security_state =
         context_getter_->GetURLRequestContext()->transport_security_state();
     if (!transport_security_state) {
       result->SetString("error", "no TransportSecurityState active");
     } else {
       net::TransportSecurityState::DomainState state;
       const bool found = transport_security_state->GetDomainState(
           domain, true, &state);
 
       result->SetBoolean("result", found);
       if (found) {
         result->SetInteger("mode", static_cast<int>(state.upgrade_mode));
         result->SetBoolean("subdomains", state.include_subdomains);
         result->SetString("domain", state.domain);
         result->SetDouble("expiry", state.upgrade_expiry.ToDoubleT());
         result->SetDouble("dynamic_spki_hashes_expiry",
                           state.dynamic_spki_hashes_expiry.ToDoubleT());
 
-        std::string hashes;
-        SPKIHashesToString(state.static_spki_hashes, &hashes);
-        result->SetString("static_spki_hashes", hashes);
+        std::string hashes_str;
+        hashes_str = HashesToBase64String(state.static_spki_hashes);
+        result->SetString("static_spki_hashes", hashes_str);

[Ryan Sleevi, 2012/12/07 23:37:21]

nit: Seems like we can just drop the temporary |hashes_str| here

result->SetString(
    "static_spki_hashes",
    HashesToBase64String(...));
result->SetString(
    "dynamic_spki_hashes"
    HashesToBase64String(...));

(not sure if it needs that much wrapping, may all fit on two lines < 80, like
1161 does )

[unsafe, 2012/12/08 09:22:42]

Done.

 
-        hashes.clear();
-        SPKIHashesToString(state.dynamic_spki_hashes, &hashes);
-        result->SetString("dynamic_spki_hashes", hashes);
+        hashes_str = HashesToBase64String(state.dynamic_spki_hashes);
+        result->SetString("dynamic_spki_hashes", hashes_str);
       }
     }
   }
 
   SendJavascriptCommand("receivedHSTSResult", result);
 }
 
 void NetInternalsMessageHandler::IOThreadImpl::OnHSTSAdd(
     const ListValue* list) {
   // |list| should be: [<domain to query>, <include subdomains>, <cert pins>].
@@ skipping 10 matching lines @@
   CHECK(list->GetString(2, &hashes_str));
 
   net::TransportSecurityState* transport_security_state =
       context_getter_->GetURLRequestContext()->transport_security_state();
   if (!transport_security_state)
     return;
 
   net::TransportSecurityState::DomainState state;
   state.upgrade_expiry = state.created + base::TimeDelta::FromDays(1000);
   state.include_subdomains = include_subdomains;
-  if (!hashes_str.empty()) {
-    std::vector<std::string> type_and_b64s;
-    base::SplitString(hashes_str, ',', &type_and_b64s);
-    for (std::vector<std::string>::const_iterator
-         i = type_and_b64s.begin(); i != type_and_b64s.end(); ++i) {
-      std::string type_and_b64;
-      RemoveChars(*i, " \t\r\n", &type_and_b64);
-      net::HashValue hash;
-      if (!net::TransportSecurityState::ParsePin(type_and_b64, &hash))
-        continue;
-
-      state.dynamic_spki_hashes.push_back(hash);
-    }
-  }
-
-  transport_security_state->EnableHost(domain, state);
+  if (Base64StringToHashes(hashes_str, &state.dynamic_spki_hashes))
+    transport_security_state->EnableHost(domain, state);

[Ryan Sleevi, 2012/12/07 23:37:21]

I find this new structure a little confusing, as it threw my reading at first.

Base64StringToHashes returns true for an empty string, which I think is
different/subtle, and hides an important detail when reading the code here -
that hashes_str can be empty (enabling HSTS without enabling HPKP)

I would suggest removing line 122-123 from Base64... and placing them here

if (!hashes_str.empty())
  Base64StringToHashes(...)

tss->EnableHost(...)


The problem here is I don't see a good way to respect Base64StringToHashes'
result, when examining the original code, and it's not clear to me whether that
represents 'bug' or 'feature'.

The old code ignored any bad hashes, but still enabled the HSTS. It seems like
some form of feedback mechanism is needed to allow OnHSTSAdd to indicate
success/failure, and then and only then check the result of
Base64StringToHashes.

[unsafe, 2012/12/08 09:22:42]

I moved lines 122-123 per your suggestion.  But I think a parse failure should
NOT cause HSTS or pins enablement.
Feedback is provided to the user since adding a pin/HSTS immediately queries and
displays the new pin/HSTS, so if a base64 error causes nothing to take effect,
the user should notice it.

 }
 
 void NetInternalsMessageHandler::IOThreadImpl::OnHSTSDelete(
     const ListValue* list) {
   // |list| should be: [<domain to query>].
   std::string domain;
   CHECK(list->GetString(0, &domain));
   if (!IsStringASCII(domain)) {
     // There cannot be a unicode entry in the HSTS set.
     return;
@@ skipping 566 matching lines @@
 }
 
 NetInternalsUI::NetInternalsUI(content::WebUI* web_ui)
     : WebUIController(web_ui) {
   web_ui->AddMessageHandler(new NetInternalsMessageHandler());
 
   // Set up the chrome://net-internals/ source.
   Profile* profile = Profile::FromWebUI(web_ui);
   ChromeURLDataManager::AddDataSource(profile, CreateNetInternalsHTMLSource());
 }