From 3599533c24630cdebb5d122e26b1782894231d59 Mon Sep 17 00:00:00 2001 From: yhirose Date: Wed, 21 Oct 2026 23:16:45 -0400 Subject: [PATCH] Drop invalid response header fields before any header writer The server left response header fields with an invalid name or value, such as a value carrying CR/LF that would split the response, for the default header writer to skip, so a writer set with set_header_writer() wrote them. Drop such fields from res.headers right after the handlers that can fill it directly have run, before the server reads it for framing and range decisions and before any header writer sees it. The client already rejects invalid request headers before writing them. --- httplib.h | 24 +++++++++----- test/test.cc | 88 +++++++++++++++++++++++++++++++--------------------- 2 files changed, 70 insertions(+), 42 deletions(-) diff --git a/httplib.h b/httplib.h index 72239556..666cd9d3 100644 --- a/httplib.h +++ b/httplib.h @@ -8419,6 +8419,18 @@ inline void set_header(Headers &headers, const std::string &key, if (fields::is_field_valid(key, val)) { headers.emplace(key, val); } } +// Drops the fields set_header() would refuse, from headers an application can +// fill directly: CR/LF in one would split the message. +inline void erase_invalid_fields(Headers &headers) { + for (auto it = headers.begin(); it != headers.end();) { + if (fields::is_field_valid(it->first, it->second)) { + ++it; + } else { + it = headers.erase(it); + } + } +} + inline bool read_headers(Stream &strm, Headers &headers) { const auto bufsiz = 2048; char buf[bufsiz]; @@ -8896,13 +8908,6 @@ inline ssize_t write_response_line(Stream &strm, int status) { inline ssize_t write_headers(Stream &strm, const Headers &headers) { ssize_t write_len = 0; for (const auto &x : headers) { - // Skip fields with invalid names or values to prevent response splitting - // via CR/LF injection, matching set_header(). The client validates request - // headers up front in check_and_write_headers, but the server passes - // res.headers straight to this writer, and res.headers is a public field - // an application can populate directly with request-derived values. - if (!fields::is_field_valid(x.first, x.second)) { continue; } - std::string s; s = x.first; s += ": "; @@ -14004,6 +14009,10 @@ inline bool Server::write_response_core(Stream &strm, bool close_connection, need_apply_ranges = true; } + // Before anything below reads res.headers, which the handlers may have + // filled directly + detail::erase_invalid_fields(res.headers); + if (!detail::body_allowed_for_status(res.status)) { detail::clear_content(res); } @@ -14047,6 +14056,7 @@ inline bool Server::write_response_core(Stream &strm, bool close_connection, if (post_routing_handler_) { post_routing_handler_(req, res); + detail::erase_invalid_fields(res.headers); // Again, for a body or framing headers the handler changed. detail::set_framing_headers(req, res, boundary, content_type); } diff --git a/test/test.cc b/test/test.cc index cc7931a8..f426c5d5 100644 --- a/test/test.cc +++ b/test/test.cc @@ -11231,48 +11231,66 @@ TEST(ServerResponseSplittingTest, ChunkedTrailerCRLFInjection) { } TEST(ServerResponseSplittingTest, ResponseHeaderCRLFInjection) { - Server svr; - svr.Get("/injected-header", [&](const Request & /*req*/, Response &res) { - // res.headers is a public field an application can populate directly, - // bypassing set_header()'s validation (e.g. reflecting a request value). - // A valid header that must survive. - res.headers.emplace("X-Safe", "safe"); - // Attacker-controlled value carrying CR/LF (response splitting). - res.headers.emplace("X-Reflected", - "legit\r\nInjected-Header: pwned\r\nX-Evil: also"); - // Attacker-controlled name carrying CR/LF. - res.headers.emplace("X-Bad\r\nSet-Cookie: a=1", "x"); - res.set_content("body", "text/plain"); - }); + // A custom header writer gets only the fields the default one writes + auto verbatim_writer = [](Stream &strm, Headers &hdrs) -> ssize_t { + std::string s; + for (const auto &h : hdrs) { + s += h.first + ": " + h.second + "\r\n"; + } + s += "\r\n"; + return strm.write(s.data(), s.size()); + }; - thread t = thread([&] { svr.listen(HOST, PORT); }); - auto se = detail::scope_exit([&] { - svr.stop(); - t.join(); - ASSERT_FALSE(svr.is_running()); - }); + for (auto custom_writer : {false, true}) { + SCOPED_TRACE(custom_writer ? "custom header writer" : "default writer"); + Server svr; + if (custom_writer) { svr.set_header_writer(verbatim_writer); } + svr.set_post_routing_handler([](const Request &, Response &res) { + res.headers.emplace("X-Late", "late\r\nInjected-Late: pwned"); + }); + svr.Get("/injected-header", [&](const Request & /*req*/, Response &res) { + // res.headers is a public field an application can populate directly, + // bypassing set_header()'s validation (e.g. reflecting a request value). + // A valid header that must survive. + res.headers.emplace("X-Safe", "safe"); + // Attacker-controlled value carrying CR/LF (response splitting). + res.headers.emplace("X-Reflected", + "legit\r\nInjected-Header: pwned\r\nX-Evil: also"); + // Attacker-controlled name carrying CR/LF. + res.headers.emplace("X-Bad\r\nSet-Cookie: a=1", "x"); + res.set_content("body", "text/plain"); + }); - svr.wait_until_ready(); + thread t = thread([&] { svr.listen(HOST, PORT); }); + auto se = detail::scope_exit([&] { + svr.stop(); + t.join(); + ASSERT_FALSE(svr.is_running()); + }); - const std::string req = std::string("GET /injected-header HTTP/1.1\r\n") + - "Host: " + HOST + - "\r\n" - "Connection: close\r\n" - "\r\n"; + svr.wait_until_ready(); - std::string res; - ASSERT_TRUE(send_request(5, req, &res)); + const std::string req = std::string("GET /injected-header HTTP/1.1\r\n") + + "Host: " + HOST + + "\r\n" + "Connection: close\r\n" + "\r\n"; - // The injected fields must not appear anywhere in the raw response. - EXPECT_EQ(std::string::npos, res.find("Injected-Header")); - EXPECT_EQ(std::string::npos, res.find("X-Evil")); - EXPECT_EQ(std::string::npos, res.find("Set-Cookie")); + std::string res; + ASSERT_TRUE(send_request(5, req, &res)); - // Invalid headers are dropped entirely, matching set_header(). - EXPECT_EQ(std::string::npos, res.find("X-Reflected:")); + // The injected fields must not appear anywhere in the raw response. + EXPECT_EQ(std::string::npos, res.find("Injected-Header")); + EXPECT_EQ(std::string::npos, res.find("X-Evil")); + EXPECT_EQ(std::string::npos, res.find("Set-Cookie")); + EXPECT_EQ(std::string::npos, res.find("Injected-Late")); - // The valid header still passes through. - EXPECT_NE(std::string::npos, res.find("X-Safe: safe")); + // Invalid headers are dropped entirely, matching set_header(). + EXPECT_EQ(std::string::npos, res.find("X-Reflected:")); + + // The valid header still passes through. + EXPECT_NE(std::string::npos, res.find("X-Safe: safe")); + } } // Forward declaration: in split builds split.py strips `inline` and moves the