Add HttpValidationPolicy controlling semicolon delimitation of chunk-exts

Balsa currently accepts requests where `chunk-size` contains illegal whitespace (SP or HTAB). For example, Balsa parses an `1 A` chunk-size as `1` and the chunk extension as ` A`. This is slightly incorrect.

There are two primary cases to consider:

1. `chunk-size` followed by `chunk-ext` + `CRLF` framing
2. `chunk-size` followed only by `CRLF` framing

For `chunk-size` without a `chunk-ext`, any trailing `SP/HTAB` is always illegal.

For `chunk-size` with a `chunk-ext`, there actually is a case where it’s valid to have whitespace after `chunk-size`. If that `chunk-ext` begins with `BWS`, it is perfectly legal to have `SP` or `HTAB` following the `chunk-size`. However, that is legal if and only if, the `BWS` is part of a `chunk-ext` and not superfluous before a `CRLF`.

Unfortunately, Balsa does not have any enforcement that `chunk-ext` contain a `;` which makes it difficult to differentiate between the `SP` / `HTAB` being `BWS` in a chunk-ext or erroneous trailing whitespace so we add enforcement that the `chunk-ext` contains a `;` and, if it is present, it is only preceded by `SP` or `HTAB`.

Protected by unused http validation policy.

PiperOrigin-RevId: 874812254

Author: reubent-goog

quiche/balsa/balsa_frame.cc

@@ -1237,10 +1237,10 @@ size_t BalsaFrame::ProcessInput(const char* input, size_t size) {
         continue;
 
       case BalsaFrameEnums::READING_CHUNK_EXTENSION: {
-        // TODO(phython): Convert this scanning to be 16 bytes at a time if
-        // there is data to be read.
         const char* extensions_start = current;
         size_t extensions_length = 0;
+        bool found_semicolon = false;
+        bool found_non_bws_before_semicolon = false;
         QUICHE_DCHECK_LE(current, end);
         while (true) {
           if (current == end) {
@@ -1251,6 +1251,12 @@ size_t BalsaFrame::ProcessInput(const char* input, size_t size) {
             return current - input;
           }
           const char c = *current;
+          if (c == ';') {
+            found_semicolon = true;
+          }
+          if (!found_semicolon && (c != ' ' && c != '\t')) {
+            found_non_bws_before_semicolon = true;
+          }
           if (!IsValidChunkExtensionCharacter(c, current, input, end)) {
             HandleError(BalsaFrameEnums::INVALID_CHUNK_EXTENSION);
             return current - input;
@@ -1270,6 +1276,14 @@ size_t BalsaFrame::ProcessInput(const char* input, size_t size) {
           }
         }
 
+        if (http_validation_policy_
+                .require_semicolon_delimited_chunk_extension &&
+            extensions_length > 0 &&
+            (!found_semicolon || found_non_bws_before_semicolon)) {
+          HandleError(BalsaFrameEnums::INVALID_CHUNK_EXTENSION);
+          return current - input;
+        }
+
         chunk_length_character_extracted_ = false;
         visitor_->OnChunkExtensionInput(
             absl::string_view(extensions_start, extensions_length));

quiche/balsa/balsa_frame_test.cc

@@ -24,6 +24,7 @@
 #include "absl/strings/str_format.h"
 #include "absl/strings/str_replace.h"
 #include "absl/strings/string_view.h"
+#include "absl/types/span.h"
 #include "quiche/balsa/balsa_enums.h"
 #include "quiche/balsa/balsa_headers.h"
 #include "quiche/balsa/balsa_visitor_interface.h"
@@ -47,6 +48,7 @@ using ::testing::NiceMock;
 using ::testing::Pointee;
 using ::testing::Property;
 using ::testing::Range;
+using ::testing::Sequence;
 using ::testing::StrEq;
 using ::testing::StrictMock;
 
@@ -5101,6 +5103,207 @@ TEST_F(HTTPBalsaFrameTest, ObsTextInReasonPhraseAllowed) {
   EXPECT_EQ(BalsaFrameEnums::BALSA_NO_ERROR, balsa_frame_.ErrorCode());
 }
 
+TEST_F(HTTPBalsaFrameTest, MoreChunkExtensions) {
+  struct TestCase {
+    absl::string_view chunks;  // chunks to process.
+    absl::Span<const size_t> expected_chunk_sizes;
+    absl::Span<const absl::string_view> expected_chunk_extensions;
+    HttpValidationPolicy policy;
+    BalsaFrameEnums::ErrorCode expected_error;
+  };
+
+  HttpValidationPolicy strict_chunks_policy;
+  strict_chunks_policy.disallow_stray_data_after_chunk = true;
+  strict_chunks_policy.disallow_lone_lf_in_chunk_extension = true;
+  strict_chunks_policy.disallow_lone_cr_in_chunk_extension = true;
+  strict_chunks_policy.require_chunked_body_end_with_crlf_crlf = true;
+
+  HttpValidationPolicy strict_chunk_and_ext_validation;
+  strict_chunk_and_ext_validation.disallow_stray_data_after_chunk = true;
+  strict_chunk_and_ext_validation.disallow_lone_lf_in_chunk_extension = true;
+  strict_chunk_and_ext_validation.disallow_lone_cr_in_chunk_extension = true;
+  strict_chunk_and_ext_validation.require_chunked_body_end_with_crlf_crlf =
+      true;
+  strict_chunk_and_ext_validation.require_semicolon_delimited_chunk_extension =
+      true;
+
+  std::vector<TestCase> cases = {
+      // no body, just the last-chunk
+      {"0\r\n"
+       "\r\n",
+       {0},
+       {""},
+       strict_chunks_policy,
+       BalsaFrameEnums::BALSA_NO_ERROR},
+
+      // no body, just last chunk with valid extension
+      {"0;chunked_extension=\"foobar\"quote\"\"\r\n"
+       "\r\n",
+       {0},
+       {";chunked_extension=\"foobar\"quote\"\""},
+       strict_chunks_policy,
+       BalsaFrameEnums::BALSA_NO_ERROR},
+
+      // Two valid chunks, last chunk has non-delimited extension
+      {"1;ext1\r\n"
+       "A\r\n"
+       "1;ext2\r\n"
+       "B\r\n"
+       "0 ext3\r\n"  // non-semicolon delimited extension
+       "\r\n",
+       {1, 1, 0},
+       {";ext1", ";ext2"},  // last-chunks's extension is rejected
+       strict_chunk_and_ext_validation,
+       BalsaFrameEnums::INVALID_CHUNK_EXTENSION},
+
+      // Two valid chunks, last chunk has non-delimited extension
+      {"1;ext1\r\n"
+       "A\r\n"
+       "1;ext2\r\n"
+       "B\r\n"
+       "0 ext3\r\n"  // non-semicolon delimited extension
+       "\r\n",
+       {1, 1, 0},
+       {";ext1", ";ext2", " ext3"},
+       strict_chunks_policy,
+       BalsaFrameEnums::BALSA_NO_ERROR},
+
+      // Trailing WS before semicolon
+      {"1 \t;ext\r\n"  // BWS before semicolon is valid
+       "Q\r\n"
+       "0\r\n"
+       "\r\n",
+       {1, 0},
+       {" \t;ext", ""},
+       strict_chunks_policy,
+       BalsaFrameEnums::BALSA_NO_ERROR},
+
+      // Non BWS before semicolon
+      {"1 \tnon-bws;ext\r\n"
+       "Q\r\n"
+       "0\r\n"
+       "\r\n",
+       {1, 0},
+       {" \tnon-bws;ext", ""},
+       strict_chunks_policy,
+       BalsaFrameEnums::BALSA_NO_ERROR},
+
+      // Non BWS before semicolon
+      {"1 \tnon-bws;ext\r\n"
+       "Q\r\n"
+       "0\r\n"
+       "\r\n",
+       {1},
+       {},
+       strict_chunk_and_ext_validation,
+       BalsaFrameEnums::INVALID_CHUNK_EXTENSION},
+
+      // BWS with no extension
+      {"1 \t \r\n"
+       "Q\r\n"
+       "0\r\n"
+       "\r\n",
+       {1},
+       {},
+       strict_chunk_and_ext_validation,
+       BalsaFrameEnums::INVALID_CHUNK_EXTENSION},
+
+      // BWS with no extension is invalid
+      {"1 \t \r\n"
+       "Q\r\n"
+       "0\r\n"
+       "\r\n",
+       {1, 0},
+       {" \t ", ""},
+       strict_chunks_policy,
+       BalsaFrameEnums::BALSA_NO_ERROR},
+
+      // Trailing BWS after semicolon
+      {"1; \t ext\r\n"
+       "Q\r\n"
+       "0\r\n"
+       "\r\n",
+       {1, 0},
+       {"; \t ext", ""},
+       strict_chunks_policy,
+       BalsaFrameEnums::BALSA_NO_ERROR},
+
+      // Trailing illegal characters before semicolon
+      {"1ERROR;ext\r\n"  // not valid hex
+       "Q\r\n"
+       "0\r\n"
+       "\r\n",
+       {},
+       {},
+       strict_chunks_policy,
+       BalsaFrameEnums::INVALID_CHUNK_LENGTH},
+
+      // Extension with semicolon & CRLF
+      {"1;ext=\";foo\"\r\n"
+       "Q\r\n"
+       "0\r\n"
+       "\r\n",
+       {1, 0},
+       {";ext=\";foo\"", ""},
+       strict_chunks_policy,
+       BalsaFrameEnums::BALSA_NO_ERROR},
+
+      // Bare semicolon
+      {"A;\r\n"
+       "0123456789\r\n"
+       "0\r\n"
+       "\r\n",
+       {10, 0},
+       {";", ""},
+       strict_chunks_policy,
+       BalsaFrameEnums::BALSA_NO_ERROR},
+  };
+
+  for (const TestCase& test : cases) {
+    SCOPED_TRACE(absl::StrCat("Testing chunks: ", absl::CEscape(test.chunks)));
+    NiceMock<BalsaVisitorMock> visitor_mock;
+    BalsaFrame balsa_frame;
+    BalsaHeaders headers;
+    balsa_frame.set_is_request(true);
+    balsa_frame.set_balsa_headers(&headers);
+    balsa_frame.set_http_validation_policy(test.policy);
+    balsa_frame.set_balsa_visitor(&visitor_mock);
+
+    std::string message_headers =
+        "POST / HTTP/1.1\r\n"
+        "Transfer-Encoding:  chunked\r\n"
+        "\r\n";
+
+    ASSERT_EQ(balsa_frame.ProcessInput(message_headers.data(),
+                                       message_headers.size()),
+              message_headers.size());
+    ASSERT_FALSE(balsa_frame.Error());
+
+    Sequence size_sequence, extension_sequence;
+    for (size_t chunk_size : test.expected_chunk_sizes) {
+      EXPECT_CALL(visitor_mock, OnChunkLength(chunk_size))
+          .InSequence(size_sequence);
+    }
+
+    for (absl::string_view extension : test.expected_chunk_extensions) {
+      EXPECT_CALL(visitor_mock, OnChunkExtensionInput(extension))
+          .InSequence(extension_sequence);
+    }
+
+    size_t bytes_consumed =
+        balsa_frame.ProcessInput(test.chunks.data(), test.chunks.size());
+    EXPECT_EQ(balsa_frame.ErrorCode(), test.expected_error);
+
+    if (test.expected_error == BalsaFrameEnums::BALSA_NO_ERROR) {
+      EXPECT_EQ(bytes_consumed, test.chunks.size());
+      EXPECT_TRUE(balsa_frame.MessageFullyRead());
+      EXPECT_FALSE(balsa_frame.Error());
+    }
+
+    Mock::VerifyAndClearExpectations(&visitor_mock);
+  }
+}
+
 TEST_F(HTTPBalsaFrameTest, MostRestrictiveTest) {
   BalsaFrame balsa_frame;
   BalsaHeaders headers;

quiche/balsa/balsa_fuzz_util.cc

@@ -23,7 +23,7 @@ fuzztest::Domain<HttpValidationPolicy> ArbitraryHttpValidationPolicy() {
       fuzztest::Arbitrary<bool>(), fuzztest::Arbitrary<bool>(),
       fuzztest::Arbitrary<bool>(), ArbitraryFirstLineValidationOption(),
       fuzztest::Arbitrary<bool>(), fuzztest::Arbitrary<bool>(),
-      fuzztest::Arbitrary<bool>());
+      fuzztest::Arbitrary<bool>(), fuzztest::Arbitrary<bool>());
 }
 
 fuzztest::Domain<HttpValidationPolicy::FirstLineValidationOption>

quiche/balsa/http_validation_policy.h

@@ -116,6 +116,14 @@ struct QUICHE_EXPORT HttpValidationPolicy {
   // https://datatracker.ietf.org/doc/html/rfc9110#section-5.6.2
   bool disallow_invalid_request_methods = false;
 
+  // Chunk extensions are optional but, if they are present, they MUST be
+  // preceded by a semicolon and follow the grammar:
+  // chunk-ext = *( BWS ";" BWS chunk-ext-name [ BWS "=" BWS chunk-ext-val ] )
+  // where BWS is SP or HTAB. See
+  // https://datatracker.ietf.org/doc/html/rfc9112#name-chunk-extensions for
+  // more information
+  bool require_semicolon_delimited_chunk_extension = false;
+
   template <typename Sink>
   friend void AbslStringify(Sink& sink, FirstLineValidationOption option) {
     switch (option) {
@@ -153,7 +161,8 @@ struct QUICHE_EXPORT HttpValidationPolicy {
                  "sanitize_firstline_spaces=%v, "
                  "sanitize_obs_fold_in_header_values=%v, "
                  "disallow_stray_data_after_chunk=%v, "
-                 "disallow_invalid_request_methods=%v}",
+                 "disallow_invalid_request_methods=%v, "
+                 "require_semicolon_delimited_chunk_extension=%v}",
                  policy.disallow_header_continuation_lines,
                  policy.require_header_colon,
                  policy.disallow_multiple_content_length,
@@ -172,7 +181,8 @@ struct QUICHE_EXPORT HttpValidationPolicy {
                  policy.sanitize_firstline_spaces,
                  policy.sanitize_obs_fold_in_header_values,
                  policy.disallow_stray_data_after_chunk,
-                 policy.disallow_invalid_request_methods);
+                 policy.disallow_invalid_request_methods,
+                 policy.require_semicolon_delimited_chunk_extension);
   }
 };
 
@@ -197,7 +207,8 @@ static constexpr HttpValidationPolicy kMostStrictHttpValidationPolicy = {
         quiche::HttpValidationPolicy::FirstLineValidationOption::SANITIZE,
     .sanitize_obs_fold_in_header_values = true,
     .disallow_stray_data_after_chunk = true,
-    .disallow_invalid_request_methods = true};
+    .disallow_invalid_request_methods = true,
+    .require_semicolon_delimited_chunk_extension = true};
 
 }  // namespace quiche