Net: Stop treating partial HTTP headers as a valid response.

net/http/http_stream_parser.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_stream_parser.h"
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
@@ skipping 11 matching lines @@
 #include "net/socket/client_socket_handle.h"
 #include "net/socket/ssl_client_socket.h"
 
 namespace net {
 
 namespace {
 
 enum HttpHeaderParserEvent {
   HEADER_PARSER_INVOKED = 0,
   HEADER_HTTP_09_RESPONSE = 1,
-  HEADER_ALLOWED_TRUNCATED_HEADERS = 2,
+  // Obsolete:  HEADER_ALLOWED_TRUNCATED_HEADERS = 2,
   HEADER_SKIPPED_WS_PREFIX = 3,
   HEADER_SKIPPED_NON_WS_PREFIX = 4,
   NUM_HEADER_EVENTS
 };
 
 void RecordHeaderParserEvent(HttpHeaderParserEvent header_event) {
   UMA_HISTOGRAM_ENUMERATION("Net.HttpHeaderParserEvent", header_event,
                             NUM_HEADER_EVENTS);
 }
 
@@ skipping 721 matching lines @@
 
 int HttpStreamParser::HandleReadHeaderResult(int result) {
   DCHECK_EQ(0, read_buf_unused_offset_);
 
   if (result == 0)
     result = ERR_CONNECTION_CLOSED;
 
   if (result < 0 && result != ERR_CONNECTION_CLOSED) {
     io_state_ = STATE_DONE;
     return result;
   }

[davidben, 2015/07/31 21:32:50]

If you want, you could further simplify checks by moving this block underneath
the ERR_CONNECTION_CLOSED codepath.

[mmenke, 2015/07/31 21:45:36]

Good idea, done.

-  // If we've used the connection before, then we know it is not a HTTP/0.9
-  // response and return ERR_CONNECTION_CLOSED.
-  if (result == ERR_CONNECTION_CLOSED && read_buf_->offset() == 0 &&
-      connection_->is_reused()) {
-    io_state_ = STATE_DONE;
+
+  if (result == ERR_CONNECTION_CLOSED) {
+    if (read_buf_->offset() == 0) {
+      io_state_ = STATE_DONE;
+      // If the connection has not been reused, it may have been a 0-length
+      // HTTP/0.9 responses, but it was most likely an error, so just return
+      // ERR_EMPTY_RESPONSE instead. If the connection was reused, just pass
+      // on the original connection close error, as rather than being an
+      // empty HTTP/0.9 response it's much more likely the server closed the
+      // socket before it received the request.
+      if (!connection_->is_reused())
+        return ERR_EMPTY_RESPONSE;
+      return result;
+    }
+
+    // The connection closed before the end of the headers. For HTTPS,
+    // accepting the partial headers headers is a potential security
+    // vulnerability.  For HTTP, it just doesn't seem like a good idea.
+    if (response_header_start_offset_ >= 0) {
+      io_state_ = STATE_DONE;
+      return ERR_RESPONSE_HEADERS_TRUNCATED;
+    }
+
+    // The response is apparently using HTTP/0.9.  Treat the entire response as
+    // the body.
+    int rv = ParseResponseHeaders(0);
+    if (rv < 0)
+      return rv;
     return result;
   }
 
   // Record our best estimate of the 'response time' as the time when we read
   // the first bytes of the response headers.
-  if (read_buf_->offset() == 0 && result != ERR_CONNECTION_CLOSED)
+  if (read_buf_->offset() == 0)
     response_->response_time = base::Time::Now();
 
-  if (result == ERR_CONNECTION_CLOSED) {
-    // The connection closed before we detected the end of the headers.
-    if (read_buf_->offset() == 0) {
-      // The connection was closed before any data was sent. Likely an error
-      // rather than empty HTTP/0.9 response.
-      io_state_ = STATE_DONE;
-      return ERR_EMPTY_RESPONSE;
-    } else if (request_->url.SchemeIsCryptographic()) {
-      // The connection was closed in the middle of the headers. For HTTPS we
-      // don't parse partial headers. Return a different error code so that we
-      // know that we shouldn't attempt to retry the request.
-      io_state_ = STATE_DONE;
-      return ERR_RESPONSE_HEADERS_TRUNCATED;
-    }
-    // Parse things as well as we can and let the caller decide what to do.
-    int end_offset;
-    if (response_header_start_offset_ >= 0) {
-      // The response looks to be a truncated set of HTTP headers.
-      io_state_ = STATE_READ_BODY_COMPLETE;
-      end_offset = read_buf_->offset();
-      RecordHeaderParserEvent(HEADER_ALLOWED_TRUNCATED_HEADERS);
-    } else {
-      // The response is apparently using HTTP/0.9.  Treat the entire response
-      // the body.
-      end_offset = 0;
-    }
-    int rv = ParseResponseHeaders(end_offset);
-    if (rv < 0)
-      return rv;
-    return result;
-  }
-
   read_buf_->set_offset(read_buf_->offset() + result);
   DCHECK_LE(read_buf_->offset(), read_buf_->capacity());
   DCHECK_GE(result,  0);
 
   int end_of_header_offset = FindAndParseResponseHeaders();
 
   // Note: -1 is special, it indicates we haven't found the end of headers.
   // Anything less than -1 is a net::Error, so we bail out.
   if (end_of_header_offset < -1)
     return end_of_header_offset;
@@ skipping 267 matching lines @@
       request_body->IsInMemory() &&
       request_body->size() > 0) {
     uint64 merged_size = request_headers.size() + request_body->size();
     if (merged_size <= kMaxMergedHeaderAndBodySize)
       return true;
   }
   return false;
 }
 
 }  // namespace net