diff --git a/httplib.h b/httplib.h index 7622b740..e7d2f176 100644 --- a/httplib.h +++ b/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 - // otherwise let an intermediary and this parser disagree on where the body - // ends and enable request smuggling. Two cases: a non-zero Content-Length - // alongside any Transfer-Encoding (Content-Length: 0 is tolerated for - // compatibility with existing clients), and a Transfer-Encoding whose final - // coding is not chunked, which leaves the body length undeterminable. The - // latter must not fall through to the "no body" path, or the body bytes are - // parsed as the next request on a persistent connection. - if (detail::has_conflicting_content_length(req.headers) || + // RFC 9112 §6.3: Reject requests whose framing is invalid or ambiguous, + // which would otherwise let an intermediary and this parser disagree on + // where the body ends and enable request smuggling. Three cases: a + // Content-Length that is not a valid decimal length (e.g. "42, 42", "+42" + // or empty), which would otherwise be read as "no body"; a non-zero + // Content-Length alongside any Transfer-Encoding (Content-Length: 0 is + // tolerated for compatibility with existing clients); and a + // Transfer-Encoding whose final coding is not chunked, which leaves the body + // 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 (detail::has_header_token(res.headers, "Connection", "close")) { + if (!req.body_consumed_ && detail::has_framed_body(req) && + !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; - } } } diff --git a/test/test.cc b/test/test.cc index 0cdeaa1b..f7383094 100644 --- a/test/test.cc +++ b/test/test.cc @@ -25289,14 +25289,15 @@ TEST(SymlinkTest, SymlinkEscapeFromBaseDirectory) { } #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. +// Sends `outer_head` (with every "{len}" replaced by the length of an embedded +// "GET /smuggled" request), reads the first response, and only then sends the +// embedded request as the body. Returns how many times /smuggled ran. +// +// 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. +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" - "Host: localhost\r\n" - "Content-Length: " + - std::to_string(smuggled.size()) + - "\r\n" - "\r\n"; + auto sent = send(sock, head.data(), head.size(), 0); + EXPECT_EQ(static_cast(head.size()), sent); - auto sent = send(sock, outer_headers.data(), outer_headers.size(), 0); - ASSERT_EQ(static_cast(outer_headers.size()), sent); - - // Step 2: Read the first response (server serves file without reading body) - std::string first_response; + // Step 2: Read the 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. - // On a vulnerable server the keep-alive loop reads this as a second request. - sent = send(sock, smuggled.data(), smuggled.size(), 0); - ASSERT_EQ(static_cast(smuggled.size()), sent); + // 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. The + // server may already have closed the connection, so the result is ignored. + send(sock, smuggled.data(), smuggled.size(), 0); - // Step 4: Try to read a second response (should NOT exist after fix) - std::string second_response; + // Half-close so that a server which drained the body sees EOF and closes, + // 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(n)); } - // The smuggled request must NOT have been processed - EXPECT_EQ(0, smuggled_count.load()); + return 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) {