Reject unadvertised encodings

net/http/http_network_transaction.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 "net/http/http_network_transaction.h"
 
 #include <memory>
 #include <set>
 #include <utility>
 #include <vector>
@@ skipping 14 matching lines @@
 #include "base/values.h"
 #include "build/build_config.h"
 #include "net/base/auth.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/io_buffer.h"
 #include "net/base/load_flags.h"
 #include "net/base/load_timing_info.h"
 #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"
 #include "net/http/http_basic_stream.h"
 #include "net/http/http_chunked_decoder.h"
 #include "net/http/http_network_session.h"
 #include "net/http/http_proxy_client_socket.h"
 #include "net/http/http_proxy_client_socket_pool.h"
 #include "net/http/http_request_headers.h"
 #include "net/http/http_request_info.h"
@@ skipping 29 matching lines @@
 std::unique_ptr<base::Value> NetLogSSLCipherFallbackCallback(
     const GURL* url,
     int net_error,
     NetLogCaptureMode /* capture_mode */) {
   std::unique_ptr<base::DictionaryValue> dict(new base::DictionaryValue());
   dict->SetString("host_and_port", GetHostAndPort(*url));
   dict->SetInteger("net_error", net_error);
   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
 
 //-----------------------------------------------------------------------------
 
 HttpNetworkTransaction::HttpNetworkTransaction(RequestPriority priority,
                                                HttpNetworkSession* session)
     : pending_auth_target_(HttpAuth::AUTH_NONE),
       io_callback_(base::Bind(&HttpNetworkTransaction::OnIOComplete,
                               base::Unretained(this))),
       session_(session),
@@ skipping 444 matching lines @@
     const HttpResponseInfo& proxy_response,
     const SSLConfig& used_ssl_config,
     const ProxyInfo& used_proxy_info,
     HttpAuthController* auth_controller) {
   DCHECK(stream_request_.get());
   DCHECK_EQ(STATE_CREATE_STREAM_COMPLETE, next_state_);
 
   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;
 
   auth_controllers_[HttpAuth::AUTH_PROXY] = auth_controller;
   pending_auth_target_ = HttpAuth::AUTH_PROXY;
 
   DoCallback(OK);
 }
 
@@ skipping 681 matching lines @@
   // bizarre for SPDY. Assuming this logic is useful at all.
   // TODO(davidben): Bubble the error code up so we do not cache?
   if (result == ERR_CONNECTION_CLOSED && response_.headers.get())
     result = OK;
 
   if (result < 0)
     return HandleIOError(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.
   if (response_.headers.get() && response_.headers->response_code() == 408 &&
       stream_->IsConnectionReused()) {
     net_log_.AddEventWithNetErrorCode(
         NetLogEventType::HTTP_TRANSACTION_RESTART_AFTER_ERROR,
         response_.headers->response_code());
     // This will close the socket - it would be weird to try and reuse it, even
     // if the server doesn't actually close it.
@@ skipping 445 matching lines @@
 void HttpNetworkTransaction::CopyConnectionAttemptsFromStreamRequest() {
   DCHECK(stream_request_);
 
   // Since the transaction can restart with auth credentials, it may create a
   // stream more than once. Accumulate all of the connection attempts across
   // those streams by appending them to the vector:
   for (const auto& attempt : stream_request_->connection_attempts())
     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