From b155af75fad7861e941b5939dc001abf581c9203 Mon Sep 17 00:00:00 2001 From: htuch Date: Wed, 27 Mar 2019 10:35:14 -0400 Subject: [PATCH] codec: reject embedded NUL in headers. (#2) http-parser doesn't sanitize header values as per RFC 7230 today, so add an additional check (yielding a CodecProtocolException) for this. See CVE-2019-9900 for further details. Added CR/LF, in addition to NUL, as prohibited in header strings as per RFC 7230. Also added codec_impl_tests for H1 and H2 codecs to validate that NUL/LF/CR mutations in a request don't violate internal invariants. Fixes CVE-2019-9900 and oss-fuzz issue https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=13613. Risk level: Low Testing: Corpus entry and unit test added. Signed-off-by: Harvey Tuch --- docs/root/intro/version_history.rst | 4 + include/envoy/http/header_map.h | 17 ++- source/common/http/header_map_impl.cc | 4 +- source/common/http/http1/codec_impl.cc | 7 ++ test/common/http/http1/codec_impl_test.cc | 61 ++++++++++ test/common/http/http2/codec_impl_test.cc | 110 +++++++++++++++--- .../filters/http/gzip/gzip_filter_test.cc | 6 +- test/integration/h1_corpus/embed_null.pb_text | 1 + tools/spelling_dictionary.txt | 1 + 9 files changed, 182 insertions(+), 29 deletions(-) create mode 100644 test/integration/h1_corpus/embed_null.pb_text diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index ac2168dba4ac..6880dd4d1fa6 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -85,6 +85,10 @@ Version history * upstream: added configuration option to select any host when the fallback policy fails. * upstream: stopped incrementing upstream_rq_total for HTTP/1 conn pool when request is circuit broken. +1.9.1 (Apr 2, 2019) +=================== +* http: fixed CVE-2019-9900 by rejecting HTTP/1.x headers with embedded NUL characters. + 1.9.0 (Dec 20, 2018) ==================== * access log: added a :ref:`JSON logging mode ` to output access logs in JSON format. diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index f67a55c60796..b68967ed8492 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -20,6 +20,17 @@ namespace Envoy { namespace Http { +// Used by ASSERTs to validate internal consistency. E.g. valid HTTP header keys/values should +// never contain embedded NULLs. +static inline bool validHeaderString(absl::string_view s) { + for (const char c : {'\0', '\r', '\n'}) { + if (s.find(c) != absl::string_view::npos) { + return false; + } + } + return true; +} + /** * Wrapper for a lower case string used in header operations to generally avoid needless case * insensitive compares. @@ -40,9 +51,7 @@ class LowerCaseString { private: void lower() { std::transform(string_.begin(), string_.end(), string_.begin(), tolower); } - // Used by ASSERTs to validate internal consistency. E.g. valid HTTP header keys/values should - // never contain embedded NULLs. - bool valid() const { return string_.find('\0') == std::string::npos; } + bool valid() const { return validHeaderString(string_); } std::string string_; }; @@ -183,8 +192,6 @@ class HeaderString { }; void freeDynamic(); - // Used by ASSERTs to validate internal consistency. E.g. valid HTTP header keys/values should - // never contain embedded NULLs. bool valid() const; uint32_t string_length_; diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index 68bfc4365d53..56185e9058ec 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -90,9 +90,7 @@ void HeaderString::freeDynamic() { } } -bool HeaderString::valid() const { - return std::string(c_str(), string_length_).find('\0') == std::string::npos; -} +bool HeaderString::valid() const { return validHeaderString(getStringView()); } void HeaderString::append(const char* data, uint32_t size) { switch (type_) { diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index d85e64244478..9adef3d928c3 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -418,6 +418,13 @@ void ConnectionImpl::onHeaderValue(const char* data, size_t length) { // Ignore trailers. return; } + // http-parser should filter for this + // (https://tools.ietf.org/html/rfc7230#section-3.2.6), but it doesn't today. HeaderStrings + // have an invariant that they must not contain embedded zero characters + // (NUL, ASCII 0x0). + if (absl::string_view(data, length).find('\0') != absl::string_view::npos) { + throw CodecProtocolException("http/1.1 protocol error: header value contains NUL"); + } header_parsing_state_ = HeaderParsingState::Value; current_header_value_.append(data, length); diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index b6639dfd4237..79bea04d1c0f 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -289,6 +289,67 @@ TEST_F(Http1ServerConnectionImplTest, HostHeaderTranslation) { EXPECT_EQ(0U, buffer.length()); } +// Regression test for http-parser allowing embedded NULs in header values, +// verify we reject them. +TEST_F(Http1ServerConnectionImplTest, HeaderEmbeddedNulRejection) { + initialize(); + + InSequence sequence; + + Http::MockStreamDecoder decoder; + EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); + + Buffer::OwnedImpl buffer( + absl::StrCat("GET / HTTP/1.1\r\nHOST: h.com\r\nfoo: bar", std::string(1, '\0'), "baz\r\n")); + EXPECT_THROW_WITH_MESSAGE(codec_->dispatch(buffer), CodecProtocolException, + "http/1.1 protocol error: header value contains NUL"); +} + +// Mutate an HTTP GET with embedded NULs, this should always be rejected in some +// way (not necessarily with "head value contains NUL" though). +TEST_F(Http1ServerConnectionImplTest, HeaderMutateEmbeddedNul) { + const std::string example_input = "GET / HTTP/1.1\r\nHOST: h.com\r\nfoo: barbaz\r\n"; + + for (size_t n = 1; n < example_input.size(); ++n) { + initialize(); + + InSequence sequence; + + Http::MockStreamDecoder decoder; + EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); + + Buffer::OwnedImpl buffer( + absl::StrCat(example_input.substr(0, n), std::string(1, '\0'), example_input.substr(n))); + EXPECT_THROW_WITH_REGEX(codec_->dispatch(buffer), CodecProtocolException, + "http/1.1 protocol error:"); + } +} + +// Mutate an HTTP GET with CR or LF. These can cause an exception or maybe +// result in a valid decodeHeaders(). In any case, the validHeaderString() +// ASSERTs should validate we never have any embedded CR or LF. +TEST_F(Http1ServerConnectionImplTest, HeaderMutateEmbeddedCRLF) { + const std::string example_input = "GET / HTTP/1.1\r\nHOST: h.com\r\nfoo: barbaz\r\n"; + + for (const char c : {'\r', '\n'}) { + for (size_t n = 1; n < example_input.size(); ++n) { + initialize(); + + InSequence sequence; + + NiceMock decoder; + EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); + + Buffer::OwnedImpl buffer( + absl::StrCat(example_input.substr(0, n), std::string(1, c), example_input.substr(n))); + try { + codec_->dispatch(buffer); + } catch (CodecProtocolException) { + } + } + } +} + TEST_F(Http1ServerConnectionImplTest, CloseDuringHeadersComplete) { initialize(); diff --git a/test/common/http/http2/codec_impl_test.cc b/test/common/http/http2/codec_impl_test.cc index 759a39ddb203..a220413d3496 100644 --- a/test/common/http/http2/codec_impl_test.cc +++ b/test/common/http/http2/codec_impl_test.cc @@ -34,7 +34,13 @@ namespace Http2 { using Http2SettingsTuple = ::testing::tuple; using Http2SettingsTestParam = ::testing::tuple; -class Http2CodecImplTest : public testing::TestWithParam { +constexpr Http2SettingsTuple + DefaultHttp2SettingsTuple(Http2Settings::DEFAULT_HPACK_TABLE_SIZE, + Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, + Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS, + Http2Settings::DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE); + +class Http2CodecImplTestFixture { public: struct ConnectionWrapper { void dispatch(const Buffer::Instance& data, ConnectionImpl& connection) { @@ -52,9 +58,13 @@ class Http2CodecImplTest : public testing::TestWithParam Buffer::OwnedImpl buffer_; }; - void initialize() { - Http2SettingsFromTuple(client_http2settings_, ::testing::get<0>(GetParam())); - Http2SettingsFromTuple(server_http2settings_, ::testing::get<1>(GetParam())); + Http2CodecImplTestFixture(Http2SettingsTuple client_settings, Http2SettingsTuple server_settings) + : client_settings_(client_settings), server_settings_(server_settings) {} + virtual ~Http2CodecImplTestFixture() {} + + virtual void initialize() { + Http2SettingsFromTuple(client_http2settings_, client_settings_); + Http2SettingsFromTuple(server_http2settings_, server_settings_); client_ = std::make_unique(client_connection_, client_callbacks_, stats_store_, client_http2settings_, max_request_headers_kb_); @@ -76,8 +86,11 @@ class Http2CodecImplTest : public testing::TestWithParam void setupDefaultConnectionMocks() { ON_CALL(client_connection_, write(_, _)) .WillByDefault(Invoke([&](Buffer::Instance& data, bool) -> void { - if (corrupt_data_) { - corruptFramePayload(data); + if (corrupt_metadata_frame_) { + corruptMetadataFramePayload(data); + } + if (corrupt_at_offset_ >= 0) { + corruptAtOffset(data, corrupt_at_offset_, corrupt_with_char_); } server_wrapper_.dispatch(data, *server_); })); @@ -95,22 +108,27 @@ class Http2CodecImplTest : public testing::TestWithParam setting.allow_metadata_ = allow_metadata_; } - // corruptFramePayload assumes data contains at least 10 bytes of the beginning of a frame. - void corruptFramePayload(Buffer::Instance& data) { + // corruptMetadataFramePayload assumes data contains at least 10 bytes of the beginning of a + // frame. + void corruptMetadataFramePayload(Buffer::Instance& data) { const size_t length = data.length(); const size_t corrupt_start = 10; if (length < corrupt_start || length > METADATA_MAX_PAYLOAD_SIZE) { ENVOY_LOG_MISC(error, "data size too big or too small"); return; } - uint8_t buf[METADATA_MAX_PAYLOAD_SIZE] = {0}; - data.copyOut(0, length, static_cast(buf)); - data.drain(length); - // Keeps the frame header (9 bytes) valid, and corrupts the payload. - buf[10] |= 0xff; - data.add(buf, length); + corruptAtOffset(data, corrupt_start, 0xff); + } + + void corruptAtOffset(Buffer::Instance& data, size_t index, char new_value) { + if (data.length() == 0) { + return; + } + reinterpret_cast(data.linearize(data.length()))[index % data.length()] = new_value; } + const Http2SettingsTuple client_settings_; + const Http2SettingsTuple server_settings_; bool allow_metadata_ = false; Stats::IsolatedStoreImpl stats_store_; Http2Settings client_http2settings_; @@ -128,10 +146,22 @@ class Http2CodecImplTest : public testing::TestWithParam MockStreamDecoder request_decoder_; StreamEncoder* response_encoder_{}; MockStreamCallbacks server_stream_callbacks_; - bool corrupt_data_ = false; + // Corrupt a metadata frame payload. + bool corrupt_metadata_frame_ = false; + // Corrupt frame at a given offset (if positive). + ssize_t corrupt_at_offset_{-1}; + char corrupt_with_char_{'\0'}; + uint32_t max_request_headers_kb_ = Http::DEFAULT_MAX_REQUEST_HEADERS_KB; }; +class Http2CodecImplTest : public ::testing::TestWithParam, + protected Http2CodecImplTestFixture { +public: + Http2CodecImplTest() + : Http2CodecImplTestFixture(::testing::get<0>(GetParam()), ::testing::get<1>(GetParam())) {} +}; + TEST_P(Http2CodecImplTest, ShutdownNotice) { initialize(); @@ -191,7 +221,7 @@ TEST_P(Http2CodecImplTest, InvalidContinueWithFin) { client_wrapper_.dispatch(Buffer::OwnedImpl(), *client_); EXPECT_EQ(1, stats_store_.counter("http2.rx_messaging_error").value()); -}; +} TEST_P(Http2CodecImplTest, InvalidRepeatContinue) { initialize(); @@ -242,7 +272,7 @@ TEST_P(Http2CodecImplTest, Invalid103) { EXPECT_THROW_WITH_MESSAGE(response_encoder_->encodeHeaders(early_hint_headers, false), CodecProtocolException, "Unexpected 'trailers' with no end stream."); EXPECT_EQ(1, stats_store_.counter("http2.too_many_header_frames").value()); -}; +} TEST_P(Http2CodecImplTest, Invalid204WithContentLength) { initialize(); @@ -444,7 +474,7 @@ TEST_P(Http2CodecImplTest, BadMetadataVecReceivedTest) { MetadataMapVector metadata_map_vector; metadata_map_vector.push_back(std::move(metadata_map_ptr)); - corrupt_data_ = true; + corrupt_metadata_frame_ = true; EXPECT_THROW_WITH_MESSAGE(request_encoder_->encodeMetadata(metadata_map_vector), EnvoyException, "The user callback function failed"); } @@ -1011,6 +1041,50 @@ TEST_P(Http2CodecImplTest, TestCodecHeaderCompression) { } } +// Validate that nghttp2 rejects NUL/CR/LF as per +// https://httpwg.org/specs/rfc7540.html#rfc.section.10.3. +// TEST_P(Http2CodecImplTest, InvalidHeaderChars) { +// TODO(htuch): Write me. Http2CodecImplMutationTest basically covers this, +// but we could be a bit more specific and add a captured H2 HEADERS frame +// here and inject it with mutation of just the header value, ensuring we get +// the expected codec exception. +// } + +class Http2CodecImplMutationTest : public ::testing::TestWithParam<::testing::tuple>, + protected Http2CodecImplTestFixture { +public: + Http2CodecImplMutationTest() + : Http2CodecImplTestFixture(DefaultHttp2SettingsTuple, DefaultHttp2SettingsTuple) {} + + void initialize() override { + corrupt_with_char_ = ::testing::get<0>(GetParam()); + corrupt_at_offset_ = ::testing::get<1>(GetParam()); + Http2CodecImplTestFixture::initialize(); + } +}; + +INSTANTIATE_TEST_SUITE_P(Http2CodecImplMutationTest, Http2CodecImplMutationTest, + ::testing::Combine(::testing::ValuesIn({'\0', '\r', '\n'}), + ::testing::Range(0, 128))); + +// Mutate an arbitrary offset in the HEADERS frame with NUL/CR/LF. This should +// either throw an exception or continue, but we shouldn't crash due to +// validHeaderString() ASSERTs. +TEST_P(Http2CodecImplMutationTest, HandleInvalidChars) { + initialize(); + + TestHeaderMapImpl request_headers; + request_headers.addCopy("foo", "barbaz"); + HttpTestUtility::addDefaultHeaders(request_headers); + EXPECT_CALL(request_decoder_, decodeHeaders_(_, _)).Times(AnyNumber()); + EXPECT_CALL(client_callbacks_, onGoAway()).Times(AnyNumber()); + try { + request_encoder_->encodeHeaders(request_headers, true); + } catch (const CodecProtocolException& e) { + ENVOY_LOG_MISC(trace, "CodecProtocolException: {}", e.what()); + } +} + } // namespace Http2 } // namespace Http } // namespace Envoy diff --git a/test/extensions/filters/http/gzip/gzip_filter_test.cc b/test/extensions/filters/http/gzip/gzip_filter_test.cc index 685b21a5e688..80d9b0df9523 100644 --- a/test/extensions/filters/http/gzip/gzip_filter_test.cc +++ b/test/extensions/filters/http/gzip/gzip_filter_test.cc @@ -208,7 +208,7 @@ TEST_F(GzipFilterTest, isAcceptEncodingAllowed) { } { Http::TestHeaderMapImpl headers = { - {"accept-encoding", "\tdeflate\t, gzip\t ; q\t =\t 1.0,\t * ;q=0.5\n"}}; + {"accept-encoding", "\tdeflate\t, gzip\t ; q\t =\t 1.0,\t * ;q=0.5"}}; EXPECT_TRUE(isAcceptEncodingAllowed(headers)); EXPECT_EQ(3, stats_.counter("test.gzip.header_gzip").value()); } @@ -416,7 +416,7 @@ TEST_F(GzipFilterTest, isContentTypeAllowed) { EXPECT_TRUE(isContentTypeAllowed(headers)); } { - Http::TestHeaderMapImpl headers = {{"content-type", "\ttext/html\t\n"}}; + Http::TestHeaderMapImpl headers = {{"content-type", "\ttext/html\t"}}; EXPECT_TRUE(isContentTypeAllowed(headers)); } @@ -588,7 +588,7 @@ TEST_F(GzipFilterTest, isTransferEncodingAllowed) { EXPECT_FALSE(isTransferEncodingAllowed(headers)); } { - Http::TestHeaderMapImpl headers = {{"transfer-encoding", " gzip\t, chunked\t\n"}}; + Http::TestHeaderMapImpl headers = {{"transfer-encoding", " gzip\t, chunked\t"}}; EXPECT_FALSE(isTransferEncodingAllowed(headers)); } } diff --git a/test/integration/h1_corpus/embed_null.pb_text b/test/integration/h1_corpus/embed_null.pb_text new file mode 100644 index 000000000000..c15fc60b65da --- /dev/null +++ b/test/integration/h1_corpus/embed_null.pb_text @@ -0,0 +1 @@ +events { downstream_send_bytes: "POST /\nntnt: � \0 " } diff --git a/tools/spelling_dictionary.txt b/tools/spelling_dictionary.txt index b553e2f43c77..01b8c8658d7c 100644 --- a/tools/spelling_dictionary.txt +++ b/tools/spelling_dictionary.txt @@ -137,6 +137,7 @@ LC LDS LEV LHS +LF MB MD MGET