Parse HPKP report-uri and persist in TransportSecurityPersister

net/http/http_security_headers.cc

Index: net/http/http_security_headers.cc
diff --git a/net/http/http_security_headers.cc b/net/http/http_security_headers.cc
index d95e5878d7c4ce10e2e393d875fb254257d5b6de..1dd01bb690eaf6ddd21d2dbe5f6d7beb1fdb2ab4 100644
--- a/net/http/http_security_headers.cc
+++ b/net/http/http_security_headers.cc
@@ -92,29 +92,6 @@ bool IsPinListValid(const HashValueVector& pins,
          HashesIntersect(pins, from_cert_chain);
 }
 
-std::string Strip(const std::string& source) {
-  if (source.empty())
-    return source;
-
-  std::string::const_iterator start = source.begin();
-  std::string::const_iterator end = source.end();
-  HttpUtil::TrimLWS(&start, &end);
-  return std::string(start, end);
-}
-
-typedef std::pair<std::string, std::string> StringPair;
-
-StringPair Split(const std::string& source, char delimiter) {
-  StringPair pair;
-  size_t point = source.find(delimiter);
-
-  pair.first = source.substr(0, point);
-  if (std::string::npos != point)
-    pair.second = source.substr(point + 1);
-
-  return pair;
-}
-
 bool ParseAndAppendPin(const std::string& value,
                        HashValueTag tag,
                        HashValueVector* hashes) {
@@ -273,51 +250,66 @@ bool ParseHSTSHeader(const std::string& value,
   }
 }
 
-// "Public-Key-Pins" ":"
+// "Public-Key-Pins[-Report-Only]" ":"
 //     "max-age" "=" delta-seconds ";"
 //     "pin-" algo "=" base64 [ ";" ... ]
+//     [ ";" "includeSubdomains" ]
+//     [ ";" "report-uri" "=" uri-reference ]
 bool ParseHPKPHeader(const std::string& value,
                      const HashValueVector& chain_hashes,
                      base::TimeDelta* max_age,
                      bool* include_subdomains,
-                     HashValueVector* hashes) {
+                     HashValueVector* hashes,
+                     std::string* report_uri) {
   bool parsed_max_age = false;
   bool include_subdomains_candidate = false;
   uint32 max_age_candidate = 0;
   HashValueVector pins;
 
-  std::string source = value;
-
-  while (!source.empty()) {
-    StringPair semicolon = Split(source, ';');
-    semicolon.first = Strip(semicolon.first);
-    semicolon.second = Strip(semicolon.second);
-    StringPair equals = Split(semicolon.first, '=');
-    equals.first = Strip(equals.first);
-    equals.second = Strip(equals.second);
+  HttpUtil::NameValuePairsIterator name_value_pairs(
+      value.begin(), value.end(), ';',
+      HttpUtil::NameValuePairsIterator::VALUES_OPTIONAL);
 
-    if (base::LowerCaseEqualsASCII(equals.first, "max-age")) {
-      if (equals.second.empty() ||
-          !MaxAgeToInt(equals.second.begin(), equals.second.end(),
-                       &max_age_candidate)) {
+  while (name_value_pairs.GetNext()) {
+    if (base::LowerCaseEqualsASCII(name_value_pairs.name(), "max-age")) {
+      if (name_value_pairs.value().empty() ||

[Ryan Sleevi, 2015/07/16 02:25:44]

It's worth noting that you force a string coercion here, only to discard it,
only to make a new copy in MaxAgeToInt, which isn't needed because it's a
base::StringPiece.

Oh the silliness abounds here (in the existing code)

Suggestion:

if (!MaxAgeToInt(...begin(), ...end(), &max_age_candidate)) {
  return false;
}

and in MaxAgeToInt(iter begin, iter end, result) {
  const base::StringPiece s(begin, end);
  if (s.empty())
    return false;

  int64_t i = 0;
  ...
}

(since base::StringToInt64 is perfectly happy with a base::StringPiece)

Avoids all copies :)

[estark, 2015/07/16 04:52:18]

Done.

+          !MaxAgeToInt(name_value_pairs.value_begin(),
+                       name_value_pairs.value_end(), &max_age_candidate)) {
         return false;
       }
       parsed_max_age = true;
-    } else if (base::LowerCaseEqualsASCII(equals.first, "pin-sha1")) {
-      if (!ParseAndAppendPin(equals.second, HASH_VALUE_SHA1, &pins))
+    } else if (base::LowerCaseEqualsASCII(name_value_pairs.name(),
+                                          "pin-sha1")) {
+      if (!ParseAndAppendPin(name_value_pairs.raw_value(), HASH_VALUE_SHA1,

[Ryan Sleevi, 2015/07/16 02:25:44]

Man, never realized how many copies we were making for this, unnecessarily. Oh
well. Bonus cleanups for more StringPiece's (sorry, brettw@ keeps sending me
cleanups to reduce strings, this stood out as well)

(It's also unfortunate that we don't just update ParseAndAppendPin to expect the
value to be unquoted already, as a base::StringPiece, but that's neither here
nor there)

[estark, 2015/07/16 04:52:18]

Done, I think -- please check and make sure I did something that makes sense?
(Moved the quoted check outside of ParseAndAppendPin(); passed begin and end
iterators to ParseAndAppendPin() and constructed a StringPiece from them)

+                             &pins)) {
         return false;
-    } else if (base::LowerCaseEqualsASCII(equals.first, "pin-sha256")) {
-      if (!ParseAndAppendPin(equals.second, HASH_VALUE_SHA256, &pins))
+      }
+    } else if (base::LowerCaseEqualsASCII(name_value_pairs.name(),
+                                          "pin-sha256")) {
+      if (!ParseAndAppendPin(name_value_pairs.raw_value(), HASH_VALUE_SHA256,
+                             &pins)) {
         return false;
-    } else if (base::LowerCaseEqualsASCII(equals.first, "includesubdomains")) {
+      }
+    } else if (base::LowerCaseEqualsASCII(name_value_pairs.name(),
+                                          "includesubdomains")) {
       include_subdomains_candidate = true;
+    } else if (base::LowerCaseEqualsASCII(name_value_pairs.name(),
+                                          "report-uri")) {
+      // report-uris are always quoted.
+      if (!name_value_pairs.value_is_quoted())
+        return false;
+
+      *report_uri = name_value_pairs.value();
+      if (report_uri->empty())
+        return false;

[Ryan Sleevi, 2015/07/16 02:25:44]

So this makes me a little uncomfortable, because usually with API contracts, we
don't modify output variables unless/until we return true - see lines 319-321.

So I think you want a temporary here and to update it down there, to be entirely
consistent.

[Ryan Sleevi, 2015/07/16 02:25:44]

BUG: We parse the other fields for validity and conformance, but here, we don't
do anything to validate that report-uri is a valid URL that is understood.

It seems like we should at least run through a GURL and canonicalize it (by
virtue of the string ctor), and then check validity. This also implies that
report_uri should return a GURL (which would thus indicate it's always in a
canonical form).

For example, under the current code, report-uri of "foo" seems like it would be
valid and persisted. Have I missed a check?

[estark, 2015/07/16 04:52:18]

Done.

[estark, 2015/07/16 04:52:18]

Ah, good point... Done. I'm now passing around GURL references everywhere
instead of strings and just converting to/from strings when
serializing/deserializing to/from disk; do you think that makes sense?

     } else {
       // Silently ignore unknown directives for forward compatibility.
     }
-
-    source = semicolon.second;
   }
 
+  if (!name_value_pairs.valid())
+    return false;
+
   if (!parsed_max_age)
     return false;