Reject unadvertised encodings

net/http/http_network_transaction.cc

Index: net/http/http_network_transaction.cc
diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index a5395d758bcc4c9d26ef115ca43da28ce9424a0c..aaeb7ab0d278a588de323e2b7df42ec9bbc6829d 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -81,6 +81,127 @@ std::unique_ptr<base::Value> NetLogSSLCipherFallbackCallback(
   return std::move(dict);
 }
 
+enum QualityParsingState {
+  BEFORE_QUALIFIER,  // waiting for ";"
+  QUALIFIER,         // waiting for "q"
+  EQUALS,            // waiting for "="
+  ZERO_OR_ONE,       // waiting for "0" or "1"
+  PERIOD,            // waiting for "."
+  DECIMALS,
+  DONE
+};
+
+// Tests is specified encoding is allowed by the given "Accept-Encoding" header.
+// It is expected that header value is somewhat well-formatted (according to the
+// RFC2616), and no "content-coding" is a prefix of another "content-coding".
+bool IsAcceptableEncoding(const std::string& accept_encoding,

[Randy Smith (Not in Mondays), 2017/03/15 16:35:31]

Confirming: This is here because there's no existing code in chrome to parse
client headers, specifically Accept-Encoding, because we generally create that
field?  This feels like the wrong place, but maybe there isn't a better one :-{.

[eustas, 2017/03/16 12:08:16]

Yup, hasn't found parsers for "Accept-xxx" header.
Perhaps it could be placed somewhere in net/base, but it might look strange, if
there will be only one user of it.

[Randy Smith (Not in Mondays), 2017/03/16 17:50:44]

Stick in a TODO to that effect?  (To move it somewhere more global if other code
needs it?)

I'm tempted to suggest moving it into net/filter along the lines of the earlier
comment, but that's not needed for this CL.  Maybe I'll do it for mine.

[eustas, 2017/03/20 13:05:18]

Reworked and moved to HttpUtil

+                          const std::string& encoding) {
+  size_t cursor = accept_encoding.find(encoding);
+  char c;
+
+  // Encoding is not mentioned.
+  if (cursor == std::string::npos)
+    return false;
+
+  // Check that match position is not in the middle of another entry.
+  if (cursor != 0) {
+    c = accept_encoding[cursor - 1];
+    if ((c != ' ') && (c != ','))
+      return false;
+  }
+  cursor += encoding.size();
+
+  QualityParsingState state = BEFORE_QUALIFIER;
+  int ones = 0;
+  int sum = 0;
+  int digits = 0;
+
+  for (; cursor < accept_encoding.size(); ++cursor) {
+    c = accept_encoding[cursor];
+    switch (state) {
+      case BEFORE_QUALIFIER:
+        if (c == ' ')
+          break;
+        if (c == ',')
+          return true;
+        if (c == ';') {
+          state = QUALIFIER;
+          break;
+        }
+        return false;
+
+      case QUALIFIER:
+        if (c == ' ')
+          break;
+        if (c == 'q' || c == 'Q') {
+          state = EQUALS;
+          break;
+        }
+        return false;
+
+      case EQUALS:
+        if (c == '=') {
+          state = ZERO_OR_ONE;
+          break;
+        }
+        return false;
+
+      case ZERO_OR_ONE:
+        if (c == '0' || c == '1') {
+          ones = c - '0';
+          state = PERIOD;
+          break;
+        }
+        return false;
+
+      case PERIOD:
+        if (c == '.') {
+          state = DECIMALS;
+          break;
+        }
+        return false;
+
+      case DECIMALS:
+        if (c == ',' || c == ' ') {
+          state = DONE;
+          break;
+        }
+        if (c >= '0' && c <= '9') {
+          sum += c - '0';
+          digits++;
+          break;
+        }
+        return false;
+
+      case DONE:
+        // assert(0)
+        break;
+    }
+    if (state == DONE)
+      break;
+  }
+
+  // Before "q".
+  if (state == BEFORE_QUALIFIER)
+    return true;
+
+  // Before value.
+  if (state == QUALIFIER || state == EQUALS || state == ZERO_OR_ONE)
+    return false;
+
+  // assert(state == PERIOD || state == DECIMALS || state == DONE
+  // Some value is read.
+
+  if (digits > 3)
+    return false;
+
+  if (ones == 0)
+    return (sum != 0);
+
+  // Reject values like "1.x", where x != 0
+  return (sum == 0);
+}
+
 }  // namespace
 
 //-----------------------------------------------------------------------------
@@ -545,6 +666,7 @@ void HttpNetworkTransaction::OnNeedsProxyAuth(
   establishing_tunnel_ = true;
   response_.headers = proxy_response.headers;
   response_.auth_challenge = proxy_response.auth_challenge;
+  // TODO(eustas): Should content encoding be checked here?

[Randy Smith (Not in Mondays), 2017/03/15 16:35:32]

Hmmm.  I'm inclined to think yes.  I'll ask Matt for an opinion, though; I don't
know the proxy code well.

[eustas, 2017/03/16 12:08:16]

Added similar check here.

   headers_valid_ = true;
   server_ssl_config_ = used_ssl_config;
   proxy_info_ = used_proxy_info;
@@ -574,6 +696,7 @@ void HttpNetworkTransaction::OnHttpsProxyTunnelResponse(
 
   CopyConnectionAttemptsFromStreamRequest();
 
+  // TODO(eustas): Should content encoding be checked here?
   headers_valid_ = true;
   response_ = response_info;
   server_ssl_config_ = used_ssl_config;
@@ -1246,6 +1369,12 @@ int HttpNetworkTransaction::DoReadHeadersComplete(int result) {
 
   DCHECK(response_.headers.get());
 
+  if (response_.headers.get() && !ContentEncodingsValid()) {
+    // TODO(eustas): report in UMA?

[Randy Smith (Not in Mondays), 2017/03/15 16:35:32]

See comment about which error.

[eustas, 2017/03/16 12:08:16]

Done.

+    // TODO(eustas): ignore, based on Finch flag?

[Randy Smith (Not in Mondays), 2017/03/15 16:35:32]

I'm inclined to think we can just watch the metrics and the screaming as this
rolls out, and roll it back if we've done something horrible.

[eustas, 2017/03/16 12:08:16]

Done.

+    return ERR_INVALID_CONTENT_ENCODING;
+  }
+
   // On a 408 response from the server ("Request Timeout") on a stale socket,
   // retry the request.
   // Headers can be NULL because of http://crbug.com/384554.
@@ -1711,4 +1840,44 @@ void HttpNetworkTransaction::CopyConnectionAttemptsFromStreamRequest() {
     connection_attempts_.push_back(attempt);
 }
 
+bool HttpNetworkTransaction::ContentEncodingsValid() const {
+  HttpResponseHeaders* headers = GetResponseHeaders();
+  DCHECK(headers);
+
+  // It is OK, if both "Accept-Encoding" and "Content-Encoding" headers are
+  // missing. It is a widely used setup in unit-tests.

[Randy Smith (Not in Mondays), 2017/03/15 16:35:32]

If this is specifically for unit tests, it's not very performance critical.  If
so, I'd suggest pulling the reading of the AcceptEncoding header up to here and
having an immediate bail in the while loop if it wasn't found; I think that'd be
easier to read code.  

[eustas, 2017/03/16 12:08:16]

Pulled GetHeader out of loop.

+
+  bool accept_encoding_acquired = false;
+  std::string accept_encoding;
+  std::string encoding;
+  size_t iter = 0;
+  while (headers->EnumerateHeader(&iter, "Content-Encoding", &encoding)) {
+    encoding = base::ToLowerASCII(encoding);
+    // RFC2616 Section 3.5 recommendation: applications SHOULD consider "x-gzip"
+    // and "x-compress" to be equivalent to "gzip" and "compress" respectively.
+    if (encoding.compare("x-gzip") == 0)
+      encoding = "gzip";
+    if (encoding.compare("x-compress") == 0)
+      encoding = "compress";
+
+    // Reject malformed encodings.
+    if (encoding.find_first_of("=;, ") != std::string::npos)
+      return false;
+
+    if (!accept_encoding_acquired) {
+      if (!request_headers_.GetHeader(HttpRequestHeaders::kAcceptEncoding,
+                                      &accept_encoding)) {
+        // URLRequestHttpJob sets the header if no value is provided.
+        // Other clients shoot themselves in a foot, if they don't.
+        return false;
+      }
+      accept_encoding_acquired = true;
+    }
+
+    if (!IsAcceptableEncoding(accept_encoding, encoding))
+      return false;
+  }
+  return true;
+}
+
 }  // namespace net