From 70a77242a5307f0a788d4b68415dacfe9e1c2a69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bence=20B=C3=A9ky?= Date: Wed, 20 Jul 2022 12:08:58 -0400 Subject: [PATCH 1/5] [balsa] Fix header size limit error messages. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change error messages for too large headers that match ConnectionImpl::checkMaxHeadersSize() behavior. When http-parser parses a piece of data, it immediately calls ParserCallbacks::onHeaderField() or ParserCallbacks::onHeaderValue() with the current fragment of header key or value, and ConnectionImpl::checkMaxHeadersSize() checks size limit each time. On the other hand, BalsaFrame buffers the header key and value and only calls BalsaVisitorInterface::OnHeader() when the given header completes. In order to prevent from memory attacks, the header size limit is passed to BalsaFrame through BalsaParser constructor, and BalsaFrame internally enforces the limit while parsing. Signed-off-by: Bence Béky --- source/common/http/http1/balsa_parser.cc | 5 ++- test/common/http/http1/codec_impl_test.cc | 54 +---------------------- 2 files changed, 5 insertions(+), 54 deletions(-) diff --git a/source/common/http/http1/balsa_parser.cc b/source/common/http/http1/balsa_parser.cc index 550f453a7288..f38620da18b2 100644 --- a/source/common/http/http1/balsa_parser.cc +++ b/source/common/http/http1/balsa_parser.cc @@ -266,7 +266,10 @@ void BalsaParser::HandleError(BalsaFrameEnums::ErrorCode error_code) { error_message_ = "HPE_INVALID_CHUNK_SIZE"; break; case BalsaFrameEnums::HEADERS_TOO_LONG: - error_message_ = "size exceeds limit"; + error_message_ = "headers size exceeds limit"; + break; + case BalsaFrameEnums::TRAILER_TOO_LONG: + error_message_ = "trailers size exceeds limit"; break; default: error_message_ = BalsaFrameEnums::ErrorCodeToString(error_code); diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 98bf95d2549b..9b31532dffbe 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -922,11 +922,6 @@ TEST_P(Http1ServerConnectionImplTest, Http11AbsoluteEnabledNoOp) { } TEST_P(Http1ServerConnectionImplTest, Http11InvalidRequest) { - if (parser_impl_ == ParserImpl::BalsaParser) { - // TODO(#21245): Re-enable this test for BalsaParser. - return; - } - initialize(); // Invalid because www.somewhere.com is not an absolute path nor an absolute url @@ -1794,14 +1789,7 @@ TEST_P(Http1ServerConnectionImplTest, DoubleRequest) { TEST_P(Http1ServerConnectionImplTest, RequestWithTrailersDropped) { expectTrailersTest(false); } -TEST_P(Http1ServerConnectionImplTest, RequestWithTrailersKept) { - if (parser_impl_ == ParserImpl::BalsaParser) { - // TODO(#21245): Re-enable this test for BalsaParser. - return; - } - - expectTrailersTest(true); -} +TEST_P(Http1ServerConnectionImplTest, RequestWithTrailersKept) { expectTrailersTest(true); } TEST_P(Http1ServerConnectionImplTest, IgnoreUpgradeH2c) { initialize(); @@ -2926,11 +2914,6 @@ TEST_P(Http1ClientConnectionImplTest, LowWatermarkDuringClose) { } TEST_P(Http1ServerConnectionImplTest, LargeTrailersRejected) { - if (parser_impl_ == ParserImpl::BalsaParser) { - // TODO(#21245): Re-enable this test for BalsaParser. - return; - } - // Default limit of 60 KiB std::string long_string = "big: " + std::string(60 * 1024, 'q') + "\r\n\r\n\r\n"; testTrailersExceedLimit(long_string, "http/1.1 protocol error: trailers size exceeds limit", @@ -2938,11 +2921,6 @@ TEST_P(Http1ServerConnectionImplTest, LargeTrailersRejected) { } TEST_P(Http1ServerConnectionImplTest, LargeTrailerFieldRejected) { - if (parser_impl_ == ParserImpl::BalsaParser) { - // TODO(#21245): Re-enable this test for BalsaParser. - return; - } - // Construct partial headers with a long field name that exceeds the default limit of 60KiB. std::string long_string = "bigfield" + std::string(60 * 1024, 'q'); testTrailersExceedLimit(long_string, "http/1.1 protocol error: trailers size exceeds limit", @@ -2951,11 +2929,6 @@ TEST_P(Http1ServerConnectionImplTest, LargeTrailerFieldRejected) { // Tests that the default limit for the number of request headers is 100. TEST_P(Http1ServerConnectionImplTest, ManyTrailersRejected) { - if (parser_impl_ == ParserImpl::BalsaParser) { - // TODO(#21245): Re-enable this test for BalsaParser. - return; - } - // Send a request with 101 headers. testTrailersExceedLimit(createHeaderFragment(101) + "\r\n\r\n", "http/1.1 protocol error: trailers count exceeds limit", true); @@ -3020,11 +2993,6 @@ TEST_P(Http1ServerConnectionImplTest, LargeRequestUrlRejected) { } TEST_P(Http1ServerConnectionImplTest, LargeRequestHeadersRejected) { - if (parser_impl_ == ParserImpl::BalsaParser) { - // TODO(#21245): Re-enable this test for BalsaParser. - return; - } - // Default limit of 60 KiB std::string long_string = "big: " + std::string(60 * 1024, 'q') + "\r\n"; testRequestHeadersExceedLimit(long_string, "http/1.1 protocol error: headers size exceeds limit", @@ -3032,11 +3000,6 @@ TEST_P(Http1ServerConnectionImplTest, LargeRequestHeadersRejected) { } TEST_P(Http1ServerConnectionImplTest, LargeRequestHeadersRejectedBeyondMaxConfigurable) { - if (parser_impl_ == ParserImpl::BalsaParser) { - // TODO(#21245): Re-enable this test for BalsaParser. - return; - } - max_request_headers_kb_ = 8192; std::string long_string = "big: " + std::string(8193 * 1024, 'q') + "\r\n"; testRequestHeadersExceedLimit(long_string, "http/1.1 protocol error: headers size exceeds limit", @@ -3324,11 +3287,6 @@ TEST_P(Http1ServerConnectionImplTest, PipedRequestWithMutipleEvent) { } TEST_P(Http1ServerConnectionImplTest, Utf8Path) { - if (parser_impl_ == ParserImpl::BalsaParser) { - // TODO(#21245): Re-enable this test for BalsaParser. - return; - } - initialize(); MockRequestDecoder decoder; @@ -3358,11 +3316,6 @@ TEST_P(Http1ServerConnectionImplTest, Utf8Path) { // Tests that incomplete response headers of 80 kB header value fails. TEST_P(Http1ClientConnectionImplTest, ResponseHeadersWithLargeValueRejected) { - if (parser_impl_ == ParserImpl::BalsaParser) { - // TODO(#21245): Re-enable this test for BalsaParser. - return; - } - initialize(); NiceMock response_decoder; @@ -3382,11 +3335,6 @@ TEST_P(Http1ClientConnectionImplTest, ResponseHeadersWithLargeValueRejected) { // Tests that incomplete response headers with a 80 kB header field fails. TEST_P(Http1ClientConnectionImplTest, ResponseHeadersWithLargeFieldRejected) { - if (parser_impl_ == ParserImpl::BalsaParser) { - // TODO(#21245): Re-enable this test for BalsaParser. - return; - } - initialize(); NiceMock decoder; From 9705041ff8414aea3f854e238b040cccd4952370 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bence=20B=C3=A9ky?= Date: Thu, 28 Jul 2022 13:59:00 -0400 Subject: [PATCH 2/5] Re-disable Parsers/Http1ServerConnectionImplTest.Utf8Path/BalsaParser. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Bence Béky --- test/common/http/http1/codec_impl_test.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 9b31532dffbe..c56dbc7dd5b4 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -3287,6 +3287,11 @@ TEST_P(Http1ServerConnectionImplTest, PipedRequestWithMutipleEvent) { } TEST_P(Http1ServerConnectionImplTest, Utf8Path) { + if (parser_impl_ == ParserImpl::BalsaParser) { + // TODO(#21245): Re-enable this test for BalsaParser. + return; + } + initialize(); MockRequestDecoder decoder; From ecfb2b59f3a0a5b519a098b3027c0a5791387f84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bence=20B=C3=A9ky?= Date: Tue, 2 Aug 2022 12:17:50 -0400 Subject: [PATCH 3/5] Further improvements. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Bence Béky --- source/common/http/http1/codec_impl.cc | 7 ++++++- test/common/http/http1/codec_impl_test.cc | 5 ----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 5ed61ca014fd..3c96feaa8000 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -687,7 +687,12 @@ Envoy::StatusOr ConnectionImpl::dispatchSlice(const char* slice, size_t const ParserStatus status = parser_->getStatus(); if (status != ParserStatus::Ok && status != ParserStatus::Paused) { - RETURN_IF_ERROR(sendProtocolError(Http1ResponseCodeDetails::get().HttpCodecError)); + absl::string_view error = Http1ResponseCodeDetails::get().HttpCodecError; + if (parser_->errorMessage() == "headers size exceeds limit" || + parser_->errorMessage() == "trailers size exceeds limit") { + error = Http1ResponseCodeDetails::get().HeadersTooLarge; + } + RETURN_IF_ERROR(sendProtocolError(error)); // Avoid overwriting the codec_status_ set in the callbacks. ASSERT(codec_status_.ok()); codec_status_ = diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index c56dbc7dd5b4..8ec69c0f8aab 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -2966,11 +2966,6 @@ TEST_P(Http1ServerConnectionImplTest, ManyTrailersIgnored) { } TEST_P(Http1ServerConnectionImplTest, LargeRequestUrlRejected) { - if (parser_impl_ == ParserImpl::BalsaParser) { - // TODO(#21245): Re-enable this test for BalsaParser. - return; - } - initialize(); std::string exception_reason; From c2fa4b44cd7b31db49603bb1479616e0d5af2bfe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bence=20B=C3=A9ky?= Date: Tue, 2 Aug 2022 16:20:03 -0400 Subject: [PATCH 4/5] Re-enable two more tests. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Bence Béky --- test/common/http/http1/codec_impl_test.cc | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index bbacbb428f6c..bce63d550d61 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -3008,11 +3008,6 @@ TEST_P(Http1ServerConnectionImplTest, ManyRequestHeadersRejected) { } TEST_P(Http1ServerConnectionImplTest, LargeRequestHeadersSplitRejected) { - if (parser_impl_ == ParserImpl::BalsaParser) { - // TODO(#21245): Re-enable this test for BalsaParser. - return; - } - // Default limit of 60 KiB initialize(); @@ -3042,11 +3037,6 @@ TEST_P(Http1ServerConnectionImplTest, LargeRequestHeadersSplitRejected) { } TEST_P(Http1ServerConnectionImplTest, LargeRequestHeadersSplitRejectedMaxConfigurable) { - if (parser_impl_ == ParserImpl::BalsaParser) { - // TODO(#21245): Re-enable this test for BalsaParser. - return; - } - max_request_headers_kb_ = 8192; max_request_headers_count_ = 150; initialize(); From 877a0225f22f5c95fad438b54031f09bc3a50dd6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bence=20B=C3=A9ky?= Date: Wed, 3 Aug 2022 08:43:59 -0400 Subject: [PATCH 5/5] Protect ConnectionImpl::dispatchSlice() behavior change with existing flag. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Bence Béky --- source/common/http/http1/codec_impl.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index a1ebfb376223..e41097b3e4e1 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -688,8 +688,9 @@ Envoy::StatusOr ConnectionImpl::dispatchSlice(const char* slice, size_t const ParserStatus status = parser_->getStatus(); if (status != ParserStatus::Ok && status != ParserStatus::Paused) { absl::string_view error = Http1ResponseCodeDetails::get().HttpCodecError; - if (parser_->errorMessage() == "headers size exceeds limit" || - parser_->errorMessage() == "trailers size exceeds limit") { + if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.http1_use_balsa_parser") && + (parser_->errorMessage() == "headers size exceeds limit" || + parser_->errorMessage() == "trailers size exceeds limit")) { error = Http1ResponseCodeDetails::get().HeadersTooLarge; } RETURN_IF_ERROR(sendProtocolError(error));