From 511707d8eca6db72ce3a95464f64ee8f14fa9c35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bence=20B=C3=A9ky?= Date: Fri, 28 Apr 2023 16:12:34 -0400 Subject: [PATCH] [balsa] Add config field to enable custom methods. (#26448) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [balsa] Add config field to enable custom methods. This is no behavioral change by default: only methods from a hard-coded list (that matches the list hard-coded in http-parser, and is slightly different from the one that will be used by UHV) are accepted. Then the new knob is true, BalsaParser does the exact same validation as UHV will by default: method has to be non-empty and only contain allowed characters. When UHV method validation logic is turned on in the future, all validation can be removed from BalsaParser. When non-UHV mode is deprecated, this new proto field can be removed. Tracking issue: #21245 Signed-off-by: Bence Béky Signed-off-by: Ryan Eskin --- api/envoy/config/core/v3/protocol.proto | 15 +++++- envoy/http/codec.h | 5 ++ source/common/http/http1/balsa_parser.cc | 39 ++++++++++----- source/common/http/http1/balsa_parser.h | 3 +- source/common/http/http1/codec_impl.cc | 3 +- source/common/http/http1/settings.cc | 2 + .../transport_sockets/http_11_proxy/connect.h | 3 +- test/common/http/http1/codec_impl_test.cc | 50 ++++++++++++------- test/common/http/utility_test.cc | 14 ++++++ test/integration/protocol_integration_test.cc | 41 +++++---------- 10 files changed, 111 insertions(+), 64 deletions(-) diff --git a/api/envoy/config/core/v3/protocol.proto b/api/envoy/config/core/v3/protocol.proto index 620a2ef59801..95670bb752b4 100644 --- a/api/envoy/config/core/v3/protocol.proto +++ b/api/envoy/config/core/v3/protocol.proto @@ -259,7 +259,7 @@ message HttpProtocolOptions { google.protobuf.UInt32Value max_requests_per_connection = 6; } -// [#next-free-field: 10] +// [#next-free-field: 11] message Http1ProtocolOptions { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.core.Http1ProtocolOptions"; @@ -358,6 +358,19 @@ message Http1ProtocolOptions { // See issue #21245. google.protobuf.BoolValue use_balsa_parser = 9 [(xds.annotations.v3.field_status).work_in_progress = true]; + + // [#not-implemented-hide:] Hiding so that field can be removed. + // If true, and BalsaParser is used (either `use_balsa_parser` above is true, + // or `envoy.reloadable_features.http1_use_balsa_parser` is true and + // `use_balsa_parser` is unset), then every non-empty method with only valid + // characters is accepted. Otherwise, methods not on the hard-coded list are + // rejected. + // Once UHV is enabled, this field should be removed, and BalsaParser should + // allow any method. UHV validates the method, rejecting empty string or + // invalid characters, and provides :ref:`restrict_http_methods + // ` + // to reject custom methods. + bool allow_custom_methods = 10 [(xds.annotations.v3.field_status).work_in_progress = true]; } message KeepaliveSettings { diff --git a/envoy/http/codec.h b/envoy/http/codec.h index f9fb05d857bd..5ad04479d47f 100644 --- a/envoy/http/codec.h +++ b/envoy/http/codec.h @@ -526,6 +526,11 @@ struct Http1Settings { // If true, BalsaParser is used for HTTP/1 parsing; if false, http-parser is // used. See issue #21245. bool use_balsa_parser_{false}; + + // If true, any non-empty method composed of valid characters is accepted. + // If false, only methods from a hard-coded list of known methods are accepted. + // Only implemented in BalsaParser. http-parser only accepts known methods. + bool allow_custom_methods_{false}; }; /** diff --git a/source/common/http/http1/balsa_parser.cc b/source/common/http/http1/balsa_parser.cc index 707f3c2391e3..a274a76e6723 100644 --- a/source/common/http/http1/balsa_parser.cc +++ b/source/common/http/http1/balsa_parser.cc @@ -23,9 +23,6 @@ constexpr absl::string_view kColonSlashSlash = "://"; // Response must start with "HTTP". constexpr char kResponseFirstByte = 'H'; -#ifndef ENVOY_ENABLE_UHV -// When UHV is enabled, BalsaParser allows any method and defers to UHV for -// validation. When UHV is disabled, BalsaParser behavior matches http-parser. bool isFirstCharacterOfValidMethod(char c) { static constexpr char kValidFirstCharacters[] = {'A', 'B', 'C', 'D', 'G', 'H', 'L', 'M', 'N', 'O', 'P', 'R', 'S', 'T', 'U'}; @@ -35,7 +32,27 @@ bool isFirstCharacterOfValidMethod(char c) { return std::binary_search(begin, end, c); } -bool isMethodValid(absl::string_view method) { +// TODO(#21245): Skip method validation altogether when UHV method validation is +// enabled. +bool isMethodValid(absl::string_view method, bool allow_custom_methods) { + if (allow_custom_methods) { + // Allowed characters in method according to RFC 9110, + // https://www.rfc-editor.org/rfc/rfc9110.html#section-5.1. + static constexpr char kValidCharacters[] = { + '!', '#', '$', '%', '&', '\'', '*', '+', '-', '.', '0', '1', '2', '3', '4', '5', + '6', '7', '8', '9', 'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', + 'M', 'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z', '^', '_', + '`', 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', + 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z', '|', '~'}; + const auto* begin = &kValidCharacters[0]; + const auto* end = &kValidCharacters[ABSL_ARRAYSIZE(kValidCharacters) - 1] + 1; + + return !method.empty() && + std::all_of(method.begin(), method.end(), [begin, end](absl::string_view::value_type c) { + return std::binary_search(begin, end, c); + }); + } + static constexpr absl::string_view kValidMethods[] = { "ACL", "BIND", "CHECKOUT", "CONNECT", "COPY", "DELETE", "GET", "HEAD", "LINK", "LOCK", "MERGE", "MKACTIVITY", "MKCALENDAR", "MKCOL", @@ -47,7 +64,6 @@ bool isMethodValid(absl::string_view method) { const auto* end = &kValidMethods[ABSL_ARRAYSIZE(kValidMethods) - 1] + 1; return std::binary_search(begin, end, method); } -#endif // This function is crafted to match the URL validation behavior of the http-parser library. bool isUrlValid(absl::string_view url, bool is_connect) { @@ -125,8 +141,8 @@ bool isVersionValid(absl::string_view version_input) { } // anonymous namespace BalsaParser::BalsaParser(MessageType type, ParserCallbacks* connection, size_t max_header_length, - bool enable_trailers) - : message_type_(type), connection_(connection) { + bool enable_trailers, bool allow_custom_methods) + : message_type_(type), connection_(connection), allow_custom_methods_(allow_custom_methods) { ASSERT(connection_ != nullptr); quiche::HttpValidationPolicy http_validation_policy; @@ -158,13 +174,12 @@ size_t BalsaParser::execute(const char* slice, int len) { ASSERT(status_ != ParserStatus::Error); if (len > 0 && !first_byte_processed_) { -#ifndef ENVOY_ENABLE_UHV - if (message_type_ == MessageType::Request && !isFirstCharacterOfValidMethod(*slice)) { + if (message_type_ == MessageType::Request && !allow_custom_methods_ && + !isFirstCharacterOfValidMethod(*slice)) { status_ = ParserStatus::Error; error_message_ = "HPE_INVALID_METHOD"; return 0; } -#endif if (message_type_ == MessageType::Response && *slice != kResponseFirstByte) { status_ = ParserStatus::Error; error_message_ = "HPE_INVALID_CONSTANT"; @@ -266,13 +281,11 @@ void BalsaParser::OnRequestFirstLineInput(absl::string_view /*line_input*/, if (status_ == ParserStatus::Error) { return; } -#ifndef ENVOY_ENABLE_UHV - if (!isMethodValid(method_input)) { + if (!isMethodValid(method_input, allow_custom_methods_)) { status_ = ParserStatus::Error; error_message_ = "HPE_INVALID_METHOD"; return; } -#endif const bool is_connect = method_input == Headers::get().MethodValues.Connect; if (!isUrlValid(request_uri, is_connect)) { status_ = ParserStatus::Error; diff --git a/source/common/http/http1/balsa_parser.h b/source/common/http/http1/balsa_parser.h index 38607f2100c1..088051612523 100644 --- a/source/common/http/http1/balsa_parser.h +++ b/source/common/http/http1/balsa_parser.h @@ -17,7 +17,7 @@ namespace Http1 { class BalsaParser : public Parser, public quiche::BalsaVisitorInterface { public: BalsaParser(MessageType type, ParserCallbacks* connection, size_t max_header_length, - bool enable_trailers); + bool enable_trailers, bool allow_custom_methods); ~BalsaParser() override = default; // Http1::Parser implementation @@ -72,6 +72,7 @@ class BalsaParser : public Parser, public quiche::BalsaVisitorInterface { const MessageType message_type_ = MessageType::Request; ParserCallbacks* connection_ = nullptr; + const bool allow_custom_methods_ = false; bool first_byte_processed_ = false; bool headers_done_ = false; ParserStatus status_ = ParserStatus::Ok; diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 405b75983f2b..4b36728f02c6 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -526,7 +526,8 @@ ConnectionImpl::ConnectionImpl(Network::Connection& connection, CodecStats& stat deferred_end_stream_headers_(false), dispatching_(false), max_headers_kb_(max_headers_kb), max_headers_count_(max_headers_count) { if (codec_settings_.use_balsa_parser_) { - parser_ = std::make_unique(type, this, max_headers_kb_ * 1024, enableTrailers()); + parser_ = std::make_unique(type, this, max_headers_kb_ * 1024, enableTrailers(), + codec_settings_.allow_custom_methods_); } else { parser_ = std::make_unique(type, this); } diff --git a/source/common/http/http1/settings.cc b/source/common/http/http1/settings.cc index 7c68a49c7c52..e74af4f49135 100644 --- a/source/common/http/http1/settings.cc +++ b/source/common/http/http1/settings.cc @@ -39,6 +39,8 @@ Http1Settings parseHttp1Settings(const envoy::config::core::v3::Http1ProtocolOpt Runtime::runtimeFeatureEnabled("envoy.reloadable_features.http1_use_balsa_parser"); } + ret.allow_custom_methods_ = config.allow_custom_methods(); + return ret; } diff --git a/source/extensions/transport_sockets/http_11_proxy/connect.h b/source/extensions/transport_sockets/http_11_proxy/connect.h index e570cc7ac743..2e06915a11f1 100644 --- a/source/extensions/transport_sockets/http_11_proxy/connect.h +++ b/source/extensions/transport_sockets/http_11_proxy/connect.h @@ -62,7 +62,8 @@ class UpstreamHttp11ConnectSocketFactory : public PassthroughFactory { class SelfContainedParser : public Http::Http1::ParserCallbacks { public: SelfContainedParser() - : parser_(Http::Http1::MessageType::Response, this, 2000, /* enable_trailers = */ false) {} + : parser_(Http::Http1::MessageType::Response, this, 2000, /* enable_trailers = */ false, + /* allow_custom_methods = */ false) {} Http::Http1::CallbackResult onMessageBegin() override { return Http::Http1::CallbackResult::Success; } diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 6a65b3499538..227b498f1c9a 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -1226,36 +1226,48 @@ TEST_P(Http1ServerConnectionImplTest, BadRequestNoStream) { EXPECT_TRUE(isCodecProtocolError(status)); } -TEST_P(Http1ServerConnectionImplTest, CustomMethod) { - bool reject_custom_methods = true; -#ifdef ENVOY_ENABLE_UHV - if (parser_impl_ == Http1ParserImpl::BalsaParser) { - // When both BalsaParser and UHV are enabled, custom methods are allowed by - // default. - reject_custom_methods = false; - } -#endif - +TEST_P(Http1ServerConnectionImplTest, RejectCustomMethod) { initialize(); MockRequestDecoder decoder; EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); - if (reject_custom_methods) { - EXPECT_CALL(decoder, sendLocalReply(_, _, _, _, _)); - } + EXPECT_CALL(decoder, sendLocalReply(_, _, _, _, _)); Buffer::OwnedImpl buffer("BAD / HTTP/1.1\r\n"); auto status = codec_->dispatch(buffer); - if (reject_custom_methods) { - // http-parser rejects unknown methods. - // This behavior was observed during CVE-2019-18801 and helped to limit the - // scope of affected Envoy configurations. - EXPECT_TRUE(isCodecProtocolError(status)); - EXPECT_EQ(status.message(), "http/1.1 protocol error: HPE_INVALID_METHOD"); + EXPECT_TRUE(isCodecProtocolError(status)); + EXPECT_EQ(status.message(), "http/1.1 protocol error: HPE_INVALID_METHOD"); +} + +TEST_P(Http1ServerConnectionImplTest, RejectInvalidCharacterInMethod) { + codec_settings_.allow_custom_methods_ = true; + initialize(); + + MockRequestDecoder decoder; + EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); + EXPECT_CALL(decoder, sendLocalReply(_, _, _, _, _)); + + Buffer::OwnedImpl buffer("B{}D / HTTP/1.1\r\n"); + auto status = codec_->dispatch(buffer); + + EXPECT_TRUE(isCodecProtocolError(status)); + EXPECT_EQ(status.message(), "http/1.1 protocol error: HPE_INVALID_METHOD"); +} + +TEST_P(Http1ServerConnectionImplTest, AllowCustomMethod) { + if (parser_impl_ == Http1ParserImpl::HttpParser) { return; } + codec_settings_.allow_custom_methods_ = true; + initialize(); + + MockRequestDecoder decoder; + EXPECT_CALL(callbacks_, newStream(_, _)).WillOnce(ReturnRef(decoder)); + + Buffer::OwnedImpl buffer("BAD / HTTP/1.1\r\n"); + auto status = codec_->dispatch(buffer); ASSERT_TRUE(status.ok()); TestRequestHeaderMapImpl expected_headers{ diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 4cadf8ec167d..aeecabe7f94e 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -674,6 +674,20 @@ TEST(HttpUtility, UseBalsaParser) { .use_balsa_parser_); } +TEST(HttpUtility, AllowCustomMethods) { + envoy::config::core::v3::Http1ProtocolOptions http1_options; + ProtobufWkt::BoolValue hcm_value; + NiceMock validation_visitor; + + EXPECT_FALSE(http1_options.allow_custom_methods()); + EXPECT_FALSE(Http1::parseHttp1Settings(http1_options, validation_visitor, hcm_value, false) + .allow_custom_methods_); + + http1_options.set_allow_custom_methods(true); + EXPECT_TRUE(Http1::parseHttp1Settings(http1_options, validation_visitor, hcm_value, false) + .allow_custom_methods_); +} + TEST(HttpUtility, getLastAddressFromXFF) { { const std::string first_address = "192.0.2.10"; diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index d8227d3178ab..f742e4cad2d5 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -2390,33 +2390,19 @@ TEST_P(DownstreamProtocolIntegrationTest, ManyTrailerHeaders) { // :method request headers, since the case of other large headers is // covered in the various testLargeRequest-based integration tests here. // -// Tests are run with two HTTP/1 parsers: http-parser and BalsaParser. -// http-parser rejects large method strings, because it only accepts known -// methods from a hardcoded list. BalsaParser mimics http-parser behavior when -// UHV is disabled, but defers method validation to UHV when it is enabled. +// Both HTTP/1 parsers (http-parser and BalsaParser) reject large method strings +// by default, because they only accepts known methods from a hardcoded list. +// HTTP/2 and HTTP/3 codecs accept large methods. The table below describes the +// expected behaviors (in addition we should never see an ASSERT or ASAN failure +// trigger). // -// In addition to BalsaParser with UHV enabled, H2 and H3 codecs also accept -// large methods. The table below describes the expected behaviors (in addition -// we should never see an ASSERT or ASAN failure trigger). -// -// Downstream proto Upstream proto Behavior expected -// -------------------------------------------------------- -// accepts accepts Success -// accepts rejects Envoy will forward; backend will reject -// rejects accepts Envoy will reject -// rejects rejects Envoy will reject +// Downstream proto Upstream proto Behavior expected +// ---------------------------------------------------------- +// HTTP/2 or HTTP/3 HTTP/2 or HTTP/3 Success +// HTTP/2 or HTTP/3 HTTP/1 Envoy will forward; backend will reject +// HTTP/1 HTTP/2 or HTTP/3 Envoy will reject +// HTTP/1 HTTP/1 Envoy will reject TEST_P(ProtocolIntegrationTest, LargeRequestMethod) { - bool http1_codec_rejects_large_method = true; -#ifdef ENVOY_ENABLE_UHV - if (GetParam().http1_implementation == Http1ParserImpl::BalsaParser) { - http1_codec_rejects_large_method = false; - } -#endif - const bool downstream_proto_rejects_large_method = - downstreamProtocol() == Http::CodecType::HTTP1 && http1_codec_rejects_large_method; - const bool upstream_proto_rejects_large_method = - upstreamProtocol() == Http::CodecType::HTTP1 && http1_codec_rejects_large_method; - // There will be no upstream connections for HTTP/1 downstream, we need to // test the full mesh regardless. testing_upstream_intentionally_ = true; @@ -2428,15 +2414,14 @@ TEST_P(ProtocolIntegrationTest, LargeRequestMethod) { initialize(); codec_client_ = makeHttpConnection(lookupPort("http")); - - if (downstream_proto_rejects_large_method) { + if (downstreamProtocol() == Http::CodecType::HTTP1) { auto encoder_decoder = codec_client_->startRequest(request_headers); request_encoder_ = &encoder_decoder.first; auto response = std::move(encoder_decoder.second); ASSERT_TRUE(codec_client_->waitForDisconnect()); EXPECT_TRUE(response->complete()); EXPECT_EQ("400", response->headers().getStatusValue()); - } else if (upstream_proto_rejects_large_method) { + } else if (upstreamProtocol() == Http::CodecType::HTTP1) { auto response = codec_client_->makeHeaderOnlyRequest(request_headers); ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_)); ASSERT_TRUE(response->waitForEndStream());