Bläddra i källkod

Implement request body consumption and reject invalid Content-Length with Transfer-Encoding to prevent request smuggling

yhirose 4 dagar sedan
förälder
incheckning
6fd97aeca0
2 ändrade filer med 196 tillägg och 19 borttagningar
  1. 53 19
      httplib.h
  2. 143 0
      test/test.cc

+ 53 - 19
httplib.h

@@ -1270,6 +1270,7 @@ struct Request {
   bool is_multipart_form_data() const;
 
   // private members...
+  bool body_consumed_ = false;
   size_t redirect_count_ = CPPHTTPLIB_REDIRECT_MAX_COUNT;
   size_t content_length_ = 0;
   ContentProvider content_provider_;
@@ -11288,6 +11289,8 @@ inline bool Server::read_content_core(
     return false;
   }
 
+  req.body_consumed_ = true;
+
   if (req.is_multipart_form_data()) {
     if (!multipart_form_data_parser.is_valid()) {
       res.status = StatusCode::BadRequest_400;
@@ -11906,8 +11909,19 @@ Server::process_request(Stream &strm, const std::string &remote_addr,
     return write_response(strm, close_connection, req, res);
   }
 
+  // RFC 9112 §6.3: Reject requests with both a non-zero Content-Length and
+  // any Transfer-Encoding to prevent request smuggling. Content-Length: 0 is
+  // tolerated for compatibility with existing clients.
+  if (req.get_header_value_u64("Content-Length") > 0 &&
+      req.has_header("Transfer-Encoding")) {
+    connection_closed = true;
+    res.status = StatusCode::BadRequest_400;
+    return write_response(strm, close_connection, req, res);
+  }
+
   // Check if the request URI doesn't exceed the limit
   if (req.target.size() > CPPHTTPLIB_REQUEST_URI_MAX_LENGTH) {
+    connection_closed = true;
     res.status = StatusCode::UriTooLong_414;
     output_error_log(Error::ExceedUriMaxLength, &req);
     return write_response(strm, close_connection, req, res);
@@ -11936,6 +11950,7 @@ Server::process_request(Stream &strm, const std::string &remote_addr,
   if (req.has_header("Accept")) {
     const auto &accept_header = req.get_header_value("Accept");
     if (!detail::parse_accept_header(accept_header, req.accept_content_types)) {
+      connection_closed = true;
       res.status = StatusCode::BadRequest_400;
       output_error_log(Error::HTTPParsing, &req);
       return write_response(strm, close_connection, req, res);
@@ -11945,6 +11960,7 @@ Server::process_request(Stream &strm, const std::string &remote_addr,
   if (req.has_header("Range")) {
     const auto &range_header_value = req.get_header_value("Range");
     if (!detail::parse_range_header(range_header_value, req.ranges)) {
+      connection_closed = true;
       res.status = StatusCode::RangeNotSatisfiable_416;
       output_error_log(Error::InvalidRangeHeader, &req);
       return write_response(strm, close_connection, req, res);
@@ -12072,6 +12088,7 @@ Server::process_request(Stream &strm, const std::string &remote_addr,
     }
   }
 #endif
+  auto ret = false;
   if (routed) {
     if (res.status == -1) {
       res.status = req.ranges.empty() ? StatusCode::OK_200
@@ -12079,6 +12096,7 @@ Server::process_request(Stream &strm, const std::string &remote_addr,
     }
 
     // Serve file content by using a content provider
+    auto file_open_error = false;
     if (!res.file_content_path_.empty()) {
       const auto &path = res.file_content_path_;
       auto mm = std::make_shared<detail::mmap>(path.c_str());
@@ -12088,37 +12106,53 @@ Server::process_request(Stream &strm, const std::string &remote_addr,
         res.content_provider_ = nullptr;
         res.status = StatusCode::NotFound_404;
         output_error_log(Error::OpenFile, &req);
-        return write_response(strm, close_connection, req, res);
-      }
+        file_open_error = true;
+      } else {
+        auto content_type = res.file_content_content_type_;
+        if (content_type.empty()) {
+          content_type = detail::find_content_type(
+              path, file_extension_and_mimetype_map_, default_file_mimetype_);
+        }
 
-      auto content_type = res.file_content_content_type_;
-      if (content_type.empty()) {
-        content_type = detail::find_content_type(
-            path, file_extension_and_mimetype_map_, default_file_mimetype_);
+        res.set_content_provider(
+            mm->size(), content_type,
+            [mm](size_t offset, size_t length, DataSink &sink) -> bool {
+              sink.write(mm->data() + offset, length);
+              return true;
+            });
       }
-
-      res.set_content_provider(
-          mm->size(), content_type,
-          [mm](size_t offset, size_t length, DataSink &sink) -> bool {
-            sink.write(mm->data() + offset, length);
-            return true;
-          });
     }
 
-    if (detail::range_error(req, res)) {
+    if (file_open_error) {
+      ret = write_response(strm, close_connection, req, res);
+    } else if (detail::range_error(req, res)) {
       res.body.clear();
       res.content_length_ = 0;
       res.content_provider_ = nullptr;
       res.status = StatusCode::RangeNotSatisfiable_416;
-      return write_response(strm, close_connection, req, res);
+      ret = write_response(strm, close_connection, req, res);
+    } else {
+      ret = write_response_with_content(strm, close_connection, req, res);
     }
-
-    return write_response_with_content(strm, close_connection, req, res);
   } else {
     if (res.status == -1) { res.status = StatusCode::NotFound_404; }
-
-    return write_response(strm, close_connection, req, res);
+    ret = write_response(strm, close_connection, req, res);
+  }
+
+  // Drain any unconsumed request body to prevent request smuggling on
+  // keep-alive connections.
+  if (!req.body_consumed_ && detail::expect_content(req)) {
+    int drain_status = 200; // required by read_content signature
+    if (!detail::read_content(
+            strm, req, payload_max_length_, drain_status, nullptr,
+            [](const char *, size_t, size_t, size_t) { return true; }, false)) {
+      // Body exceeds payload limit or read error — close the connection
+      // to prevent leftover bytes from being misinterpreted.
+      connection_closed = true;
+    }
   }
+
+  return ret;
 }
 
 inline bool Server::is_valid() const { return true; }

+ 143 - 0
test/test.cc

@@ -17266,3 +17266,146 @@ TEST(SymlinkTest, SymlinkEscapeFromBaseDirectory) {
   EXPECT_EQ(StatusCode::Forbidden_403, res->status);
 }
 #endif
+
+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.
+  //
+  // 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
+  // headers of the first request.
+  Server svr;
+  svr.set_mount_point("/", "./www");
+
+  std::atomic<int> smuggled_count(0);
+  svr.Get("/smuggled", [&](const Request &, Response &res) {
+    smuggled_count++;
+    res.set_content("oops", "text/plain");
+  });
+
+  auto port = svr.bind_to_any_port("localhost");
+  thread t = thread([&] { svr.listen_after_bind(); });
+  auto se = detail::scope_exit([&] {
+    svr.stop();
+    t.join();
+  });
+  svr.wait_until_ready();
+
+  auto error = Error::Success;
+  auto sock = detail::create_client_socket(
+      "localhost", "", port, AF_UNSPEC, false, false, nullptr,
+      /*connection_timeout_sec=*/2, 0,
+      /*read_timeout_sec=*/2, 0,
+      /*write_timeout_sec=*/2, 0, std::string(), error);
+  ASSERT_NE(INVALID_SOCKET, sock);
+  auto sock_se = detail::scope_exit([&] { detail::close_socket(sock); });
+
+  // The "smuggled" request will be sent as the body of the outer GET
+  std::string smuggled = "GET /smuggled HTTP/1.1\r\n"
+                         "Host: localhost\r\n"
+                         "Connection: close\r\n"
+                         "\r\n";
+
+  // Step 1: Send only the outer request headers (no body yet)
+  std::string outer_headers = "GET /file HTTP/1.1\r\n"
+                              "Host: localhost\r\n"
+                              "Content-Length: " +
+                              std::to_string(smuggled.size()) +
+                              "\r\n"
+                              "\r\n";
+
+  auto sent =
+      send(sock, outer_headers.data(), outer_headers.size(), MSG_NOSIGNAL);
+  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);
+    if (n <= 0) break;
+    first_response.append(buf, static_cast<size_t>(n));
+    // Stop once we have a complete response (headers + body)
+    auto hdr_end = first_response.find("\r\n\r\n");
+    if (hdr_end != std::string::npos) {
+      // Check for Content-Length to know when the body is complete
+      auto cl_pos = first_response.find("Content-Length:");
+      if (cl_pos != std::string::npos) {
+        auto cl_val_start = cl_pos + 15; // length of "Content-Length:"
+        auto cl_val_end = first_response.find("\r\n", cl_val_start);
+        auto cl = std::stoul(
+            first_response.substr(cl_val_start, cl_val_end - cl_val_start));
+        if (first_response.size() >= hdr_end + 4 + cl) { break; }
+      } else {
+        break; // No Content-Length, assume headers-only response
+      }
+    }
+  }
+  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.
+  // On a vulnerable server the keep-alive loop reads this as a second request.
+  sent = send(sock, smuggled.data(), smuggled.size(), MSG_NOSIGNAL);
+  ASSERT_EQ(static_cast<ssize_t>(smuggled.size()), sent);
+
+  // Step 4: Try to read a second response (should NOT exist after fix)
+  std::string second_response;
+  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
+  EXPECT_EQ(0, smuggled_count.load());
+}
+
+TEST(RequestSmugglingTest, ContentLengthAndTransferEncodingRejected) {
+  // RFC 9112 §6.3: A request with both Content-Length and Transfer-Encoding
+  // must be rejected with 400 Bad Request.
+  Server svr;
+  svr.Post("/test", [&](const Request &, Response &res) {
+    res.set_content("ok", "text/plain");
+  });
+
+  thread t = thread([&] { svr.listen(HOST, PORT); });
+  auto se = detail::scope_exit([&] {
+    svr.stop();
+    t.join();
+    ASSERT_FALSE(svr.is_running());
+  });
+  svr.wait_until_ready();
+
+  // Exact "chunked"
+  {
+    auto req = "POST /test HTTP/1.1\r\n"
+               "Host: localhost\r\n"
+               "Content-Length: 5\r\n"
+               "Transfer-Encoding: chunked\r\n"
+               "Connection: close\r\n"
+               "\r\n"
+               "hello";
+
+    std::string response;
+    ASSERT_TRUE(send_request(1, req, &response));
+    EXPECT_EQ("HTTP/1.1 400 Bad Request",
+              response.substr(0, response.find("\r\n")));
+  }
+
+  // Multi-valued Transfer-Encoding (e.g., "gzip, chunked")
+  {
+    auto req = "POST /test HTTP/1.1\r\n"
+               "Host: localhost\r\n"
+               "Content-Length: 5\r\n"
+               "Transfer-Encoding: gzip, chunked\r\n"
+               "Connection: close\r\n"
+               "\r\n"
+               "hello";
+
+    std::string response;
+    ASSERT_TRUE(send_request(1, req, &response));
+    EXPECT_EQ("HTTP/1.1 400 Bad Request",
+              response.substr(0, response.find("\r\n")));
+  }
+}