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 <yuji.hirose.bug@gmail.com>

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<size_t>::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<size_t>(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;

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 <typename ClientT>
-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<Response>();
   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 <typename ClientT>
+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());