Reject an invalid Content-Length and honor Connection: close

A request whose Content-Length was present but not a valid decimal length
(e.g. "42, 42", "+42", "0x2e" or empty) was treated as having no body
unless a handler read it: no 400 was returned, the body was not drained,
and the bytes after the header block were parsed as the next request on
the keep-alive connection. Reject such a request with 400 and close the
connection before routing, as RFC 9112 Section 6.3 requires.

The server also kept reading after a response that announced
Connection: close. A rejected request line or header block left the rest
of the message to be parsed as a new request, and an error response to a
bodyless request did the same with whatever followed. Close the
connection whenever the final response carries Connection: close
(RFC 9112 Section 9.6), and mark the two request-head rejection paths
closed explicitly as the other rejection paths already do.

httplib.h

@@ -14380,8 +14380,21 @@ Server::process_request(Stream &strm, const std::string &remote_addr,
   res.version = "HTTP/1.1";
   res.headers = default_headers_;
 
-  // Request line and headers
+  // RFC 9112 §9.6: a server that sends the "close" connection option must
+  // close the connection after that response, whichever path wrote it (an
+  // error status, a handler, or a rejected request). Reading on would also
+  // parse whatever the client sent next on a connection it considers done.
+  auto honor_connection_close = detail::scope_exit([&] {
+    if (detail::has_header_token(res.headers, "Connection", "close")) {
+      connection_closed = true;
+    }
+  });
+
+  // Request line and headers. A rejected message leaves the rest of it (and
+  // any body) unread, so the connection cannot be reused: the leftover bytes
+  // would be parsed as the next request.
   if (!parse_request_line(line_reader.ptr(), req)) {
+    connection_closed = true;
     res.status = StatusCode::BadRequest_400;
     output_error_log(Error::InvalidRequestLine, &req);
     return write_response(strm, close_connection, req, res);
@@ -14389,20 +14402,28 @@ Server::process_request(Stream &strm, const std::string &remote_addr,
 
   // Request headers
   if (!detail::read_headers(strm, req.headers)) {
+    connection_closed = true;
     res.status = StatusCode::BadRequest_400;
     output_error_log(Error::InvalidHeaders, &req);
     return write_response(strm, close_connection, req, res);
   }
 
-  // RFC 9112 §6.3: Reject requests whose framing is ambiguous, which would
+  // RFC 9112 §6.3: Reject requests whose framing is invalid or ambiguous,
-  // otherwise let an intermediary and this parser disagree on where the body
+  // which would otherwise let an intermediary and this parser disagree on
-  // ends and enable request smuggling. Two cases: a non-zero Content-Length
+  // where the body ends and enable request smuggling. Three cases: a
-  // alongside any Transfer-Encoding (Content-Length: 0 is tolerated for
+  // Content-Length that is not a valid decimal length (e.g. "42, 42", "+42"
-  // compatibility with existing clients), and a Transfer-Encoding whose final
+  // or empty), which would otherwise be read as "no body"; a non-zero
-  // coding is not chunked, which leaves the body length undeterminable. The
+  // Content-Length alongside any Transfer-Encoding (Content-Length: 0 is
-  // latter must not fall through to the "no body" path, or the body bytes are
+  // tolerated for compatibility with existing clients); and a
-  // parsed as the next request on a persistent connection.
+  // Transfer-Encoding whose final coding is not chunked, which leaves the body
-  if (detail::has_conflicting_content_length(req.headers) ||
+  // length undeterminable. None of them may fall through to the "no body"
+  // path, or the body bytes are parsed as the next request on a persistent
+  // connection.
+  auto is_invalid_content_length = false;
+  detail::get_header_value_u64(req.headers, "Content-Length", 0, 0,
+                               is_invalid_content_length);
+  if (is_invalid_content_length ||
+      detail::has_conflicting_content_length(req.headers) ||
       (req.has_header("Transfer-Encoding") &&
        !detail::is_chunked_transfer_encoding(req.headers))) {
     connection_closed = true;
@@ -14675,17 +14696,13 @@ Server::process_request(Stream &strm, const std::string &remote_addr,
   // keep-alive. Without framing there is no body to drain — reading would
   // consume the next request (issue #2450). If the response has committed the
   // connection to close, there is no next request to protect.
-  if (!req.body_consumed_ && detail::has_framed_body(req)) {
+  if (!req.body_consumed_ && detail::has_framed_body(req) &&
-    if (detail::has_header_token(res.headers, "Connection", "close")) {
+      !detail::has_header_token(res.headers, "Connection", "close")) {
+    int dummy_status;
+    if (!detail::read_content(
+            strm, req, payload_max_length_, dummy_status, nullptr,
+            [](const char *, size_t, size_t, size_t) { return true; }, false)) {
       connection_closed = true;
-    } else {
-      int dummy_status;
-      if (!detail::read_content(
-              strm, req, payload_max_length_, dummy_status, nullptr,
-              [](const char *, size_t, size_t, size_t) { return true; },
-              false)) {
-        connection_closed = true;
-      }
     }
   }
 

test/test.cc

@@ -25289,14 +25289,15 @@ TEST(SymlinkTest, SymlinkEscapeFromBaseDirectory) {
 }
 #endif
 
-TEST(RequestSmugglingTest, UnconsumedGETBodyOnFileHandler) {
+// Sends `outer_head` (with every "{len}" replaced by the length of an embedded
-  // A GET request with Content-Length to a static file handler must have its
+// "GET /smuggled" request), reads the first response, and only then sends the
-  // body drained before the keep-alive connection is reused. Otherwise the
+// embedded request as the body. Returns how many times /smuggled ran.
-  // unread body bytes are interpreted as the next HTTP request.
+//
-  //
+// The body is sent AFTER receiving the first response (as in the original
-  // The body is sent AFTER receiving the first response (as in the original
+// PoC) so that the stream_line_reader cannot buffer it together with the
-  // PoC) so that the stream_line_reader cannot buffer it together with the
+// headers of the first request.
-  // headers of the first request.
+static int count_smuggled_requests(const std::string &outer_head,
+                                   std::string &first_response) {
   Server svr;
   svr.set_mount_point("/", "./www");
 
@@ -25305,6 +25306,9 @@ TEST(RequestSmugglingTest, UnconsumedGETBodyOnFileHandler) {
     smuggled_count++;
     res.set_content("oops", "text/plain");
   });
+  svr.Post("/post", [&](const Request &req, Response &res) {
+    res.set_content(req.body, "text/plain");
+  });
 
   auto port = svr.bind_to_any_port("localhost");
   thread t = thread([&] { svr.listen_after_bind(); });
@@ -25320,28 +25324,28 @@ TEST(RequestSmugglingTest, UnconsumedGETBodyOnFileHandler) {
       /*connection_timeout_sec=*/2, 0,
       /*read_timeout_sec=*/2, 0,
       /*write_timeout_sec=*/2, 0, std::string(), error);
-  ASSERT_NE(INVALID_SOCKET, sock);
+  EXPECT_NE(INVALID_SOCKET, sock);
+  if (sock == INVALID_SOCKET) { return -1; }
   auto sock_se = detail::scope_exit([&] { detail::close_socket(sock); });
 
-  // The "smuggled" request will be sent as the body of the outer GET
+  // The "smuggled" request will be sent as the body of the outer request
   std::string smuggled = "GET /smuggled HTTP/1.1\r\n"
                          "Host: localhost\r\n"
                          "Connection: close\r\n"
                          "\r\n";
 
+  auto head = outer_head;
+  auto len = std::to_string(smuggled.size());
+  for (auto pos = head.find("{len}"); pos != std::string::npos;
+       pos = head.find("{len}", pos + len.size())) {
+    head.replace(pos, 5, len);
+  }
+
   // Step 1: Send only the outer request headers (no body yet)
-  std::string outer_headers = "GET /file HTTP/1.1\r\n"
+  auto sent = send(sock, head.data(), head.size(), 0);
-                              "Host: localhost\r\n"
+  EXPECT_EQ(static_cast<ssize_t>(head.size()), sent);
-                              "Content-Length: " +
-                              std::to_string(smuggled.size()) +
-                              "\r\n"
-                              "\r\n";
 
-  auto sent = send(sock, outer_headers.data(), outer_headers.size(), 0);
+  // Step 2: Read the first response
-  ASSERT_EQ(static_cast<ssize_t>(outer_headers.size()), sent);
-
-  // Step 2: Read the first response (server serves file without reading body)
-  std::string first_response;
   char buf[4096];
   for (;;) {
     auto n = recv(sock, buf, sizeof(buf), 0);
@@ -25363,23 +25367,103 @@ TEST(RequestSmugglingTest, UnconsumedGETBodyOnFileHandler) {
       }
     }
   }
-  ASSERT_TRUE(first_response.find("HTTP/1.1 200") != std::string::npos);
 
-  // Step 3: Now send the body, which looks like a new HTTP request.
+  // Step 3: Now send the body, which looks like a new HTTP request. On a
-  // On a vulnerable server the keep-alive loop reads this as a second request.
+  // vulnerable server the keep-alive loop reads this as a second request. The
-  sent = send(sock, smuggled.data(), smuggled.size(), 0);
+  // server may already have closed the connection, so the result is ignored.
-  ASSERT_EQ(static_cast<ssize_t>(smuggled.size()), sent);
+  send(sock, smuggled.data(), smuggled.size(), 0);
 
-  // Step 4: Try to read a second response (should NOT exist after fix)
+  // Half-close so that a server which drained the body sees EOF and closes,
-  std::string second_response;
+  // instead of the read below waiting for the read timeout.
+#ifdef _WIN32
+  ::shutdown(sock, SD_SEND);
+#else
+  ::shutdown(sock, SHUT_WR);
+#endif
+
+  // Step 4: Read until the server closes the connection
   for (;;) {
     auto n = recv(sock, buf, sizeof(buf), 0);
     if (n <= 0) break;
-    second_response.append(buf, static_cast<size_t>(n));
   }
 
-  // The smuggled request must NOT have been processed
+  return smuggled_count.load();
-  EXPECT_EQ(0, smuggled_count.load());
+}
+
+TEST(RequestSmugglingTest, UnconsumedGETBodyOnFileHandler) {
+  // A GET request with Content-Length to a static file handler must have its
+  // body drained before the keep-alive connection is reused. Otherwise the
+  // unread body bytes are interpreted as the next HTTP request.
+  std::string first_response;
+  EXPECT_EQ(0, count_smuggled_requests("GET /file HTTP/1.1\r\n"
+                                       "Host: localhost\r\n"
+                                       "Content-Length: {len}\r\n"
+                                       "\r\n",
+                                       first_response));
+  EXPECT_EQ(0u, first_response.find("HTTP/1.1 200"));
+}
+
+TEST(RequestSmugglingTest, InvalidContentLengthRejected) {
+  // RFC 9112 §6.3: a Content-Length that is not a valid decimal length must
+  // be answered with 400 and the connection closed. Treating it as "no body"
+  // leaves the body to be parsed as the next request.
+  const char *values[] = {"{len}, {len}", "+{len}", "0x2e", "", " {len}x"};
+  const char *targets[] = {
+      "GET /file",      // handler that never reads the body
+      "GET /not-found", // no handler matches (404)
+      "POST /post",     // handler that reads the body
+  };
+  for (auto target : targets) {
+    for (auto value : values) {
+      std::string first_response;
+      EXPECT_EQ(0, count_smuggled_requests(std::string(target) +
+                                               " HTTP/1.1\r\n"
+                                               "Host: localhost\r\n"
+                                               "Content-Length: " +
+                                               value + "\r\n\r\n",
+                                           first_response))
+          << target << " with Content-Length: " << value;
+      EXPECT_EQ(0u, first_response.find("HTTP/1.1 400"))
+          << target << " with Content-Length: " << value;
+    }
+  }
+}
+
+TEST(RequestSmugglingTest, RejectedRequestLineClosesConnection) {
+  // A 400 for an unparseable request line leaves the rest of the message
+  // unread, so the connection must be closed rather than reused.
+  std::string first_response;
+  EXPECT_EQ(0, count_smuggled_requests("FOO /file HTTP/1.1\r\n"
+                                       "Host: localhost\r\n"
+                                       "Content-Length: {len}\r\n"
+                                       "\r\n",
+                                       first_response));
+  EXPECT_EQ(0u, first_response.find("HTTP/1.1 400"));
+}
+
+TEST(RequestSmugglingTest, RejectedHeadersCloseConnection) {
+  // Same as above for a header block that fails to parse.
+  std::string first_response;
+  EXPECT_EQ(0, count_smuggled_requests("GET /file HTTP/1.1\r\n"
+                                       "Host: localhost\r\n"
+                                       "Bad Header\r\n"
+                                       "Content-Length: {len}\r\n"
+                                       "\r\n",
+                                       first_response));
+  EXPECT_EQ(0u, first_response.find("HTTP/1.1 400"));
+}
+
+TEST(RequestSmugglingTest, ErrorResponseClosesConnection) {
+  // RFC 9112 §9.6: an error response carries "Connection: close", so the
+  // server must not read another request on that connection, even when the
+  // request had no body to drain.
+  std::string first_response;
+  EXPECT_EQ(0, count_smuggled_requests("GET /not-found HTTP/1.1\r\n"
+                                       "Host: localhost\r\n"
+                                       "\r\n",
+                                       first_response));
+  EXPECT_EQ(0u, first_response.find("HTTP/1.1 404"));
+  EXPECT_NE(std::string::npos, first_response.find("Connection: close"));
 }
 
 TEST(RequestSmugglingTest, ContentLengthAndTransferEncodingRejected) {