diff --git a/httplib.h b/httplib.h index 7f9813e7f9..efa06839c3 100644 --- a/httplib.h +++ b/httplib.h @@ -7504,6 +7504,13 @@ bool read_content(Stream &strm, T &x, size_t payload_max_length, int &status, inline ssize_t write_request_line(Stream &strm, const std::string &method, const std::string &path) { + // A request target must not carry CR/LF (or other control octets); otherwise + // a value smuggled into it splits the request line and injects headers or a + // whole request. The same field-value check already guards header values in + // check_and_write_headers and the request target in + // perform_websocket_handshake; apply it here too. + if (!fields::is_field_value(path)) { return -1; } + std::string s = method; s += ' '; s += path; @@ -13812,7 +13819,14 @@ inline bool ClientImpl::write_request(Stream &strm, Request &req, } // Write request line and headers - detail::write_request_line(bstrm, req.method, path_with_query); + if (detail::write_request_line(bstrm, req.method, path_with_query) < 0) { + // A rejected target (e.g. CR/LF smuggled in via a decoded redirect + // Location under set_path_encode(false)) must fail the request cleanly + // instead of emitting a request-line-less, header-injecting request. + error = Error::Write; + output_error_log(error, &req); + return false; + } if (!detail::check_and_write_headers(bstrm, req.headers, header_writer_, error)) { output_error_log(error, &req); diff --git a/test/test.cc b/test/test.cc index b1d4847622..3ecf4e19d0 100644 --- a/test/test.cc +++ b/test/test.cc @@ -7896,6 +7896,80 @@ TEST(ServerResponseSplittingTest, ChunkedTrailerCRLFInjection) { EXPECT_NE(std::string::npos, res.find("X-Safe: safe")); } +// Forward declaration: in split builds split.py strips `inline` and moves the +// definition into httplib.cc, so detail::write_request_line is not visible from +// the public httplib.h. Re-declaring it here lets the tests link against the +// symbol in both header-only and split builds. +namespace httplib { +namespace detail { +ssize_t write_request_line(Stream &strm, const std::string &method, + const std::string &path); +} // namespace detail +} // namespace httplib + +TEST(RequestLineInjectionTest, RejectsCRLFInTarget) { + // A well-formed target is written verbatim. + { + detail::BufferStream strm; + auto n = detail::write_request_line(strm, "GET", "/path?a=b"); + EXPECT_GT(n, 0); + EXPECT_EQ("GET /path?a=b HTTP/1.1\r\n", strm.get_buffer()); + } + + // A target carrying CR/LF must be rejected before anything reaches the wire, + // otherwise it splits the request line and injects a header or a whole + // request. This is what a decoded redirect Location ("%0D%0A") turns into + // when path encoding is disabled. + const std::string evil_targets[] = { + "/a\r\nInjected: pwned", + "/a\rInjected", + "/a\nInjected", + "/a\r\n\r\nGET /evil HTTP/1.1\r\nHost: victim\r\n\r\n", + }; + for (const auto &evil : evil_targets) { + detail::BufferStream strm; + auto n = detail::write_request_line(strm, "GET", evil); + EXPECT_LT(n, 0); + EXPECT_TRUE(strm.get_buffer().empty()); + } +} + +TEST(RequestLineInjectionTest, ClientRejectsCRLFTargetEndToEnd) { + // End-to-end counterpart to RejectsCRLFInTarget. With path encoding disabled + // the client transmits the target verbatim, so a CR/LF-bearing target -- what + // a redirect Location "%0D%0A" decodes to -- reaches write_request. The + // client must fail cleanly with Error::Write instead of putting a + // request-line-less, header-injecting request on the wire. + Server svr; + + svr.Get("/a", [](const Request &, Response &res) { + res.set_content("ok", "text/plain"); + }); + + auto thread = std::thread([&]() { svr.listen(HOST, PORT); }); + auto se = detail::scope_exit([&] { + svr.stop(); + thread.join(); + ASSERT_FALSE(svr.is_running()); + }); + + svr.wait_until_ready(); + + { + Client cli(HOST, PORT); + cli.set_path_encode(false); + // The request is rejected before anything is written, so the connection + // stays silent and the subsequent response read would otherwise block for + // the full default read timeout. Shorten it -- the assertion is on the + // error, not the timeout. + cli.set_read_timeout(1, 0); + + auto res = cli.Get("/a\r\nInjected: pwned"); + EXPECT_FALSE(res); + EXPECT_EQ(Error::Write, res.error()); + } +} + // Sends a raw request and verifies that there isn't a crash or exception. static void test_raw_request(const std::string &req, std::string *out = nullptr) {