From 14495e4de37388947743d91b1c6c1d3059975a58 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Fri, 3 Apr 2020 09:51:02 -0400 Subject: [PATCH 1/3] fuzz: handle stream resets in codec_impl_fuzz_test with HTTP/1. The HTTP/1 codec implies a connection close following a stream reset of any kind, including explicit resetStream(). This patch ensures that codec_impl_fuzz_test completes without any further connection use following a stream reset. Fixes oss-fuzz issue https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=21272. Risk level: Low Testing: Corpus entry added. Signed-off-by: Harvey Tuch --- source/common/http/http1/codec_impl.h | 2 + ...ized-codec_impl_fuzz_test-5635865126895616 | 184 ++++++++++++++++++ test/common/http/codec_impl_fuzz_test.cc | 27 ++- 3 files changed, 209 insertions(+), 4 deletions(-) create mode 100644 test/common/http/codec_impl_corpus/clusterfuzz-testcase-minimized-codec_impl_fuzz_test-5635865126895616 diff --git a/source/common/http/http1/codec_impl.h b/source/common/http/http1/codec_impl.h index 60f15e10df10c..01d1f4c001e21 100644 --- a/source/common/http/http1/codec_impl.h +++ b/source/common/http/http1/codec_impl.h @@ -60,6 +60,8 @@ class StreamEncoderImpl : public virtual StreamEncoder, // Http::Stream void addCallbacks(StreamCallbacks& callbacks) override { addCallbacks_(callbacks); } void removeCallbacks(StreamCallbacks& callbacks) override { removeCallbacks_(callbacks); } + // After this is called, for the HTTP/1 codec, the connection should be closed, i.e. no further + // progress may be made with the codec. void resetStream(StreamResetReason reason) override; void readDisable(bool disable) override; uint32_t bufferLimit() override; diff --git a/test/common/http/codec_impl_corpus/clusterfuzz-testcase-minimized-codec_impl_fuzz_test-5635865126895616 b/test/common/http/codec_impl_corpus/clusterfuzz-testcase-minimized-codec_impl_fuzz_test-5635865126895616 new file mode 100644 index 0000000000000..10ae307bc7ef9 --- /dev/null +++ b/test/common/http/codec_impl_corpus/clusterfuzz-testcase-minimized-codec_impl_fuzz_test-5635865126895616 @@ -0,0 +1,184 @@ +actions { + quiesce_drain { + } +} +actions { + stream_action { + stream_id: 2097152 + response { + continue_headers { + } + } + } +} +actions { + new_stream { + request_headers { + headers { + key: "GET" + value: "/ddddddddddddd" + } + headers { + key: ":path" + value: "/ddddddddddddd" + } + headers { + key: ":method" + value: "GET" + } + headers { + key: "0" + value: "GET" + } + headers { + key: "GET" + } + headers { + key: "GET" + } + } + } +} +actions { + new_stream { + request_headers { + headers { + key: "connection" + value: ",,,,,,,,[,(,5,,,,,,up,,,,upg1ade" + } + } + } +} +actions { + stream_action { + response { + continue_headers { + headers { + key: "connection" + value: ",,,,,,,,[,(,5,,,,,,up,,,,upg1ade" + } + } + } + } +} +actions { + stream_action { + response { + continue_headers { + headers { + key: "connection" + value: ",,,,,,,,[,(,5,,,,,,up,,,,upg1ade" + } + } + } + } +} +actions { + stream_action { + response { + continue_headers { + headers { + key: "connection" + value: ",,,,,,,,[,(,5,,,,,,up,,,,upg1ade" + } + } + } + } +} +actions { + stream_action { + response { + data: 64512 + } + } +} +actions { + stream_action { + response { + continue_headers { + headers { + key: "connection" + value: ",,,,,,,,[,(,5,,,,,,up,,,,upg1ade" + } + } + end_stream: true + } + } +} +actions { + client_drain { + } +} +actions { + client_drain { + } +} +actions { + stream_action { + response { + continue_headers { + headers { + key: "connection" + value: ",,,,,,,,[,(,5,,,,,,up,,,,upg1ade" + } + } + } + } +} +actions { + stream_action { + response { + continue_headers { + headers { + key: "connection" + value: ",,,,,,,,[,(,5,,,,,,up,,,,upg1ade" + } + } + end_stream: true + } + } +} +actions { + stream_action { + stream_id: 1024 + response { + continue_headers { + headers { + key: "connection" + value: ",,,,,,,,[,(,5,,,,,,up,,,,upg1ade" + } + } + } + } +} +actions { + stream_action { + response { + continue_headers { + headers { + key: "connection" + value: ",,,,,,,,[,(,5,,,,pg1ade" + } + } + } + } +} +actions { + stream_action { + request { + reset_stream: 2097152 + } + } +} +actions { + stream_action { + response { + continue_headers { + headers { + key: "connection" + value: ",,,,,,X,,[,(,5.,,,,,up,,,,upgeta1ade" + } + } + } + } +} diff --git a/test/common/http/codec_impl_fuzz_test.cc b/test/common/http/codec_impl_fuzz_test.cc index 98daa5c5cb3ad..bc66bfed7d68c 100644 --- a/test/common/http/codec_impl_fuzz_test.cc +++ b/test/common/http/codec_impl_fuzz_test.cc @@ -80,6 +80,8 @@ fromHttp2Settings(const test::common::http::Http2Settings& settings) { return options; } +using StreamResetCallbackFn = std::function; + // Internal representation of stream state. Encapsulates the stream state, mocks // and encoders for both the request/response. class HttpStream : public LinkedObject { @@ -122,12 +124,14 @@ class HttpStream : public LinkedObject { } request_, response_; HttpStream(ClientConnection& client, const TestRequestHeaderMapImpl& request_headers, - bool end_stream) { + bool end_stream, StreamResetCallbackFn stream_reset_callback) + : stream_reset_callback_(stream_reset_callback) { request_.request_encoder_ = &client.newStream(response_.response_decoder_); ON_CALL(request_.stream_callbacks_, onResetStream(_, _)) .WillByDefault(InvokeWithoutArgs([this] { ENVOY_LOG_MISC(trace, "reset request for stream index {}", stream_index_); resetStream(); + stream_reset_callback_(); })); ON_CALL(response_.stream_callbacks_, onResetStream(_, _)) .WillByDefault(InvokeWithoutArgs([this] { @@ -138,6 +142,7 @@ class HttpStream : public LinkedObject { // HTTP/1 codec. request_.request_encoder_->getStream().resetStream(StreamResetReason::LocalReset); resetStream(); + stream_reset_callback_(); })); ON_CALL(request_.request_decoder_, decodeHeaders_(_, true)) .WillByDefault(InvokeWithoutArgs([this] { @@ -325,6 +330,7 @@ class HttpStream : public LinkedObject { } int32_t stream_index_{-1}; + StreamResetCallbackFn stream_reset_callback_; }; // Buffer between client and server H1/H2 codecs. This models each write operation @@ -470,9 +476,14 @@ void codecFuzz(const test::common::http::CodecImplFuzzTestCase& input, HttpVersi } }; + // We track whether the connection should be closed for HTTP/1, since stream resets imply + // connection closes. + bool should_close_connection = false; + constexpr auto max_actions = 1024; try { - for (int i = 0; i < std::min(max_actions, input.actions().size()); ++i) { + for (int i = 0; i < std::min(max_actions, input.actions().size()) && !should_close_connection; + ++i) { const auto& action = input.actions(i); ENVOY_LOG_MISC(trace, "action {} with {} streams", action.DebugString(), streams.size()); switch (action.action_selector_case()) { @@ -490,7 +501,12 @@ void codecFuzz(const test::common::http::CodecImplFuzzTestCase& input, HttpVersi HttpStreamPtr stream = std::make_unique( *client, fromSanitizedHeaders(action.new_stream().request_headers()), - action.new_stream().end_stream()); + action.new_stream().end_stream(), [&should_close_connection, http2]() { + // HTTP/1 codec has stream reset implying connection close. + if (!http2) { + should_close_connection = true; + } + }); stream->moveIntoListBack(std::move(stream), pending_streams); break; } @@ -537,7 +553,10 @@ void codecFuzz(const test::common::http::CodecImplFuzzTestCase& input, HttpVersi client_server_buf_drain(); } } - client_server_buf_drain(); + // Drain all remaining buffers, unless the connection is effectively closed. + if (!should_close_connection) { + client_server_buf_drain(); + } if (http2) { dynamic_cast(*client).goAway(); dynamic_cast(*server).goAway(); From a53b3353bf23d876a84d06551ad7ae93f6ca5c18 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Mon, 6 Apr 2020 10:20:18 -0400 Subject: [PATCH 2/3] Debug mode support for close connection detection. Signed-off-by: Harvey Tuch --- test/common/http/codec_impl_fuzz_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/http/codec_impl_fuzz_test.cc b/test/common/http/codec_impl_fuzz_test.cc index bc66bfed7d68c..65409fcd301bf 100644 --- a/test/common/http/codec_impl_fuzz_test.cc +++ b/test/common/http/codec_impl_fuzz_test.cc @@ -549,7 +549,7 @@ void codecFuzz(const test::common::http::CodecImplFuzzTestCase& input, HttpVersi // Maybe nothing is set? break; } - if (DebugMode) { + if (DebugMode && !should_close_connection) { client_server_buf_drain(); } } From 1895ec4584f1447bf47c47592100e9bc41fee79c Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Mon, 6 Apr 2020 15:13:22 -0400 Subject: [PATCH 3/3] Kick CI Signed-off-by: Harvey Tuch