SLFO-pool/grpc
SHA256
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
Files
main
grpc/grpc-CVE-2024-7246.patch

410 lines
18 KiB
Diff
Raw Permalink Normal View History

Sync from SUSE:SLFO:Main grpc revision a1ef6217985585227ea544ad7e8442b9
2025-02-25 19:22:32 +01:00
From 4a3092a29cf0cf53595c137c7b20909255f78923 Mon Sep 17 00:00:00 2001
From: Craig Tiller <ctiller@google.com>
Date: Thu, 1 Aug 2024 13:02:21 -0700
Subject: [PATCH] [v1.59] [chttp2] Fix a bug in hpack error handling (#37367)
PiperOrigin-RevId: 657234128
PiperOrigin-RevId: 658458047
---
.../chttp2/transport/hpack_parser.cc | 63 +++++++------
.../transport/chttp2/transport/hpack_parser.h | 2 +
.../transport/chttp2/hpack_parser_test.cc | 89 ++++++++++++++++---
.../transport/chttp2/hpack_sync_fuzzer.cc | 62 +++++++++++++
.../transport/chttp2/hpack_sync_fuzzer.proto | 3 +
5 files changed, 179 insertions(+), 40 deletions(-)
diff --git a/src/core/ext/transport/chttp2/transport/hpack_parser.cc b/src/core/ext/transport/chttp2/transport/hpack_parser.cc
index 31bf46456f1d0..f2fe80c504e58 100644
--- a/src/core/ext/transport/chttp2/transport/hpack_parser.cc
+++ b/src/core/ext/transport/chttp2/transport/hpack_parser.cc
@@ -91,12 +91,14 @@ constexpr Base64InverseTable kBase64InverseTable;
class HPackParser::Input {
public:
Input(grpc_slice_refcount* current_slice_refcount, const uint8_t* begin,
- const uint8_t* end, absl::BitGenRef bitsrc, HpackParseResult& error)
+ const uint8_t* end, absl::BitGenRef bitsrc,
+ HpackParseResult& frame_error, HpackParseResult& field_error)
: current_slice_refcount_(current_slice_refcount),
begin_(begin),
end_(end),
frontier_(begin),
- error_(error),
+ frame_error_(frame_error),
+ field_error_(field_error),
bitsrc_(bitsrc) {}
// If input is backed by a slice, retrieve its refcount. If not, return
@@ -215,14 +217,18 @@ class HPackParser::Input {
// Check if we saw an EOF
bool eof_error() const {
- return min_progress_size_ != 0 || error_.connection_error();
+ return min_progress_size_ != 0 || frame_error_.connection_error();
+ }
+
+ // Reset the field error to be ok
+ void ClearFieldError() {
+ if (field_error_.ok()) return;
+ field_error_ = HpackParseResult();
}
// Minimum number of bytes to unstuck the current parse
size_t min_progress_size() const { return min_progress_size_; }
- bool has_error() const { return !error_.ok(); }
-
// Set the current error - tweaks the error to include a stream id so that
// chttp2 does not close the connection.
// Intended for errors that are specific to a stream and recoverable.
@@ -246,10 +252,7 @@ class HPackParser::Input {
// read prior to being able to get further in this parse.
void UnexpectedEOF(size_t min_progress_size) {
GPR_ASSERT(min_progress_size > 0);
- if (min_progress_size_ != 0 || error_.connection_error()) {
- GPR_DEBUG_ASSERT(eof_error());
- return;
- }
+ if (eof_error()) return;
// Set min progress size, taking into account bytes parsed already but not
// consumed.
min_progress_size_ = min_progress_size + (begin_ - frontier_);
@@ -302,13 +305,18 @@ class HPackParser::Input {
// Do not use this directly, instead use SetErrorAndContinueParsing or
// SetErrorAndStopParsing.
void SetError(HpackParseResult error) {
- if (!error_.ok() || min_progress_size_ > 0) {
- if (error.connection_error() && !error_.connection_error()) {
- error_ = std::move(error); // connection errors dominate
+ SetErrorFor(frame_error_, error);
+ SetErrorFor(field_error_, std::move(error));
+ }
+
+ void SetErrorFor(HpackParseResult& error, HpackParseResult new_error) {
+ if (!error.ok() || min_progress_size_ > 0) {
+ if (new_error.connection_error() && !error.connection_error()) {
+ error = std::move(new_error); // connection errors dominate
}
return;
}
- error_ = std::move(error);
+ error = std::move(new_error);
}
// Refcount if we are backed by a slice
@@ -320,7 +328,8 @@ class HPackParser::Input {
// Frontier denotes the first byte past successfully processed input
const uint8_t* frontier_;
// Current error
- HpackParseResult& error_;
+ HpackParseResult& frame_error_;
+ HpackParseResult& field_error_;
// If the error was EOF, we flag it here by noting how many more bytes would
// be needed to make progress
size_t min_progress_size_ = 0;
@@ -597,6 +606,7 @@ class HPackParser::Parser {
bool ParseTop() {
GPR_DEBUG_ASSERT(state_.parse_state == ParseState::kTop);
auto cur = *input_->Next();
+ input_->ClearFieldError();
switch (cur >> 4) {
// Literal header not indexed - First byte format: 0000xxxx
// Literal header never indexed - First byte format: 0001xxxx
@@ -702,7 +712,7 @@ class HPackParser::Parser {
break;
}
gpr_log(
- GPR_DEBUG, "HTTP:%d:%s:%s: %s%s", log_info_.stream_id, type,
+ GPR_INFO, "HTTP:%d:%s:%s: %s%s", log_info_.stream_id, type,
log_info_.is_client ? "CLI" : "SVR", memento.md.DebugString().c_str(),
memento.parse_status == nullptr
? ""
@@ -951,11 +961,10 @@ class HPackParser::Parser {
state_.string_length)
: String::Parse(input_, state_.is_string_huff_compressed,
state_.string_length);
- HpackParseResult& status = state_.frame_error;
absl::string_view key_string;
if (auto* s = absl::get_if<Slice>(&state_.key)) {
key_string = s->as_string_view();
- if (status.ok()) {
+ if (state_.field_error.ok()) {
auto r = ValidateKey(key_string);
if (r != ValidateMetadataResult::kOk) {
input_->SetErrorAndContinueParsing(
@@ -965,7 +974,7 @@ class HPackParser::Parser {
} else {
const auto* memento = absl::get<const HPackTable::Memento*>(state_.key);
key_string = memento->md.key();
- if (status.ok() && memento->parse_status != nullptr) {
+ if (state_.field_error.ok() && memento->parse_status != nullptr) {
input_->SetErrorAndContinueParsing(*memento->parse_status);
}
}
@@ -992,16 +1001,16 @@ class HPackParser::Parser {
key_string.size() + value.wire_size + hpack_constants::kEntryOverhead;
auto md = grpc_metadata_batch::Parse(
key_string, std::move(value_slice), state_.add_to_table, transport_size,
- [key_string, &status, this](absl::string_view message, const Slice&) {
- if (!status.ok()) return;
+ [key_string, this](absl::string_view message, const Slice&) {
+ if (!state_.field_error.ok()) return;
input_->SetErrorAndContinueParsing(
HpackParseResult::MetadataParseError(key_string));
gpr_log(GPR_ERROR, "Error parsing '%s' metadata: %s",
std::string(key_string).c_str(),
std::string(message).c_str());
});
- HPackTable::Memento memento{std::move(md),
- status.PersistentStreamErrorOrNullptr()};
+ HPackTable::Memento memento{
+ std::move(md), state_.field_error.PersistentStreamErrorOrNullptr()};
input_->UpdateFrontier();
state_.parse_state = ParseState::kTop;
if (state_.add_to_table) {
@@ -1163,13 +1172,13 @@ grpc_error_handle HPackParser::Parse(
std::vector<uint8_t> buffer = std::move(unparsed_bytes_);
return ParseInput(
Input(nullptr, buffer.data(), buffer.data() + buffer.size(), bitsrc,
- state_.frame_error),
+ state_.frame_error, state_.field_error),
is_last, call_tracer);
}
- return ParseInput(
- Input(slice.refcount, GRPC_SLICE_START_PTR(slice),
- GRPC_SLICE_END_PTR(slice), bitsrc, state_.frame_error),
- is_last, call_tracer);
+ return ParseInput(Input(slice.refcount, GRPC_SLICE_START_PTR(slice),
+ GRPC_SLICE_END_PTR(slice), bitsrc, state_.frame_error,
+ state_.field_error),
+ is_last, call_tracer);
}
grpc_error_handle HPackParser::ParseInput(
diff --git a/src/core/ext/transport/chttp2/transport/hpack_parser.h b/src/core/ext/transport/chttp2/transport/hpack_parser.h
index 6b28daad61d4b..8f7132092958c 100644
--- a/src/core/ext/transport/chttp2/transport/hpack_parser.h
+++ b/src/core/ext/transport/chttp2/transport/hpack_parser.h
@@ -236,6 +236,8 @@ class HPackParser {
HPackTable hpack_table;
// Error so far for this frame (set by class Input)
HpackParseResult frame_error;
+ // Error so far for this field (set by class Input)
+ HpackParseResult field_error;
// Length of frame so far.
uint32_t frame_length = 0;
// Length of the string being parsed
diff --git a/test/core/transport/chttp2/hpack_parser_test.cc b/test/core/transport/chttp2/hpack_parser_test.cc
index 3772d909b9b8b..d5b9c6cb68da2 100644
--- a/test/core/transport/chttp2/hpack_parser_test.cc
+++ b/test/core/transport/chttp2/hpack_parser_test.cc
@@ -440,19 +440,82 @@ INSTANTIATE_TEST_SUITE_P(
Test{"Base64LegalEncoding",
{},
{},
- {// Binary metadata: created using:
- // tools/codegen/core/gen_header_frame.py
- // --compression inc --no_framing --output hexstr
- // < test/core/transport/chttp2/bad-base64.headers
- {"4009612e622e632d62696e1c6c75636b696c7920666f722075732c206974"
- "27732074756573646179",
- absl::InternalError("Error parsing 'a.b.c-bin' metadata: "
- "illegal base64 encoding"),
- 0},
- {"be",
- absl::InternalError("Error parsing 'a.b.c-bin' metadata: "
- "illegal base64 encoding"),
- 0}}},
+ {
+ // Binary metadata: created using:
+ // tools/codegen/core/gen_header_frame.py
+ // --compression inc --no_framing --output hexstr
+ // < test/core/transport/chttp2/bad-base64.headers
+ {"4009612e622e632d62696e1c6c75636b696c7920666f722075732c206974"
+ "27732074756573646179",
+ absl::InternalError("Error parsing 'a.b.c-bin' metadata: "
+ "illegal base64 encoding"),
+ 0},
+ {"be",
+ absl::InternalError("Error parsing 'a.b.c-bin' metadata: "
+ "illegal base64 encoding"),
+ kEndOfHeaders},
+ {"82", ":method: GET\n", 0},
+ }},
+ Test{"Base64LegalEncodingWorksAfterFailure",
+ {},
+ {},
+ {
+ // Binary metadata: created using:
+ // tools/codegen/core/gen_header_frame.py
+ // --compression inc --no_framing --output hexstr
+ // < test/core/transport/chttp2/bad-base64.headers
+ {"4009612e622e632d62696e1c6c75636b696c7920666f722075732c206974"
+ "27732074756573646179",
+ absl::InternalError("Error parsing 'a.b.c-bin' metadata: "
+ "illegal base64 encoding"),
+ 0},
+ {"be",
+ absl::InternalError("Error parsing 'a.b.c-bin' metadata: "
+ "illegal base64 encoding"),
+ 0},
+ {"400e636f6e74656e742d6c656e6774680135",
+ absl::InternalError("Error parsing 'a.b.c-bin' metadata: "
+ "illegal base64 encoding"),
+ kEndOfHeaders},
+ {"be", "content-length: 5\n", 0},
+ }},
+ Test{"Base64LegalEncodingWorksAfterFailure2",
+ {},
+ {},
+ {
+ {// Generated with: tools/codegen/core/gen_header_frame.py
+ // --compression inc --output hexstr --no_framing <
+ // test/core/transport/chttp2/MiXeD-CaSe.headers
+ "400a4d695865442d436153651073686f756c64206e6f74207061727365",
+ absl::InternalError("Illegal header key: MiXeD-CaSe"), 0},
+ // Binary metadata: created using:
+ // tools/codegen/core/gen_header_frame.py
+ // --compression inc --no_framing --output hexstr
+ // < test/core/transport/chttp2/bad-base64.headers
+ {"4009612e622e632d62696e1c6c75636b696c7920666f722075732c206974"
+ "27732074756573646179",
+ absl::InternalError("Illegal header key: MiXeD-CaSe"), 0},
+ {"be", absl::InternalError("Illegal header key: MiXeD-CaSe"),
+ 0},
+ {"400e636f6e74656e742d6c656e6774680135",
+ absl::InternalError("Illegal header key: MiXeD-CaSe"),
+ kEndOfHeaders},
+ {"be", "content-length: 5\n", 0},
+ {"bf",
+ absl::InternalError("Error parsing 'a.b.c-bin' metadata: "
+ "illegal base64 encoding"),
+ 0},
+ // Only the first error in each frame is reported, so we should
+ // still see the same error here...
+ {"c0",
+ absl::InternalError("Error parsing 'a.b.c-bin' metadata: "
+ "illegal base64 encoding"),
+ kEndOfHeaders},
+ // ... but if we look at the next frame we should see the
+ // stored error
+ {"c0", absl::InternalError("Illegal header key: MiXeD-CaSe"),
+ kEndOfHeaders},
+ }},
Test{"TeIsTrailers",
{},
{},
diff --git a/test/core/transport/chttp2/hpack_sync_fuzzer.cc b/test/core/transport/chttp2/hpack_sync_fuzzer.cc
index 81809afcf7175..40de14def3d92 100644
--- a/test/core/transport/chttp2/hpack_sync_fuzzer.cc
+++ b/test/core/transport/chttp2/hpack_sync_fuzzer.cc
@@ -86,6 +86,10 @@ void FuzzOneInput(const hpack_sync_fuzzer::Msg& msg) {
// Not an interesting case to fuzz
continue;
}
+ if (msg.check_ab_preservation() &&
+ header.literal_inc_idx().key() == "a") {
+ continue;
+ }
if (absl::EndsWith(header.literal_inc_idx().value(), "-bin")) {
std::ignore = encoder.EmitLitHdrWithBinaryStringKeyIncIdx(
Slice::FromCopiedString(header.literal_inc_idx().key()),
@@ -97,6 +101,10 @@ void FuzzOneInput(const hpack_sync_fuzzer::Msg& msg) {
}
break;
case hpack_sync_fuzzer::Header::kLiteralNotIdx:
+ if (msg.check_ab_preservation() &&
+ header.literal_not_idx().key() == "a") {
+ continue;
+ }
if (absl::EndsWith(header.literal_not_idx().value(), "-bin")) {
encoder.EmitLitHdrWithBinaryStringKeyNotIdx(
Slice::FromCopiedString(header.literal_not_idx().key()),
@@ -115,6 +123,10 @@ void FuzzOneInput(const hpack_sync_fuzzer::Msg& msg) {
break;
}
}
+ if (msg.check_ab_preservation()) {
+ std::ignore = encoder.EmitLitHdrWithNonBinaryStringKeyIncIdx(
+ Slice::FromCopiedString("a"), Slice::FromCopiedString("b"));
+ }
// STAGE 2: Decode the buffer (encode_output) into a list of headers
HPackParser parser;
@@ -141,6 +153,21 @@ void FuzzOneInput(const hpack_sync_fuzzer::Msg& msg) {
}
}
+ if (seen_errors.empty() && msg.check_ab_preservation()) {
+ std::string backing;
+ auto a_value = read_metadata.GetStringValue("a", &backing);
+ if (!a_value.has_value()) {
+ fprintf(stderr, "Expected 'a' header to be present: %s\n",
+ read_metadata.DebugString().c_str());
+ abort();
+ }
+ if (a_value != "b") {
+ fprintf(stderr, "Expected 'a' header to be 'b', got '%s'\n",
+ std::string(*a_value).c_str());
+ abort();
+ }
+ }
+
// STAGE 3: If we reached here we either had a stream error or no error
// parsing.
// Either way, the hpack tables should be of the same size between client and
@@ -169,6 +196,41 @@ void FuzzOneInput(const hpack_sync_fuzzer::Msg& msg) {
}
abort();
}
+
+ if (msg.check_ab_preservation()) {
+ SliceBuffer encode_output_2;
+ hpack_encoder_detail::Encoder encoder_2(
+ &compressor, msg.use_true_binary_metadata(), encode_output_2);
+ encoder_2.EmitIndexed(62);
+ GPR_ASSERT(encode_output_2.Count() == 1);
+ grpc_metadata_batch read_metadata_2(arena.get());
+ parser.BeginFrame(
+ &read_metadata_2, 1024, 1024, HPackParser::Boundary::EndOfHeaders,
+ HPackParser::Priority::None,
+ HPackParser::LogInfo{3, HPackParser::LogInfo::kHeaders, false});
+ auto err = parser.Parse(encode_output_2.c_slice_at(0), true,
+ absl::BitGenRef(proto_bit_src),
+ /*call_tracer=*/nullptr);
+ if (!err.ok()) {
+ fprintf(stderr, "Error parsing preservation encoded data: %s\n",
+ err.ToString().c_str());
+ abort();
+ }
+ std::string backing;
+ auto a_value = read_metadata_2.GetStringValue("a", &backing);
+ if (!a_value.has_value()) {
+ fprintf(stderr,
+ "Expected 'a' header to be present: %s\nfirst metadata: %s\n",
+ read_metadata_2.DebugString().c_str(),
+ read_metadata.DebugString().c_str());
+ abort();
+ }
+ if (a_value != "b") {
+ fprintf(stderr, "Expected 'a' header to be 'b', got '%s'\n",
+ std::string(*a_value).c_str());
+ abort();
+ }
+ }
}
} // namespace
diff --git a/test/core/transport/chttp2/hpack_sync_fuzzer.proto b/test/core/transport/chttp2/hpack_sync_fuzzer.proto
index 72792b60d640a..2c075a6abb1a1 100644
--- a/test/core/transport/chttp2/hpack_sync_fuzzer.proto
+++ b/test/core/transport/chttp2/hpack_sync_fuzzer.proto
@@ -44,4 +44,7 @@ message Msg {
repeated Header headers = 2;
grpc.testing.FuzzConfigVars config_vars = 3;
repeated uint64 random_numbers = 4;
+ // Ensure that a header "a: b" appended to headers with hpack incremental
+ // indexing is correctly added to the hpack table.
+ bool check_ab_preservation = 5;
}
Reference in New Issue Copy Permalink