No public description PiperOrigin-RevId: 866541000
diff --git a/quiche/balsa/balsa_enums.cc b/quiche/balsa/balsa_enums.cc index ab51738..24b2c8f 100644 --- a/quiche/balsa/balsa_enums.cc +++ b/quiche/balsa/balsa_enums.cc
@@ -90,6 +90,8 @@ return "INVALID_CHUNK_EXTENSION"; case INVALID_CHUNK_FRAMING: return "INVALID_CHUNK_FRAMING"; + case STRAY_DATA_AFTER_CHUNK: + return "STRAY_DATA_AFTER_CHUNK"; case MULTIPLE_CONTENT_LENGTH_KEYS: return "MULTIPLE_CONTENT_LENGTH_KEYS"; case MULTIPLE_TRANSFER_ENCODING_KEYS:
diff --git a/quiche/balsa/balsa_enums.h b/quiche/balsa/balsa_enums.h index 113da9e..718fbd8 100644 --- a/quiche/balsa/balsa_enums.h +++ b/quiche/balsa/balsa_enums.h
@@ -89,6 +89,7 @@ CHUNK_LENGTH_OVERFLOW, INVALID_CHUNK_EXTENSION, INVALID_CHUNK_FRAMING, + STRAY_DATA_AFTER_CHUNK, // Other errors. MULTIPLE_CONTENT_LENGTH_KEYS,
diff --git a/quiche/balsa/balsa_frame.cc b/quiche/balsa/balsa_frame.cc index ae45e55..c282212 100644 --- a/quiche/balsa/balsa_frame.cc +++ b/quiche/balsa/balsa_frame.cc
@@ -1305,6 +1305,7 @@ if (chunk_length_remaining_ == 0) { parse_state_ = BalsaFrameEnums::READING_CHUNK_TERM; + saw_slash_r_after_chunk_ = false; continue; } @@ -1323,8 +1324,36 @@ const char c = *current; ++current; - if (c == '\n') { + // Right after the chunk should be a \r then a \n. + if (c == '\r') { + if (saw_slash_r_after_chunk_) { + if (http_validation_policy().disallow_stray_data_after_chunk) { + HandleError(BalsaFrameEnums::STRAY_DATA_AFTER_CHUNK); + return current - input; + } else { + HandleWarning(BalsaFrameEnums::STRAY_DATA_AFTER_CHUNK); + } + } + saw_slash_r_after_chunk_ = true; + } else if (c == '\n') { + // Can't use last_char_was_slash_r_ because a \r might've been part + // of the chunk data. + if (!saw_slash_r_after_chunk_) { + if (http_validation_policy().disallow_stray_data_after_chunk) { + HandleError(BalsaFrameEnums::STRAY_DATA_AFTER_CHUNK); + return current - input; + } else { + HandleWarning(BalsaFrameEnums::STRAY_DATA_AFTER_CHUNK); + } + } break; + } else { + if (http_validation_policy().disallow_stray_data_after_chunk) { + HandleError(BalsaFrameEnums::STRAY_DATA_AFTER_CHUNK); + return current - input; + } else { + HandleWarning(BalsaFrameEnums::STRAY_DATA_AFTER_CHUNK); + } } } parse_state_ = BalsaFrameEnums::READING_CHUNK_LENGTH;
diff --git a/quiche/balsa/balsa_frame.h b/quiche/balsa/balsa_frame.h index 8e9c35c..c0338c5 100644 --- a/quiche/balsa/balsa_frame.h +++ b/quiche/balsa/balsa_frame.h
@@ -310,6 +310,10 @@ // set_continue_headers(). bool use_interim_headers_callback_ : 1; + // Whether we've already processed the CR that should come right after + // chunk data before the \n and then the next chunk length. + bool saw_slash_r_after_chunk_ : 1; + // This is not reset in Reset(). bool parse_truncated_headers_even_when_headers_too_long_ : 1; };
diff --git a/quiche/balsa/balsa_frame_test.cc b/quiche/balsa/balsa_frame_test.cc index 7925dd4..1757947 100644 --- a/quiche/balsa/balsa_frame_test.cc +++ b/quiche/balsa/balsa_frame_test.cc
@@ -2130,12 +2130,55 @@ message.size())); EXPECT_TRUE(balsa_frame_.MessageFullyRead()); EXPECT_FALSE(balsa_frame_.Error()); - EXPECT_EQ(BalsaFrameEnums::BALSA_NO_ERROR, balsa_frame_.ErrorCode()); + EXPECT_EQ(BalsaFrameEnums::STRAY_DATA_AFTER_CHUNK, balsa_frame_.ErrorCode()); EXPECT_EQ(message_body, body_input); EXPECT_EQ(message_body_data, body_data); } +TEST_F(HTTPBalsaFrameTest, StrayDataAfterChunkRejected) { + HttpValidationPolicy http_validation_policy{.disallow_stray_data_after_chunk = + true}; + balsa_frame_.set_http_validation_policy(http_validation_policy); + EXPECT_CALL(visitor_mock_, ProcessHeaders(_)); + EXPECT_CALL(visitor_mock_, HeaderDone()); + constexpr absl::string_view headers( + "POST / HTTP/1.1\r\n" + "transfer-encoding: chunked\r\n\r\n"); + ASSERT_EQ(headers.size(), + balsa_frame_.ProcessInput(headers.data(), headers.size())); + + constexpr absl::string_view body1("3\r\nfood\r\n"); + ASSERT_EQ(body1.size() - 2, + balsa_frame_.ProcessInput(body1.data(), body1.size())); + EXPECT_TRUE(balsa_frame_.Error()); + EXPECT_EQ(BalsaFrameEnums::STRAY_DATA_AFTER_CHUNK, balsa_frame_.ErrorCode()); +} + +// A LF character preceded by CR is allowed even if separated into multiple +// calls to ProcessInput(). +TEST_F(HTTPBalsaFrameTest, ChunkLoneCarriageReturnAtBoundaryWithLineFeed) { + EXPECT_CALL(visitor_mock_, ProcessHeaders(_)); + EXPECT_CALL(visitor_mock_, HeaderDone()); + constexpr absl::string_view headers( + "POST / HTTP/1.1\r\n" + "transfer-encoding: chunked\r\n\r\n"); + ASSERT_EQ(headers.size(), + balsa_frame_.ProcessInput(headers.data(), headers.size())); + + constexpr absl::string_view body1("3\r\nfoo\r"); + ASSERT_EQ(body1.size(), + balsa_frame_.ProcessInput(body1.data(), body1.size())); + EXPECT_FALSE(balsa_frame_.Error()); + ASSERT_EQ(BalsaFrameEnums::BALSA_NO_ERROR, balsa_frame_.ErrorCode()); + + constexpr absl::string_view body2("\n0\r\n\r\n"); + EXPECT_EQ(body2.size(), + balsa_frame_.ProcessInput(body2.data(), body2.size())); + EXPECT_FALSE(balsa_frame_.Error()); + EXPECT_EQ(BalsaFrameEnums::BALSA_NO_ERROR, balsa_frame_.ErrorCode()); +} + // Validates that chunked requests terminated by \r\n\n are accepted. // Note that this does not comply with // https://datatracker.ietf.org/doc/html/rfc9112#name-chunked-transfer-coding @@ -2396,7 +2439,7 @@ message.size())); EXPECT_TRUE(balsa_frame_.MessageFullyRead()); EXPECT_FALSE(balsa_frame_.Error()); - EXPECT_EQ(BalsaFrameEnums::OBS_FOLD_IN_HEADERS, balsa_frame_.ErrorCode()); + EXPECT_EQ(BalsaFrameEnums::STRAY_DATA_AFTER_CHUNK, balsa_frame_.ErrorCode()); EXPECT_EQ(message_body, body_input); EXPECT_EQ(message_body_data, body_data); @@ -2947,7 +2990,7 @@ message.size())); EXPECT_TRUE(balsa_frame_.MessageFullyRead()); EXPECT_FALSE(balsa_frame_.Error()); - EXPECT_EQ(BalsaFrameEnums::BALSA_NO_ERROR, balsa_frame_.ErrorCode()); + EXPECT_EQ(BalsaFrameEnums::STRAY_DATA_AFTER_CHUNK, balsa_frame_.ErrorCode()); EXPECT_EQ(message_body, body_input); EXPECT_EQ(message_body_data, body_data); @@ -3016,8 +3059,7 @@ message.size())); EXPECT_TRUE(balsa_frame_.MessageFullyRead()); EXPECT_FALSE(balsa_frame_.Error()); - EXPECT_EQ(BalsaFrameEnums::BALSA_NO_ERROR, balsa_frame_.ErrorCode()); - + EXPECT_EQ(BalsaFrameEnums::STRAY_DATA_AFTER_CHUNK, balsa_frame_.ErrorCode()); EXPECT_EQ(message_body, body_input); EXPECT_EQ(message_body_data, body_data); }
diff --git a/quiche/balsa/balsa_fuzz_util.cc b/quiche/balsa/balsa_fuzz_util.cc index 790439b..fc4ed71 100644 --- a/quiche/balsa/balsa_fuzz_util.cc +++ b/quiche/balsa/balsa_fuzz_util.cc
@@ -22,7 +22,7 @@ fuzztest::Arbitrary<bool>(), ArbitraryFirstLineValidationOption(), fuzztest::Arbitrary<bool>(), fuzztest::Arbitrary<bool>(), fuzztest::Arbitrary<bool>(), ArbitraryFirstLineValidationOption(), - fuzztest::Arbitrary<bool>(), + fuzztest::Arbitrary<bool>(), fuzztest::Arbitrary<bool>(), fuzztest::Arbitrary<bool>()); }
diff --git a/quiche/balsa/http_validation_policy.h b/quiche/balsa/http_validation_policy.h index d5c07da..fc5cd7e 100644 --- a/quiche/balsa/http_validation_policy.h +++ b/quiche/balsa/http_validation_policy.h
@@ -107,6 +107,10 @@ // or more space characters. bool sanitize_obs_fold_in_header_values = false; + // If true, rejects messages with stray bytes after a HTTP chunk (before the + // \r\n that separates it from the next chunk length). + bool disallow_stray_data_after_chunk = false; + // If true, disallow HTTP request methods that do not conform to RFC // 9110, Section 5.6.2. // https://datatracker.ietf.org/doc/html/rfc9110#section-5.6.2 @@ -148,6 +152,7 @@ "require_chunked_body_end_with_crlf_crlf=%v, " "sanitize_firstline_spaces=%v, " "sanitize_obs_fold_in_header_values=%v, " + "disallow_stray_data_after_chunk=%v, " "disallow_invalid_request_methods=%v}", policy.disallow_header_continuation_lines, policy.require_header_colon, @@ -166,6 +171,7 @@ policy.require_chunked_body_end_with_crlf_crlf, policy.sanitize_firstline_spaces, policy.sanitize_obs_fold_in_header_values, + policy.disallow_stray_data_after_chunk, policy.disallow_invalid_request_methods); } };