From 9e86d0254592a9880806fc2d3b7b3b7faac8470a Mon Sep 17 00:00:00 2001 From: "Adi (Suissa) Peleg" Date: Fri, 19 Jun 2020 14:51:56 -0400 Subject: [PATCH] Fixing HCM fuzzer ContinueAndEndStream with end_stream set (#11497) Fixing the HCM fuzzer to avoid cases where its decodeHeaders method returns FilterHeadersStatus::ContinueAndEndStream when called with end_stream=true. This assert was added in #4885, to avoid misuse of the FilterHeadersStatus::ContinueAndEndStream value. Risk Level: Low - tests only Testing: Changed HCM fuzzer test code Fixes oss fuzz issue 21971 Signed-off-by: Adi Suissa-Peleg --- docs/root/faq/extensions/contract.rst | 48 +++++++++++++++++++ docs/root/faq/overview.rst | 8 ++++ include/envoy/http/filter.h | 1 + ...zz-testcase-continueandendstream-endstream | 16 +++++++ .../http/conn_manager_impl_fuzz_test.cc | 15 ++++-- 5 files changed, 83 insertions(+), 5 deletions(-) create mode 100644 docs/root/faq/extensions/contract.rst create mode 100644 test/common/http/conn_manager_impl_corpus/clusterfuzz-testcase-continueandendstream-endstream diff --git a/docs/root/faq/extensions/contract.rst b/docs/root/faq/extensions/contract.rst new file mode 100644 index 000000000000..35e9a05f06ba --- /dev/null +++ b/docs/root/faq/extensions/contract.rst @@ -0,0 +1,48 @@ +.. _faq_filter_contract: + +Is there a contract my HTTP filter must adhere to? +-------------------------------------------------- + +* Headers encoding/decoding + + * During encoding/decoding of headers if a filter returns ``FilterHeadersStatus::StopIteration``, + the processing can be resumed if ``encodeData()``/``decodeData()`` return + ``FilterDataStatus::Continue`` or by explicitly calling + ``continueEncoding()``/``continueDecoding()``. + + * During encoding/decoding of headers if a filter returns + ``FilterHeadersStatus::StopAllIterationAndBuffer`` or + ``FilterHeadersStatus::StopAllIterationAndWatermark``, the processing can be resumed by calling + ``continueEncoding()``/``continueDecoding()``. + + * A filter's ``decodeHeaders()`` implementation must not return + ``FilterHeadersStatus::ContinueAndEndStream`` when called with ``end_stream`` set to *true*. In this case + ``FilterHeadersStatus::Continue`` should be returned. + + * A filter's ``encode100ContinueHeaders()`` must return ``FilterHeadersStatus::Continue`` or + ``FilterHeadersStatus::StopIteration``. + +* Data encoding/decoding + + * During encoding/decoding of data if a filter returns + ``FilterDataStatus::StopIterationAndBuffer``, ``FilterDataStatus::StopIterationAndWatermark``, + or ``FilterDataStatus::StopIterationNoBuffer``, the processing can be resumed if + ``encodeData()``/``decodeData()`` return ``FilterDataStatus::Continue`` or by explicitly + calling ``continueEncoding()``/``continueDecoding()``. + +* Trailers encoding/decoding + + * During encoding/decoding of trailers if a filter returns ``FilterTrailersStatus::StopIteration``, + the processing can be resumed by explicitly calling ``continueEncoding()``/``continueDecoding()``. + +Are there well-known headers that will appear in the given headers map of ``decodeHeaders()``? +---------------------------------------------------------------------------------------------- + +The first filter of the decoding filter chain will have the following headers in the map: + +* ``Host`` +* ``Path`` (this might be omitted for CONNECT requests). + +Although these headers may be omitted by one of the filters on the decoding filter chain, +they should be reinserted before the terminal filter is triggered. + diff --git a/docs/root/faq/overview.rst b/docs/root/faq/overview.rst index 47156f7cd70e..3a65808a8b8b 100644 --- a/docs/root/faq/overview.rst +++ b/docs/root/faq/overview.rst @@ -72,3 +72,11 @@ Load balancing load_balancing/disable_circuit_breaking load_balancing/transient_failures load_balancing/region_failover + +Extensions +---------- + +.. toctree:: + :maxdepth: 2 + + extensions/contract diff --git a/include/envoy/http/filter.h b/include/envoy/http/filter.h index 0d6e243140a6..ee3133b3b1ee 100644 --- a/include/envoy/http/filter.h +++ b/include/envoy/http/filter.h @@ -34,6 +34,7 @@ enum class FilterHeadersStatus { StopIteration, // Continue iteration to remaining filters, but ignore any subsequent data or trailers. This // results in creating a header only request/response. + // This status MUST NOT be returned by decodeHeaders() when end_stream is set to true. ContinueAndEndStream, // Do not iterate for headers as well as data and trailers for the current filter and the filters // following, and buffer body data for later dispatching. ContinueDecoding() MUST diff --git a/test/common/http/conn_manager_impl_corpus/clusterfuzz-testcase-continueandendstream-endstream b/test/common/http/conn_manager_impl_corpus/clusterfuzz-testcase-continueandendstream-endstream new file mode 100644 index 000000000000..2b64d93f4355 --- /dev/null +++ b/test/common/http/conn_manager_impl_corpus/clusterfuzz-testcase-continueandendstream-endstream @@ -0,0 +1,16 @@ +actions { + new_stream { + request_headers { + headers { + key: ":path" + value: "/" + } + headers { + key: ":authority" + value: "foo.com" + } + } + end_stream: true + status: HEADER_CONTINUE_AND_END_STREAM + } +} diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index e151b1656fd6..196b8bdc24d2 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -284,11 +284,16 @@ class FuzzStream { return Http::okStatus(); })); ON_CALL(*decoder_filter_, decodeHeaders(_, _)) - .WillByDefault( - InvokeWithoutArgs([this, decode_header_status]() -> Http::FilterHeadersStatus { - header_status_ = fromHeaderStatus(decode_header_status); - return *header_status_; - })); + .WillByDefault(InvokeWithoutArgs([this, decode_header_status, + end_stream]() -> Http::FilterHeadersStatus { + header_status_ = fromHeaderStatus(decode_header_status); + // When a filter should not return ContinueAndEndStream when send with end_stream set + // (see https://github.com/envoyproxy/envoy/pull/4885#discussion_r232176826) + if (end_stream && (*header_status_ == Http::FilterHeadersStatus::ContinueAndEndStream)) { + *header_status_ = Http::FilterHeadersStatus::Continue; + } + return *header_status_; + })); fakeOnData(); FUZZ_ASSERT(testing::Mock::VerifyAndClearExpectations(config_.codec_)); }