From 8b872605e0dc94691ce25fdb296e2b4e454c51c1 Mon Sep 17 00:00:00 2001 From: metsw24-max Date: Mon, 21 Sep 2026 05:52:52 +0530 Subject: [PATCH] reject control characters in chunk extensions in read_payload (#2585) * reject control characters in chunk extensions in read_payload * Bound every chunk-size line scan by the line terminator read_payload() ended its scans of one line buffer two different ways: the hex-size parse and the space skip that follows stopped on the NUL that stream_line_reader::append() writes, while the new chunk-ext check walked to an explicit end pointer. Compute that end pointer first and bound all of them by it, so no scan depends on the buffer's NUL and the terminator can never be read as line content. The bare-LF branch is reachable only under CPPHTTPLIB_ALLOW_LF_AS_LINE_TERMINATOR, where getline() ends the line on an LF that is the terminator rather than extension text. Say so: the comment below it explains why a bare LF inside the line is rejected, and without that note the two read as contradictory. Its guard no longer depends on the scan cursor either, since all it ever needed was a check that there is a byte to look at. * Reuse the chunked-body helper in the chunk-ext acceptance test AcceptsChunkExtension repeated expect_chunked_body_rejected()'s body verbatim apart from the expected status, so parameterise the helper on the status and keep the rejection wrapper for the existing callers. The decoded body is already checked by the /chunked handler, so asserting the status is all the new test needs. Also record why the control-character literal stays split: a hex escape consumes every hex digit that follows it, so "\x01b" would be the single byte \x1b rather than \x01 followed by 'b', and joining the halves would quietly change what the test sends. --------- Co-authored-by: yhirose --- httplib.h | 31 +++++++++++++++++++++++++++---- test/test.cc | 48 ++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 71 insertions(+), 8 deletions(-) diff --git a/httplib.h b/httplib.h index 97d48c80..4f221f84 100644 --- a/httplib.h +++ b/httplib.h @@ -15301,22 +15301,45 @@ inline ssize_t ChunkedDecoder::read_payload(char *buf, size_t len, stream_line_reader lr(strm, line_buf, sizeof(line_buf)); if (!lr.getline()) { return -1; } + // Everything below is bounded by eol rather than by the buffer's NUL, so + // the line terminator is never mistaken for line content. + const char *eol = lr.ptr() + lr.size(); + if (lr.end_with_crlf()) { + eol -= 2; + } else if (eol != lr.ptr() && eol[-1] == '\n') { + // Only reachable under CPPHTTPLIB_ALLOW_LF_AS_LINE_TERMINATOR, where + // getline() ends the line on a bare LF. That LF is the terminator, so it + // has to come off here or the check below would reject the line. + eol -= 1; + } + // RFC 9112 §7.1: chunk-size = 1*HEXDIG const char *p = lr.ptr(); int v = 0; - if (!is_hex(*p, v)) { return -1; } + if (p == eol || !is_hex(*p, v)) { return -1; } size_t chunk_len = 0; constexpr size_t chunk_len_max = (std::numeric_limits::max)(); - for (; is_hex(*p, v); ++p) { + for (; p < eol && is_hex(*p, v); ++p) { if (chunk_len > (chunk_len_max >> 4)) { return -1; } chunk_len = (chunk_len << 4) | static_cast(v); } - while (is_space_or_tab(*p)) { + while (p < eol && is_space_or_tab(*p)) { ++p; } - if (*p != '\0' && *p != ';' && *p != '\r' && *p != '\n') { return -1; } + + // RFC 9112 §7.1.1: only a chunk-ext may sit between the size and the line + // terminator, and it is built from tokens and quoted-strings, so it never + // holds a CR, LF or any other control character. getline() reads up to the + // CRLF, so a bare LF left in here would be swallowed as extension text + // while an intermediary that ends the line on it delimits the chunks + // differently, and the two disagree on where the body ends (request + // smuggling). + if (p < eol && *p != ';') { return -1; } + for (; p < eol; ++p) { + if (!is_space_or_tab(*p) && !fields::is_field_vchar(*p)) { return -1; } + } if (chunk_len == 0) { chunk_remaining = 0; diff --git a/test/test.cc b/test/test.cc index 48834a49..f8840cfd 100644 --- a/test/test.cc +++ b/test/test.cc @@ -6987,10 +6987,9 @@ TEST_F(ServerTest, CaseInsensitiveTransferEncoding) { EXPECT_EQ(StatusCode::OK_200, res->status); } -// GHSA-h6wq-j5mv-f3q8: the server must reject malformed chunk-size lines -// rather than treat them as valid lengths. template -static void expect_chunked_body_rejected(ClientT &cli, const char *body) { +static void expect_chunked_body_status(ClientT &cli, const char *body, + int expected_status) { Request req; req.method = "POST"; req.path = "/chunked"; @@ -7008,7 +7007,14 @@ static void expect_chunked_body_rejected(ClientT &cli, const char *body) { auto res = std::make_shared(); auto error = Error::Success; ASSERT_TRUE(cli.send(req, *res, error)); - EXPECT_EQ(StatusCode::BadRequest_400, res->status); + EXPECT_EQ(expected_status, res->status); +} + +// GHSA-h6wq-j5mv-f3q8: the server must reject malformed chunk-size lines +// rather than treat them as valid lengths. +template +static void expect_chunked_body_rejected(ClientT &cli, const char *body) { + expect_chunked_body_status(cli, body, StatusCode::BadRequest_400); } TEST_F(ServerTest, RejectsNegativeChunkSize) { @@ -7020,6 +7026,40 @@ TEST_F(ServerTest, RejectsChunkSizeWithLeadingPlus) { cli_, "+4\r\ndech\r\nf\r\nunked post body\r\n0\r\n\r\n"); } +// RFC 9112 §7.1.1: a chunk-ext is made of tokens and quoted-strings, so the +// chunk-size line carries no CR, LF or other control character ahead of its +// terminator. Such a line must be refused rather than read as extension text. +TEST_F(ServerTest, RejectsBareLFInChunkExtension) { + expect_chunked_body_rejected( + cli_, "4;\nxx\r\ndech\r\nf\r\nunked post body\r\n0\r\n\r\n"); +} + +TEST_F(ServerTest, RejectsBareLFAfterChunkSize) { + expect_chunked_body_rejected( + cli_, "4\nxx\r\ndech\r\nf\r\nunked post body\r\n0\r\n\r\n"); +} + +TEST_F(ServerTest, RejectsBareCRInChunkExtension) { + expect_chunked_body_rejected( + cli_, "4;a\rb\r\ndech\r\nf\r\nunked post body\r\n0\r\n\r\n"); +} + +TEST_F(ServerTest, RejectsControlCharacterInChunkExtension) { + // The literal stays split: a hex escape consumes every hex digit that + // follows, so "\x01b" would be the single byte \x1b, not \x01 then 'b'. + expect_chunked_body_rejected( + cli_, "4;a\x01" + "b\r\ndech\r\nf\r\nunked post body\r\n0\r\n\r\n"); +} + +TEST_F(ServerTest, AcceptsChunkExtension) { + expect_chunked_body_status(cli_, + "4;name=value\r\ndech\r\n" + "f ; note=\"a;b c\"\r\nunked post body\r\n" + "0;last\r\n\r\n", + StatusCode::OK_200); +} + TEST_F(ServerTest, GetStreamed2) { auto res = cli_.Get("/streamed", Headers{{make_range_header({{2, 3}})}}); ASSERT_TRUE(res) << "Error: " << to_string(res.error());