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