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..be5abaf0f53e97189658443721dae78475e988e7 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -32,6 +32,7 @@
 #include "net/base/net_errors.h"
 #include "net/base/upload_data_stream.h"
 #include "net/base/url_util.h"
+#include "net/filter/source_stream.h"
 #include "net/http/http_auth.h"
 #include "net/http/http_auth_handler.h"
 #include "net/http/http_auth_handler_factory.h"
@@ -81,6 +82,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,
+                          const std::string& encoding) {
+  size_t cursor = accept_encoding.find(encoding);
+  char c;
+
+  // Encoding is not mentioned.
+  if (cursor == std::string::npos)
+    return false;

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

Put a note in here about explicitly not worrying about cases like:

Accept-Encoding: gzip;q=0, *
Content-Encoding: brotli

because we believe the higher layers of chrome will never do that to us?

(Note that

Accept-Encoding: identity;q=1, *;q=0

is a case we currently explicitly do have to support, but I believe this code
explicitly works for that case.  Might want to put a note in that we need to
support it, though, since it's not the type of A-E that this code is targeted
at.)

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

This code is gone.

+
+  // Check that match position is not in the middle of another entry.
+  if (cursor != 0) {
+    c = accept_encoding[cursor - 1];
+    if ((c != ' ') && (c != ','))

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

Do we need to check other whitespace?  (Tab specifically)

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

In new code whitespace is trimmed around all tokens.

+      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;

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

Is the thought that q=1 will never happen?  I'd sorta like to support it anyway.
 (It shows up in the above strange corner case, but we want a false return in
that case, so that's ok.  But I'd at least like to have comment documentation
about the ways in which this isn't a general test function, in case it's ever
made more general.)

ETA: Huh, you test q=1.  I'm missing how that's parsed--it looks to me like
we'll get to ZERO_OR_ONE, set ones to 1, go to PERIOD, not find a '.' and return
false.  What am I missing?

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

Ooops. Fixed it,... and then rewritten it to a more explicit parser.

+
+      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

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

Why not make this a DCHECK and remove the comments?

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

Acknowledged.

+  // 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 +667,14 @@ void HttpNetworkTransaction::OnNeedsProxyAuth(
   establishing_tunnel_ = true;
   response_.headers = proxy_response.headers;
   response_.auth_challenge = proxy_response.auth_challenge;
+
+  if (response_.headers.get() && !ContentEncodingsValid()) {
+    UMA_HISTOGRAM_ENUMERATION("Net.ContentDecodingFailed2.FilterType",
+                              SourceStream::TYPE_UNKNOWN,
+                              SourceStream::TYPE_MAX);
+    return ERR_CONTENT_DECODING_FAILED;
+  }
+
   headers_valid_ = true;
   server_ssl_config_ = used_ssl_config;
   proxy_info_ = used_proxy_info;
@@ -1246,6 +1376,13 @@ int HttpNetworkTransaction::DoReadHeadersComplete(int result) {
 
   DCHECK(response_.headers.get());
 
+  if (response_.headers.get() && !ContentEncodingsValid()) {
+    UMA_HISTOGRAM_ENUMERATION("Net.ContentDecodingFailed2.FilterType",
+                              SourceStream::TYPE_UNKNOWN,
+                              SourceStream::TYPE_MAX);
+    return ERR_CONTENT_DECODING_FAILED;
+  }
+
   // 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 +1848,38 @@ 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/16 17:50:44]

Suggestion: Move this comment down to above the "return false" and invert it? 
I.e. "It is only ok for Accept-Encoding to be empty if Content-Encoding is also
empty.  It is a widely used setup in unit tests."

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

Replaced with set-matching code, so now it is more explicit.

+  std::string accept_encoding;
+  request_headers_.GetHeader(HttpRequestHeaders::kAcceptEncoding,
+                             &accept_encoding);
+
+  std::string encoding;
+  size_t iter = 0;
+  while (headers->EnumerateHeader(&iter, "Content-Encoding", &encoding)) {
+    if (accept_encoding.empty())
+      return false;
+
+    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";

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

Do we need to worry about these cases in the Accept-Encoding header as well?

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

Made it symmetric in new parser.

+
+    // Reject malformed encodings.
+    if (encoding.find_first_of("=;, ") != std::string::npos)
+      return false;
+
+    if (!IsAcceptableEncoding(accept_encoding, encoding))
+      return false;
+  }
+  return true;
+}
+
 }  // namespace net