From 84dabbf4f69257b4ca7bbbcd45d8e3ed9629ae98 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Wed, 31 Jul 2019 12:59:14 -0400 Subject: [PATCH] http2: enable strict validation of HTTP/2 headers. (#25) Fixes CVE-2019-9516. Signed-off-by: Piotr Sikora --- api/envoy/api/v2/core/protocol.proto | 9 ++- docs/root/intro/version_history.rst | 1 + include/envoy/http/codec.h | 3 + source/common/http/http2/codec_impl.cc | 17 +++-- source/common/http/http2/codec_impl.h | 3 + source/common/http/utility.cc | 1 + test/common/http/http2/codec_impl_test.cc | 65 ++++++++++++++++++- test/common/http/http2/http2_frame.cc | 21 ++++++ test/common/http/http2/http2_frame.h | 4 ++ test/integration/http2_integration_test.cc | 56 ++++++++++++++++ test/integration/protocol_integration_test.cc | 58 +++++++++++++++++ 11 files changed, 229 insertions(+), 9 deletions(-) diff --git a/api/envoy/api/v2/core/protocol.proto b/api/envoy/api/v2/core/protocol.proto index a5a8ba3327d7..88a82077a428 100644 --- a/api/envoy/api/v2/core/protocol.proto +++ b/api/envoy/api/v2/core/protocol.proto @@ -49,7 +49,7 @@ message Http1ProtocolOptions { string default_host_for_http_10 = 3; } -// [#comment:next free field: 12] +// [#comment:next free field: 13] message Http2ProtocolOptions { // `Maximum table size `_ // (in octets) that the encoder is permitted to use for the dynamic HPACK table. Valid values @@ -142,6 +142,13 @@ message Http2ProtocolOptions { // [#comment:TODO: implement same limits for upstream inbound frames as well.] google.protobuf.UInt32Value max_inbound_window_update_frames_per_data_frame_sent = 11 [(validate.rules).uint32 = {gte: 1}]; + + // Allows invalid HTTP messaging and headers. When this option is disabled (default), then + // the whole HTTP/2 connection is terminated upon receiving invalid HEADERS frame. However, + // when this option is enabled, only the offending stream is terminated. + // + // See [RFC7540, sec. 8.1](https://tools.ietf.org/html/rfc7540#section-8.1) for details. + bool stream_error_on_invalid_http_messaging = 12; } // [#not-implemented-hide:] diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 7896b6a1ab65..18bc47adcac3 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -34,6 +34,7 @@ Version history * http: added :ref:`inbound_window_update_frames_flood ` counter stat to the HTTP/2 codec stats, for tracking number of connections terminated for exceeding the limit on inbound WINDOW_UPDATE frames. The limit is configured by setting the :ref:`max_inbound_window_update_frames_per_data_frame_sent config setting `. * http: added :ref:`outbound_flood ` counter stat to the HTTP/2 codec stats, for tracking number of connections terminated for exceeding the outbound queue limit. The limit is configured by setting the :ref:`max_outbound_frames config setting ` * http: added :ref:`outbound_control_flood ` counter stat to the HTTP/2 codec stats, for tracking number of connections terminated for exceeding the outbound queue limit for PING, SETTINGS and RST_STREAM frames. The limit is configured by setting the :ref:`max_outbound_control_frames config setting `. +* http: enabled strict validation of HTTP/2 messaging. Previous behavior can be restored using :ref:`stream_error_on_invalid_http_messaging config setting `. 1.11.0 (July 11, 2019) ====================== diff --git a/include/envoy/http/codec.h b/include/envoy/http/codec.h index 6c2b09b2f636..eb5951d3b9b6 100644 --- a/include/envoy/http/codec.h +++ b/include/envoy/http/codec.h @@ -235,6 +235,7 @@ struct Http2Settings { uint32_t initial_connection_window_size_{DEFAULT_INITIAL_CONNECTION_WINDOW_SIZE}; bool allow_connect_{DEFAULT_ALLOW_CONNECT}; bool allow_metadata_{DEFAULT_ALLOW_METADATA}; + bool stream_error_on_invalid_http_messaging_{DEFAULT_STREAM_ERROR_ON_INVALID_HTTP_MESSAGING}; uint32_t max_outbound_frames_{DEFAULT_MAX_OUTBOUND_FRAMES}; uint32_t max_outbound_control_frames_{DEFAULT_MAX_OUTBOUND_CONTROL_FRAMES}; uint32_t max_consecutive_inbound_frames_with_empty_payload_{ @@ -279,6 +280,8 @@ struct Http2Settings { static const bool DEFAULT_ALLOW_CONNECT = false; // By default Envoy does not allow METADATA support. static const bool DEFAULT_ALLOW_METADATA = false; + // By default Envoy does not allow invalid headers. + static const bool DEFAULT_STREAM_ERROR_ON_INVALID_HTTP_MESSAGING = false; // Default limit on the number of outbound frames of all types. static const uint32_t DEFAULT_MAX_OUTBOUND_FRAMES = 10000; diff --git a/source/common/http/http2/codec_impl.cc b/source/common/http/http2/codec_impl.cc index d59c16676a75..e45f07a101af 100644 --- a/source/common/http/http2/codec_impl.cc +++ b/source/common/http/http2/codec_impl.cc @@ -579,16 +579,19 @@ int ConnectionImpl::onInvalidFrame(int32_t stream_id, int error_code) { ENVOY_CONN_LOG(debug, "invalid frame: {} on stream {}", connection_, nghttp2_strerror(error_code), stream_id); - // The stream is about to be closed due to an invalid header or messaging. Don't kill the - // entire connection if one stream has bad headers or messaging. if (error_code == NGHTTP2_ERR_HTTP_HEADER || error_code == NGHTTP2_ERR_HTTP_MESSAGING) { stats_.rx_messaging_error_.inc(); - StreamImpl* stream = getStream(stream_id); - if (stream != nullptr) { - // See comment below in onStreamClose() for why we do this. - stream->reset_due_to_messaging_error_ = true; + + if (stream_error_on_invalid_http_messaging_) { + // The stream is about to be closed due to an invalid header or messaging. Don't kill the + // entire connection if one stream has bad headers or messaging. + StreamImpl* stream = getStream(stream_id); + if (stream != nullptr) { + // See comment below in onStreamClose() for why we do this. + stream->reset_due_to_messaging_error_ = true; + } + return 0; } - return 0; } // Cause dispatch to return with an error code. diff --git a/source/common/http/http2/codec_impl.h b/source/common/http/http2/codec_impl.h index 9ca472576cfb..15800f25a523 100644 --- a/source/common/http/http2/codec_impl.h +++ b/source/common/http/http2/codec_impl.h @@ -82,6 +82,8 @@ class ConnectionImpl : public virtual Connection, protected Logger::Loggable(tp); setting.initial_connection_window_size_ = ::testing::get<3>(tp); setting.allow_metadata_ = allow_metadata_; + setting.stream_error_on_invalid_http_messaging_ = stream_error_on_invalid_http_messaging_; setting.max_outbound_frames_ = max_outbound_frames_; setting.max_outbound_control_frames_ = max_outbound_control_frames_; setting.max_consecutive_inbound_frames_with_empty_payload_ = @@ -128,6 +129,7 @@ class Http2CodecImplTestFixture { const Http2SettingsTuple client_settings_; const Http2SettingsTuple server_settings_; bool allow_metadata_ = false; + bool stream_error_on_invalid_http_messaging_ = false; Stats::IsolatedStoreImpl stats_store_; Http2Settings client_http2settings_; NiceMock client_connection_; @@ -202,6 +204,20 @@ TEST_P(Http2CodecImplTest, ContinueHeaders) { TEST_P(Http2CodecImplTest, InvalidContinueWithFin) { initialize(); + TestHeaderMapImpl request_headers; + HttpTestUtility::addDefaultHeaders(request_headers); + EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); + request_encoder_->encodeHeaders(request_headers, true); + + TestHeaderMapImpl continue_headers{{":status", "100"}}; + EXPECT_THROW(response_encoder_->encodeHeaders(continue_headers, true), CodecProtocolException); + EXPECT_EQ(1, stats_store_.counter("http2.rx_messaging_error").value()); +} + +TEST_P(Http2CodecImplTest, InvalidContinueWithFinAllowed) { + stream_error_on_invalid_http_messaging_ = true; + initialize(); + MockStreamCallbacks request_callbacks; request_encoder_->getStream().addCallbacks(request_callbacks); @@ -229,6 +245,23 @@ TEST_P(Http2CodecImplTest, InvalidContinueWithFin) { TEST_P(Http2CodecImplTest, InvalidRepeatContinue) { initialize(); + TestHeaderMapImpl request_headers; + HttpTestUtility::addDefaultHeaders(request_headers); + EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); + request_encoder_->encodeHeaders(request_headers, true); + + TestHeaderMapImpl continue_headers{{":status", "100"}}; + EXPECT_CALL(response_decoder_, decode100ContinueHeaders_(_)); + response_encoder_->encode100ContinueHeaders(continue_headers); + + EXPECT_THROW(response_encoder_->encodeHeaders(continue_headers, true), CodecProtocolException); + EXPECT_EQ(1, stats_store_.counter("http2.rx_messaging_error").value()); +}; + +TEST_P(Http2CodecImplTest, InvalidRepeatContinueAllowed) { + stream_error_on_invalid_http_messaging_ = true; + initialize(); + MockStreamCallbacks request_callbacks; request_encoder_->getStream().addCallbacks(request_callbacks); @@ -280,6 +313,28 @@ TEST_P(Http2CodecImplTest, Invalid103) { TEST_P(Http2CodecImplTest, Invalid204WithContentLength) { initialize(); + TestHeaderMapImpl request_headers; + HttpTestUtility::addDefaultHeaders(request_headers); + EXPECT_CALL(request_decoder_, decodeHeaders_(_, true)); + request_encoder_->encodeHeaders(request_headers, true); + + TestHeaderMapImpl response_headers{{":status", "204"}, {"content-length", "3"}}; + // What follows is a hack to get headers that should span into continuation frames. The default + // maximum frame size is 16K. We will add 3,000 headers that will take us above this size and + // not easily compress with HPACK. (I confirmed this generates 26,468 bytes of header data + // which should contain a continuation.) + for (uint i = 1; i < 3000; i++) { + response_headers.addCopy(std::to_string(i), std::to_string(i)); + } + + EXPECT_THROW(response_encoder_->encodeHeaders(response_headers, false), CodecProtocolException); + EXPECT_EQ(1, stats_store_.counter("http2.rx_messaging_error").value()); +}; + +TEST_P(Http2CodecImplTest, Invalid204WithContentLengthAllowed) { + stream_error_on_invalid_http_messaging_ = true; + initialize(); + MockStreamCallbacks request_callbacks; request_encoder_->getStream().addCallbacks(request_callbacks); @@ -329,7 +384,15 @@ TEST_P(Http2CodecImplTest, RefusedStreamReset) { response_encoder_->getStream().resetStream(StreamResetReason::LocalRefusedStreamReset); } -TEST_P(Http2CodecImplTest, InvalidFrame) { +TEST_P(Http2CodecImplTest, InvalidHeadersFrame) { + initialize(); + + EXPECT_THROW(request_encoder_->encodeHeaders(TestHeaderMapImpl{}, true), CodecProtocolException); + EXPECT_EQ(1, stats_store_.counter("http2.rx_messaging_error").value()); +} + +TEST_P(Http2CodecImplTest, InvalidHeadersFrameAllowed) { + stream_error_on_invalid_http_messaging_ = true; initialize(); MockStreamCallbacks request_callbacks; diff --git a/test/common/http/http2/http2_frame.cc b/test/common/http/http2/http2_frame.cc index fc96cf77b852..368630e1f6ec 100644 --- a/test/common/http/http2/http2_frame.cc +++ b/test/common/http/http2/http2_frame.cc @@ -104,6 +104,12 @@ void Http2Frame::appendHeaderWithoutIndexing(StaticHeaderIndex index, absl::stri appendData(value); } +void Http2Frame::appendEmptyHeader() { + data_.push_back(0x40); + data_.push_back(0x00); + data_.push_back(0x00); +} + Http2Frame Http2Frame::makePingFrame(absl::string_view data) { static constexpr size_t kPingPayloadSize = 8; Http2Frame frame; @@ -169,6 +175,21 @@ Http2Frame Http2Frame::makeMalformedRequest(uint32_t stream_index) { return frame; } +Http2Frame Http2Frame::makeMalformedRequestWithZerolenHeader(uint32_t stream_index, + absl::string_view host, + absl::string_view path) { + Http2Frame frame; + frame.buildHeader(Type::HEADERS, 0, orFlags(HeadersFlags::END_STREAM, HeadersFlags::END_HEADERS), + makeRequestStreamId(stream_index)); + frame.appendStaticHeader(StaticHeaderIndex::METHOD_GET); + frame.appendStaticHeader(StaticHeaderIndex::SCHEME_HTTPS); + frame.appendHeaderWithoutIndexing(StaticHeaderIndex::PATH, path); + frame.appendHeaderWithoutIndexing(StaticHeaderIndex::HOST, host); + frame.appendEmptyHeader(); + frame.adjustPayloadSize(); + return frame; +} + Http2Frame Http2Frame::makeRequest(uint32_t stream_index, absl::string_view host, absl::string_view path) { Http2Frame frame; diff --git a/test/common/http/http2/http2_frame.h b/test/common/http/http2/http2_frame.h index 760ae69e3575..52b838dbb987 100644 --- a/test/common/http/http2/http2_frame.h +++ b/test/common/http/http2/http2_frame.h @@ -79,6 +79,9 @@ class Http2Frame { static Http2Frame makePriorityFrame(uint32_t stream_index, uint32_t dependent_index); static Http2Frame makeWindowUpdateFrame(uint32_t stream_index, uint32_t increment); static Http2Frame makeMalformedRequest(uint32_t stream_index); + static Http2Frame makeMalformedRequestWithZerolenHeader(uint32_t stream_index, + absl::string_view host, + absl::string_view path); static Http2Frame makeRequest(uint32_t stream_index, absl::string_view host, absl::string_view path); static Http2Frame makePostRequest(uint32_t stream_index, absl::string_view host, @@ -126,6 +129,7 @@ class Http2Frame { // Headers are directly encoded void appendStaticHeader(StaticHeaderIndex index); void appendHeaderWithoutIndexing(StaticHeaderIndex index, absl::string_view value); + void appendEmptyHeader(); // This method updates payload length in the HTTP2 header based on the size of the data_ void adjustPayloadSize() { diff --git a/test/integration/http2_integration_test.cc b/test/integration/http2_integration_test.cc index 5861292c9c42..b1a1bb3c6368 100644 --- a/test/integration/http2_integration_test.cc +++ b/test/integration/http2_integration_test.cc @@ -1584,6 +1584,12 @@ TEST_P(Http2FloodMitigationTest, Data) { // Verify that the server can detect flood of RST_STREAM frames. TEST_P(Http2FloodMitigationTest, RST_STREAM) { + // Use invalid HTTP headers to trigger sending RST_STREAM frames. + config_helper_.addConfigModifier( + [](envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& hcm) + -> void { + hcm.mutable_http2_protocol_options()->set_stream_error_on_invalid_http_messaging(true); + }); beginSession(); int i = 0; @@ -1717,4 +1723,54 @@ TEST_P(Http2FloodMitigationTest, WindowUpdate) { "http2.inbound_window_update_frames_flood"); } +// Verify that the HTTP/2 connection is terminated upon receiving invalid HEADERS frame. +TEST_P(Http2FloodMitigationTest, ZerolenHeader) { + beginSession(); + + // Send invalid request. + uint32_t request_idx = 0; + auto request = Http2Frame::makeMalformedRequestWithZerolenHeader(request_idx, "host", "/"); + sendFame(request); + + tcp_client_->waitForDisconnect(); + + EXPECT_EQ(1, test_server_->counter("http2.rx_messaging_error")->value()); + EXPECT_EQ(1, + test_server_->counter("http.config_test.downstream_cx_delayed_close_timeout")->value()); +} + +// Verify that only the offending stream is terminated upon receiving invalid HEADERS frame. +TEST_P(Http2FloodMitigationTest, ZerolenHeaderAllowed) { + config_helper_.addConfigModifier( + [](envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& hcm) + -> void { + hcm.mutable_http2_protocol_options()->set_stream_error_on_invalid_http_messaging(true); + }); + autonomous_upstream_ = true; + beginSession(); + fake_upstreams_[0]->set_allow_unexpected_disconnects(true); + + // Send invalid request. + uint32_t request_idx = 0; + auto request = Http2Frame::makeMalformedRequestWithZerolenHeader(request_idx, "host", "/"); + sendFame(request); + // Make sure we've got RST_STREAM from the server. + auto response = readFrame(); + EXPECT_EQ(Http2Frame::Type::RST_STREAM, response.type()); + + // Send valid request using the same connection. + request_idx++; + request = Http2Frame::makeRequest(request_idx, "host", "/"); + sendFame(request); + response = readFrame(); + EXPECT_EQ(Http2Frame::Type::HEADERS, response.type()); + EXPECT_EQ(Http2Frame::ResponseStatus::_200, response.responseStatus()); + + tcp_client_->close(); + + EXPECT_EQ(1, test_server_->counter("http2.rx_messaging_error")->value()); + EXPECT_EQ(0, + test_server_->counter("http.config_test.downstream_cx_delayed_close_timeout")->value()); +} + } // namespace Envoy diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index d550d793b999..5d6bd41de406 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -592,6 +592,36 @@ TEST_P(DownstreamProtocolIntegrationTest, InvalidContentLength) { {"content-length", "-1"}}); auto response = std::move(encoder_decoder.second); + codec_client_->waitForDisconnect(); + + if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) { + ASSERT_TRUE(response->complete()); + EXPECT_EQ("400", response->headers().Status()->value().getStringView()); + } else { + ASSERT_TRUE(response->reset()); + EXPECT_EQ(Http::StreamResetReason::ConnectionTermination, response->reset_reason()); + } +} + +// TODO(PiotrSikora): move this HTTP/2 only variant to http2_integration_test.cc. +TEST_P(DownstreamProtocolIntegrationTest, InvalidContentLengthAllowed) { + config_helper_.addConfigModifier( + [](envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& hcm) + -> void { + hcm.mutable_http2_protocol_options()->set_stream_error_on_invalid_http_messaging(true); + }); + + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + + auto encoder_decoder = + codec_client_->startRequest(Http::TestHeaderMapImpl{{":method", "POST"}, + {":path", "/test/long/url"}, + {":authority", "host"}, + {"content-length", "-1"}}); + auto response = std::move(encoder_decoder.second); + if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) { codec_client_->waitForDisconnect(); } else { @@ -618,6 +648,34 @@ TEST_P(DownstreamProtocolIntegrationTest, MultipleContentLengths) { {"content-length", "3,2"}}); auto response = std::move(encoder_decoder.second); + codec_client_->waitForDisconnect(); + + if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) { + ASSERT_TRUE(response->complete()); + EXPECT_EQ("400", response->headers().Status()->value().getStringView()); + } else { + ASSERT_TRUE(response->reset()); + EXPECT_EQ(Http::StreamResetReason::ConnectionTermination, response->reset_reason()); + } +} + +// TODO(PiotrSikora): move this HTTP/2 only variant to http2_integration_test.cc. +TEST_P(DownstreamProtocolIntegrationTest, MultipleContentLengthsAllowed) { + config_helper_.addConfigModifier( + [](envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& hcm) + -> void { + hcm.mutable_http2_protocol_options()->set_stream_error_on_invalid_http_messaging(true); + }); + + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + auto encoder_decoder = + codec_client_->startRequest(Http::TestHeaderMapImpl{{":method", "POST"}, + {":path", "/test/long/url"}, + {":authority", "host"}, + {"content-length", "3,2"}}); + auto response = std::move(encoder_decoder.second); + if (downstream_protocol_ == Http::CodecClient::Type::HTTP1) { codec_client_->waitForDisconnect(); } else {