Browse Source

Match chunked as the final transfer coding, order-independently (#2500)

is_chunked_transfer_encoding compared the whole Transfer-Encoding field
value against "chunked", so a message whose final coding is chunked but
which names another coding first ("gzip, chunked", valid under RFC 9112
6.1) was read as unframed. On a keep-alive server the body was then left
in the socket and parsed as a smuggled request; open_stream repeated the
check case-sensitively with a raw ==, desyncing the client stream the
same way.

Rework the helper to match the last coding token case-insensitively and
route open_stream through it so both paths agree. The codings may also be
split across multiple Transfer-Encoding lines (RFC 9110 5.3); since
Headers is an unordered_multimap whose duplicate-key iteration order is
not portable, the final coding of a multi-line field cannot be
determined reliably, so treat any such message that names chunked as
chunked (fail safe: a mis-parse only closes the connection, whereas the
opposite error enables smuggling). A unit test covers the helper,
including order-independent multi-line cases.

Based on #2487 by @metsw24-max.
yhirose 2 months ago
parent
commit
75938f08c7
2 changed files with 84 additions and 5 deletions
  1. 42 5
      httplib.h
  2. 42 0
      test/test.cc

+ 42 - 5
httplib.h

@@ -7386,8 +7386,46 @@ inline ReadContentResult read_content_chunked(Stream &strm, T &x,
 }
 
 inline bool is_chunked_transfer_encoding(const Headers &headers) {
-  return case_ignore::equal(
-      get_header_value(headers, "Transfer-Encoding", "", 0), "chunked");
+  // RFC 9112 6.1: a message is framed with the chunked coding when "chunked"
+  // is the final transfer coding. A single field value may list several
+  // codings ("gzip, chunked"), and the list may be split across multiple
+  // Transfer-Encoding header lines (RFC 9110 5.3). Match the last coding token
+  // case-insensitively rather than comparing the whole value against "chunked".
+  //
+  // Security: reading a chunked message as unframed leaves its body in the
+  // socket, where a keep-alive connection parses it as a smuggled request.
+  // Headers is an unordered_multimap whose iteration order for duplicate keys
+  // is not portable, so when there is more than one Transfer-Encoding line we
+  // cannot tell which coding is truly final. In that ambiguous case we fail
+  // safe by treating the message as chunked (a mis-parse just closes the
+  // connection, whereas the opposite error enables smuggling).
+  auto rng = headers.equal_range("Transfer-Encoding");
+
+  size_t line_count = 0;
+  bool chunked_present = false;
+  bool last_line_ends_with_chunked = false;
+
+  for (auto it = rng.first; it != rng.second; ++it) {
+    line_count++;
+    const auto &value = it->second;
+
+    std::string last_coding;
+    bool line_has_chunked = false;
+    split(value.data(), value.data() + value.size(), ',',
+          [&](const char *b, const char *e) {
+            last_coding.assign(b, e);
+            if (case_ignore::equal(last_coding, "chunked")) {
+              line_has_chunked = true;
+            }
+          });
+
+    if (line_has_chunked) { chunked_present = true; }
+    last_line_ends_with_chunked = case_ignore::equal(last_coding, "chunked");
+  }
+
+  if (line_count == 0) { return false; }
+  if (line_count == 1) { return last_line_ends_with_chunked; }
+  return chunked_present;
 }
 
 template <typename T, typename U>
@@ -13226,9 +13264,8 @@ ClientImpl::open_stream(const std::string &method, const std::string &path,
     handle.body_reader_.content_length = content_length;
   }
 
-  auto transfer_encoding =
-      handle.response->get_header_value("Transfer-Encoding");
-  handle.body_reader_.chunked = (transfer_encoding == "chunked");
+  handle.body_reader_.chunked =
+      detail::is_chunked_transfer_encoding(handle.response->headers);
 
   auto content_encoding = handle.response->get_header_value("Content-Encoding");
   if (!content_encoding.empty()) {

+ 42 - 0
test/test.cc

@@ -420,6 +420,48 @@ TEST(SanitizeFilenameTest, VariousPatterns) {
   EXPECT_EQ("", httplib::sanitize_filename("   "));
 }
 
+// Forward declaration (see the base64_encode note below): split builds move the
+// definition into httplib.cc, so re-declaring it here lets the test link.
+namespace httplib {
+namespace detail {
+bool is_chunked_transfer_encoding(const Headers &headers);
+} // namespace detail
+} // namespace httplib
+
+TEST(ChunkedTransferEncodingTest, DetectsChunkedAsFinalCoding) {
+  // Build a Headers with the given Transfer-Encoding lines (nullptr = none).
+  auto make = [](std::initializer_list<const char *> lines) {
+    Headers h;
+    for (auto te : lines) {
+      if (te) { h.emplace("Transfer-Encoding", te); }
+    }
+    return h;
+  };
+
+  // Sole coding, matched case-insensitively.
+  EXPECT_TRUE(detail::is_chunked_transfer_encoding(make({"chunked"})));
+  EXPECT_TRUE(detail::is_chunked_transfer_encoding(make({"Chunked"})));
+  EXPECT_TRUE(detail::is_chunked_transfer_encoding(make({"CHUNKED"})));
+
+  // RFC 9112 6.1: chunked as the final coding of a list still frames the body.
+  EXPECT_TRUE(detail::is_chunked_transfer_encoding(make({"gzip, chunked"})));
+  EXPECT_TRUE(detail::is_chunked_transfer_encoding(make({"gzip,chunked"})));
+  EXPECT_TRUE(detail::is_chunked_transfer_encoding(make({"gzip, Chunked "})));
+
+  // Single line: chunked absent, or not the final coding -> not chunk-framed.
+  EXPECT_FALSE(detail::is_chunked_transfer_encoding(make({"gzip"})));
+  EXPECT_FALSE(detail::is_chunked_transfer_encoding(make({"chunked, gzip"})));
+  EXPECT_FALSE(detail::is_chunked_transfer_encoding(make({""})));
+  EXPECT_FALSE(detail::is_chunked_transfer_encoding(make({nullptr})));
+
+  // Multiple Transfer-Encoding lines: iteration order for duplicate keys is not
+  // portable, so any line naming chunked is treated as chunked (fail safe).
+  // The result must not depend on the order the lines were added.
+  EXPECT_TRUE(detail::is_chunked_transfer_encoding(make({"gzip", "chunked"})));
+  EXPECT_TRUE(detail::is_chunked_transfer_encoding(make({"chunked", "gzip"})));
+  EXPECT_FALSE(detail::is_chunked_transfer_encoding(make({"gzip", "deflate"})));
+}
+
 // Forward declaration: in split builds split.py strips `inline` and moves the
 // definition into httplib.cc, so detail::base64_encode is not visible from the
 // public httplib.h. Re-declaring it here lets the tests link against the symbol