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..7d61e05987e31173961b1ae14a35c85e7e8450af 100644
--- a/net/http/http_security_headers.cc
+++ b/net/http/http_security_headers.cc
@@ -106,10 +106,11 @@ typedef std::pair<std::string, std::string> StringPair;
 
 StringPair Split(const std::string& source, char delimiter) {
   StringPair pair;
-  size_t point = source.find(delimiter);
+  size_t point = HttpUtil::FindDelimiter(source, 0, delimiter);

[Ryan Sleevi, 2015/06/27 12:13:59]

Hrm, this was previously dead code, and nothing uses this function - not even
unit tests!

It makes me wonder if we should be tweaking HttpUtil::NameValuePairsIterator to
allow optionally omitted values, which would at least be consistent with all the
other code we use.

[davidben, 2015/06/29 22:38:43]

That sounds reasonable to me, assuming you mean that value-less is some opt-in
mode for NameValuePairsIterator. Too bad there isn't a clear name for this
syntax that specs and NameValuePairsIterator could clearly refer to. There seem
to be a handful of parsing problems with the current code, like the one Emily
caught. We also don't accept quoted max-ages right now, if I'm reading this code
right, but RFC 6797 explicitly allows it quoted. <insert davidben's usual rant
about text-based encodings here>

Although note, by the way, that NameValuePairsIterator also accepts this broken
syntax:
http://crbug.com/39836


[Aside: Is HPKP quiiite the same syntax? I suspect this is a spec error but:

http://tools.ietf.org/html/rfc6797#section-6.1
This cites the "implied *LWS" variant and so it allows spaces (and the
line-folding craziness!) everywhere.

http://tools.ietf.org/html/rfc7469#section-2.1
This cites the RFC 7230 formulation (which says, if obsolete line folding is
supported, it's replaced with one or more SP before further processing) and has
explicit OWS in places. But there isn't OWS around the =.

Anyway, I'm guessing this is a spec error and so it's safe to use an omitted
values variant of NameValuePairsIterator.]

 
   pair.first = source.substr(0, point);
-  if (std::string::npos != point)
+
+  if (source.size() != point)
     pair.second = source.substr(point + 1);
 
   return pair;
@@ -273,14 +274,17 @@ 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;
@@ -311,6 +315,15 @@ bool ParseHPKPHeader(const std::string& value,
         return false;
     } else if (base::LowerCaseEqualsASCII(equals.first, "includesubdomains")) {
       include_subdomains_candidate = true;
+    } else if (base::LowerCaseEqualsASCII(equals.first, "report-uri")) {
+      // report-uris are always quoted.

[davidben, 2015/06/29 22:38:43]

Not quoting would be insane, but I don't actually see anything in the spec that
requires that. (Spec error? Pins explicitly say they have to be quoted.)

+      if (equals.second.empty() || !HttpUtil::IsQuote(equals.second[0]) ||
+          equals.second[0] != *equals.second.rbegin())
+        return false;
+
+      *report_uri = HttpUtil::Unquote(equals.second);
+      if (report_uri->empty())
+        return false;
     } else {
       // Silently ignore unknown directives for forward compatibility.
     }