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());