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

net/http/http_stream_parser.cc

Index: net/http/http_stream_parser.cc
diff --git a/net/http/http_stream_parser.cc b/net/http/http_stream_parser.cc
index 3c3b88d76d93dfed25087181b31c58f9aed250c7..d5578756d64a55d31d8a3f4421e54864dccd8565 100644
--- a/net/http/http_stream_parser.cc
+++ b/net/http/http_stream_parser.cc
@@ -29,7 +29,7 @@ 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
@@ -772,51 +772,49 @@ int HttpStreamParser::HandleReadHeaderResult(int result) {
     io_state_ = STATE_DONE;
     return result;
   }
-  // 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;
-    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)
-    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.
+      // 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;

[mmenke, 2015/07/31 19:11:00]

This code, in the HTTP case, is the only behavior that should change. 
Everything else is just a refactor, as the old code was rather silly.

     }
-    // 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;
+
+    if (request_->url.SchemeIsCryptographic()) {
+      // Reject HTTP/0.9 over HTTPS.
+      io_state_ = STATE_DONE;
+      // TODO(mmenke): This is a weird error to return in this case. Fix it.
+      return ERR_RESPONSE_HEADERS_TRUNCATED;
     }
-    int rv = ParseResponseHeaders(end_offset);
+
+    // 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)
+    response_->response_time = base::Time::Now();
+
   read_buf_->set_offset(read_buf_->offset() + result);
   DCHECK_LE(read_buf_->offset(), read_buf_->capacity());
   DCHECK_GE(result,  0);