From e7c0a3df9fa029bfd74979a73c8ad652c801d6a1 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Fri, 10 Apr 2020 13:45:07 -0400 Subject: [PATCH 1/2] fuzz: improve header/data stop/continue modeling in HCM fuzzer. Previously we weren't tracking the status returned from the mock decodeData(), leading to incorrect invocation of continueDecoding(). This resulted in the underlying fuzz bug that this patch fixes. In addition to improve status state tracking, this PR also adds to the mock codec's ability to return stop/continue from decodeHeaders() and the full range of stop/continue values for decodeData(). Risk level: Low Testing: Corpus entry added. Fixes oss-fuzz issue https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=18378. Signed-off-by: Harvey Tuch --- ...nn_manager_impl_fuzz_test-5669833168912384 | 1 + test/common/http/conn_manager_impl_fuzz.proto | 5 ++- .../http/conn_manager_impl_fuzz_test.cc | 37 ++++++++++++++++--- 3 files changed, 36 insertions(+), 7 deletions(-) create mode 100644 test/common/http/conn_manager_impl_corpus/clusterfuzz-testcase-minimized-conn_manager_impl_fuzz_test-5669833168912384 diff --git a/test/common/http/conn_manager_impl_corpus/clusterfuzz-testcase-minimized-conn_manager_impl_fuzz_test-5669833168912384 b/test/common/http/conn_manager_impl_corpus/clusterfuzz-testcase-minimized-conn_manager_impl_fuzz_test-5669833168912384 new file mode 100644 index 0000000000000..a25a96c91dbd6 --- /dev/null +++ b/test/common/http/conn_manager_impl_corpus/clusterfuzz-testcase-minimized-conn_manager_impl_fuzz_test-5669833168912384 @@ -0,0 +1 @@ +actions { new_stream { request_headers { headers { key: ":path" value: "/" } headers { key: ":authority" } } } } actions { stream_action { request { continue_decoding { } } } } diff --git a/test/common/http/conn_manager_impl_fuzz.proto b/test/common/http/conn_manager_impl_fuzz.proto index a6a3617d0165e..5cc690eb838aa 100644 --- a/test/common/http/conn_manager_impl_fuzz.proto +++ b/test/common/http/conn_manager_impl_fuzz.proto @@ -11,12 +11,15 @@ import "test/fuzz/common.proto"; message NewStream { test.fuzz.Headers request_headers = 1; bool end_stream = 2; - // TODO(htuch): Support stop/continue status with headers. + HeaderStatus status = 3; } enum HeaderStatus { HEADER_CONTINUE = 0; HEADER_STOP_ITERATION = 1; + HEADER_CONTINUE_AND_END_STREAM = 2; + HEADER_STOP_ALL_ITERATION_AND_BUFFER = 3; + HEADER_STOP_ALL_ITERATION_AND_WATERMARK = 4; } enum DataStatus { diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index 4d0512ff7dad6..377bf2843149c 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -217,11 +217,14 @@ class FuzzStream { enum class StreamState { PendingHeaders, PendingDataOrTrailers, Closed }; FuzzStream(ConnectionManagerImpl& conn_manager, FuzzConfig& config, - const HeaderMap& request_headers, bool end_stream) + const HeaderMap& request_headers, + test::common::http::HeaderStatus decode_header_status, bool end_stream) : conn_manager_(conn_manager), config_(config) { config_.newStream(); request_state_ = end_stream ? StreamState::Closed : StreamState::PendingDataOrTrailers; response_state_ = StreamState::PendingHeaders; + decoder_filter_ = config.decoder_filter_; + encoder_filter_ = config.encoder_filter_; EXPECT_CALL(*config_.codec_, dispatch(_)) .WillOnce(InvokeWithoutArgs([this, &request_headers, end_stream] { decoder_ = &conn_manager_.newStream(encoder_); @@ -244,9 +247,15 @@ class FuzzStream { })); decoder_->decodeHeaders(std::move(headers), end_stream); })); + ON_CALL(*decoder_filter_, decodeHeaders(_, _)) + .WillByDefault( + InvokeWithoutArgs([this, decode_header_status]() -> Http::FilterHeadersStatus { + header_status_ = fromHeaderStatus(decode_header_status); + return *header_status_; + return Http::FilterHeadersStatus:: + Continue; //;fromTrailerStatus(trailers_action.status()); + })); fakeOnData(); - decoder_filter_ = config.decoder_filter_; - encoder_filter_ = config.encoder_filter_; FUZZ_ASSERT(testing::Mock::VerifyAndClearExpectations(config_.codec_)); } @@ -261,6 +270,12 @@ class FuzzStream { return Http::FilterHeadersStatus::Continue; case test::common::http::HeaderStatus::HEADER_STOP_ITERATION: return Http::FilterHeadersStatus::StopIteration; + case test::common::http::HeaderStatus::HEADER_CONTINUE_AND_END_STREAM: + return Http::FilterHeadersStatus::ContinueAndEndStream; + case test::common::http::HeaderStatus::HEADER_STOP_ALL_ITERATION_AND_BUFFER: + return Http::FilterHeadersStatus::StopAllIterationAndBuffer; + case test::common::http::HeaderStatus::HEADER_STOP_ALL_ITERATION_AND_WATERMARK: + return Http::FilterHeadersStatus::StopAllIterationAndWatermark; default: return Http::FilterHeadersStatus::Continue; } @@ -320,7 +335,8 @@ class FuzzStream { if (data_action.has_decoder_filter_callback_action()) { decoderFilterCallbackAction(data_action.decoder_filter_callback_action()); } - return fromDataStatus(data_action.status()); + data_status_ = fromDataStatus(data_action.status()); + return *data_status_; })); EXPECT_CALL(*config_.codec_, dispatch(_)).WillOnce(InvokeWithoutArgs([this, &data_action] { Buffer::OwnedImpl buf(std::string(data_action.size() % (1024 * 1024), 'a')); @@ -355,7 +371,14 @@ class FuzzStream { break; } case test::common::http::RequestAction::kContinueDecoding: { - decoder_filter_->callbacks_->continueDecoding(); + if (header_status_ == FilterHeadersStatus::StopAllIterationAndBuffer || + header_status_ == FilterHeadersStatus::StopAllIterationAndWatermark || + (header_status_ == FilterHeadersStatus::StopIteration && + (data_status_ == FilterDataStatus::StopIterationAndBuffer || + data_status_ == FilterDataStatus::StopIterationAndWatermark || + data_status_ == FilterDataStatus::StopIterationNoBuffer))) { + decoder_filter_->callbacks_->continueDecoding(); + } break; } case test::common::http::RequestAction::kThrowDecoderException: { @@ -450,6 +473,8 @@ class FuzzStream { MockStreamEncoderFilter* encoder_filter_{}; StreamState request_state_; StreamState response_state_; + absl::optional header_status_; + absl::optional data_status_; }; using FuzzStreamPtr = std::unique_ptr; @@ -501,7 +526,7 @@ DEFINE_PROTO_FUZZER(const test::common::http::ConnManagerImplTestCase& input) { streams.emplace_back(new FuzzStream( conn_manager, config, Fuzz::fromHeaders(action.new_stream().request_headers()), - action.new_stream().end_stream())); + action.new_stream().status(), action.new_stream().end_stream())); break; } case test::common::http::Action::kStreamAction: { From ff0286d64a8d8a1217db362fcd78d8abbeaf02d7 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Fri, 24 Apr 2020 16:56:24 -0400 Subject: [PATCH 2/2] Remove snafu. Signed-off-by: Harvey Tuch --- test/common/http/conn_manager_impl_fuzz_test.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index 377bf2843149c..43db08612d9d2 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -252,8 +252,6 @@ class FuzzStream { InvokeWithoutArgs([this, decode_header_status]() -> Http::FilterHeadersStatus { header_status_ = fromHeaderStatus(decode_header_status); return *header_status_; - return Http::FilterHeadersStatus:: - Continue; //;fromTrailerStatus(trailers_action.status()); })); fakeOnData(); FUZZ_ASSERT(testing::Mock::VerifyAndClearExpectations(config_.codec_));