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 63 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,

[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
 
 //-----------------------------------------------------------------------------
 
 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;
+  // 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;
 
   auth_controllers_[HttpAuth::AUTH_PROXY] = auth_controller;
   pending_auth_target_ = HttpAuth::AUTH_PROXY;
 
   DoCallback(OK);
 }
 
 void HttpNetworkTransaction::OnNeedsClientAuth(
     const SSLConfig& used_ssl_config,
     SSLCertRequestInfo* cert_info) {
   DCHECK_EQ(STATE_CREATE_STREAM_COMPLETE, next_state_);
 
   server_ssl_config_ = used_ssl_config;
   response_.cert_request_info = cert_info;
   OnIOComplete(ERR_SSL_CLIENT_AUTH_CERT_NEEDED);
 }
 
 void HttpNetworkTransaction::OnHttpsProxyTunnelResponse(
     const HttpResponseInfo& response_info,
     const SSLConfig& used_ssl_config,
     const ProxyInfo& used_proxy_info,
     HttpStream* stream) {
   DCHECK_EQ(STATE_CREATE_STREAM_COMPLETE, next_state_);
 
   CopyConnectionAttemptsFromStreamRequest();
 
+  // TODO(eustas): Should content encoding be checked here?
   headers_valid_ = true;
   response_ = response_info;
   server_ssl_config_ = used_ssl_config;
   proxy_info_ = used_proxy_info;
   if (stream_) {
     total_received_bytes_ += stream_->GetTotalReceivedBytes();
     total_sent_bytes_ += stream_->GetTotalSentBytes();
   }
   stream_.reset(stream);
   stream_request_.reset();  // we're done with the stream request
@@ skipping 652 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()) {
+    // 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.
   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/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