Skip to content

Commit 17ba303

Browse files
yhiroseyhirose
authored
Merge commit from fork
* Fix HTTP Header Smuggling due to insecure trailers merge * Improve performance
1 parent 8027432 commit 17ba303

File tree

2 files changed

+156
-5
lines changed

2 files changed

+156
-5
lines changed

‎httplib.h‎

Lines changed: 89 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -450,6 +450,10 @@ struct hash {
450450
}
451451
};
452452

453+
template <typename T>
454+
using unordered_set = std::unordered_set<T, detail::case_ignore::hash,
455+
detail::case_ignore::equal_to>;
456+
453457
} // namespace case_ignore
454458

455459
// This is based on
@@ -710,6 +714,7 @@ struct Request {
710714
std::string matched_route;
711715
Params params;
712716
Headers headers;
717+
Headers trailers;
713718
std::string body;
714719

715720
std::string remote_addr;
@@ -744,6 +749,10 @@ struct Request {
744749
size_t get_header_value_count(const std::string &key) const;
745750
void set_header(const std::string &key, const std::string &val);
746751

752+
bool has_trailer(const std::string &key) const;
753+
std::string get_trailer_value(const std::string &key, size_t id = 0) const;
754+
size_t get_trailer_value_count(const std::string &key) const;
755+
747756
bool has_param(const std::string &key) const;
748757
std::string get_param_value(const std::string &key, size_t id = 0) const;
749758
size_t get_param_value_count(const std::string &key) const;
@@ -765,6 +774,7 @@ struct Response {
765774
int status = -1;
766775
std::string reason;
767776
Headers headers;
777+
Headers trailers;
768778
std::string body;
769779
std::string location; // Redirect location
770780

@@ -776,6 +786,10 @@ struct Response {
776786
size_t get_header_value_count(const std::string &key) const;
777787
void set_header(const std::string &key, const std::string &val);
778788

789+
bool has_trailer(const std::string &key) const;
790+
std::string get_trailer_value(const std::string &key, size_t id = 0) const;
791+
size_t get_trailer_value_count(const std::string &key) const;
792+
779793
void set_redirect(const std::string &url, int status = StatusCode::Found_302);
780794
void set_content(const char *s, size_t n, const std::string &content_type);
781795
void set_content(const std::string &s, const std::string &content_type);
@@ -4727,6 +4741,42 @@ inline ReadContentResult read_content_chunked(Stream &strm, T &x,
47274741
// chunked transfer coding data without the final CRLF.
47284742
if (!line_reader.getline()) { return ReadContentResult::Success; }
47294743

4744+
// RFC 7230 Section 4.1.2 - Headers prohibited in trailers
4745+
thread_local case_ignore::unordered_set<std::string> prohibited_trailers = {
4746+
// Message framing
4747+
"transfer-encoding", "content-length",
4748+
4749+
// Routing
4750+
"host",
4751+
4752+
// Authentication
4753+
"authorization", "www-authenticate", "proxy-authenticate",
4754+
"proxy-authorization", "cookie", "set-cookie",
4755+
4756+
// Request modifiers
4757+
"cache-control", "expect", "max-forwards", "pragma", "range", "te",
4758+
4759+
// Response control
4760+
"age", "expires", "date", "location", "retry-after", "vary", "warning",
4761+
4762+
// Payload processing
4763+
"content-encoding", "content-type", "content-range", "trailer"};
4764+
4765+
// Parse declared trailer headers once for performance
4766+
case_ignore::unordered_set<std::string> declared_trailers;
4767+
if (has_header(x.headers, "Trailer")) {
4768+
auto trailer_header = get_header_value(x.headers, "Trailer", "", 0);
4769+
auto len = std::strlen(trailer_header);
4770+
4771+
split(trailer_header, trailer_header + len, ',',
4772+
[&](const char *b, const char *e) {
4773+
std::string key(b, e);
4774+
if (prohibited_trailers.find(key) == prohibited_trailers.end()) {
4775+
declared_trailers.insert(key);
4776+
}
4777+
});
4778+
}
4779+
47304780
size_t trailer_header_count = 0;
47314781
while (strcmp(line_reader.ptr(), "\r\n") != 0) {
47324782
if (line_reader.size() > CPPHTTPLIB_HEADER_MAX_LENGTH) {
@@ -4744,11 +4794,12 @@ inline ReadContentResult read_content_chunked(Stream &strm, T &x,
47444794

47454795
parse_header(line_reader.ptr(), end,
47464796
[&](const std::string &key, const std::string &val) {
4747-
x.headers.emplace(key, val);
4797+
if (declared_trailers.find(key) != declared_trailers.end()) {
4798+
x.trailers.emplace(key, val);
4799+
trailer_header_count++;
4800+
}
47484801
});
47494802

4750-
trailer_header_count++;
4751-
47524803
if (!line_reader.getline()) { return ReadContentResult::Error; }
47534804
}
47544805

@@ -6468,6 +6519,24 @@ inline void Request::set_header(const std::string &key,
64686519
}
64696520
}
64706521

6522+
inline bool Request::has_trailer(const std::string &key) const {
6523+
return trailers.find(key) != trailers.end();
6524+
}
6525+
6526+
inline std::string Request::get_trailer_value(const std::string &key,
6527+
size_t id) const {
6528+
auto rng = trailers.equal_range(key);
6529+
auto it = rng.first;
6530+
std::advance(it, static_cast<ssize_t>(id));
6531+
if (it != rng.second) { return it->second; }
6532+
return std::string();
6533+
}
6534+
6535+
inline size_t Request::get_trailer_value_count(const std::string &key) const {
6536+
auto r = trailers.equal_range(key);
6537+
return static_cast<size_t>(std::distance(r.first, r.second));
6538+
}
6539+
64716540
inline bool Request::has_param(const std::string &key) const {
64726541
return params.find(key) != params.end();
64736542
}
@@ -6571,6 +6640,23 @@ inline void Response::set_header(const std::string &key,
65716640
headers.emplace(key, val);
65726641
}
65736642
}
6643+
inline bool Response::has_trailer(const std::string &key) const {
6644+
return trailers.find(key) != trailers.end();
6645+
}
6646+
6647+
inline std::string Response::get_trailer_value(const std::string &key,
6648+
size_t id) const {
6649+
auto rng = trailers.equal_range(key);
6650+
auto it = rng.first;
6651+
std::advance(it, static_cast<ssize_t>(id));
6652+
if (it != rng.second) { return it->second; }
6653+
return std::string();
6654+
}
6655+
6656+
inline size_t Response::get_trailer_value_count(const std::string &key) const {
6657+
auto r = trailers.equal_range(key);
6658+
return static_cast<size_t>(std::distance(r.first, r.second));
6659+
}
65746660

65756661
inline void Response::set_redirect(const std::string &url, int stat) {
65766662
if (detail::fields::is_field_value(url)) {

‎test/test.cc‎

Lines changed: 67 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4886,8 +4886,22 @@ TEST_F(ServerTest, GetStreamedChunkedWithTrailer) {
48864886
ASSERT_TRUE(res);
48874887
EXPECT_EQ(StatusCode::OK_200, res->status);
48884888
EXPECT_EQ(std::string("123456789"), res->body);
4889-
EXPECT_EQ(std::string("DummyVal1"), res->get_header_value("Dummy1"));
4890-
EXPECT_EQ(std::string("DummyVal2"), res->get_header_value("Dummy2"));
4889+
4890+
EXPECT_TRUE(res->has_header("Trailer"));
4891+
EXPECT_EQ(1U, res->get_header_value_count("Trailer"));
4892+
EXPECT_EQ(std::string("Dummy1, Dummy2"), res->get_header_value("Trailer"));
4893+
4894+
// Trailers are now stored separately from headers (security fix)
4895+
EXPECT_EQ(2U, res->trailers.size());
4896+
EXPECT_TRUE(res->has_trailer("Dummy1"));
4897+
EXPECT_TRUE(res->has_trailer("Dummy2"));
4898+
EXPECT_FALSE(res->has_trailer("Dummy3"));
4899+
EXPECT_EQ(std::string("DummyVal1"), res->get_trailer_value("Dummy1"));
4900+
EXPECT_EQ(std::string("DummyVal2"), res->get_trailer_value("Dummy2"));
4901+
4902+
// Verify trailers are NOT in headers (security verification)
4903+
EXPECT_EQ(std::string(""), res->get_header_value("Dummy1"));
4904+
EXPECT_EQ(std::string(""), res->get_header_value("Dummy2"));
48914905
}
48924906

48934907
TEST_F(ServerTest, LargeChunkedPost) {
@@ -10567,3 +10581,54 @@ TEST(ClientInThreadTest, Issue2068) {
1056710581
t.join();
1056810582
}
1056910583
}
10584+
10585+
TEST(HeaderSmugglingTest, ChunkedTrailerHeadersMerged) {
10586+
Server svr;
10587+
10588+
svr.Get("/", [](const Request &req, Response &res) {
10589+
EXPECT_EQ(2U, req.trailers.size());
10590+
10591+
EXPECT_FALSE(req.has_trailer("[invalid key...]"));
10592+
10593+
// Denied
10594+
EXPECT_FALSE(req.has_trailer("Content-Length"));
10595+
EXPECT_FALSE(req.has_trailer("X-Forwarded-For"));
10596+
10597+
// Accepted
10598+
EXPECT_TRUE(req.has_trailer("X-Hello"));
10599+
EXPECT_EQ(req.get_trailer_value("X-Hello"), "hello");
10600+
10601+
EXPECT_TRUE(req.has_trailer("X-World"));
10602+
EXPECT_EQ(req.get_trailer_value("X-World"), "world");
10603+
10604+
res.set_content("ok", "text/plain");
10605+
});
10606+
10607+
thread t = thread([&]() { svr.listen(HOST, PORT); });
10608+
auto se = detail::scope_exit([&] {
10609+
svr.stop();
10610+
t.join();
10611+
ASSERT_FALSE(svr.is_running());
10612+
});
10613+
10614+
svr.wait_until_ready();
10615+
10616+
const std::string req = "GET / HTTP/1.1\r\n"
10617+
"Transfer-Encoding: chunked\r\n"
10618+
"Trailer: X-Hello, X-World, X-AAA, X-BBB\r\n"
10619+
"\r\n"
10620+
"0\r\n"
10621+
"Content-Length: 10\r\n"
10622+
"Host: internal.local\r\n"
10623+
"Content-Type: malicious/content\r\n"
10624+
"Cookie: any\r\n"
10625+
"Set-Cookie: any\r\n"
10626+
"X-Forwarded-For: attacker.com\r\n"
10627+
"X-Real-Ip: 1.1.1.1\r\n"
10628+
"X-Hello: hello\r\n"
10629+
"X-World: world\r\n"
10630+
"\r\n";
10631+
10632+
std::string res;
10633+
ASSERT_TRUE(send_request(1, req, &res));
10634+
}

0 commit comments

Comments
 (0)