[balsa] Add config field to enable custom methods. (envoyproxy#26448)

* [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: envoyproxy#21245

Signed-off-by: Bence Béky <bnc@google.com>
Signed-off-by: Ryan Eskin <ryan.eskin89@protonmail.com>

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
+  // <envoy_v3_api_field_extensions.http.header_validators.envoy_default.v3.HeaderValidatorConfig.restrict_http_methods>`
+  // to reject custom methods.
+  bool allow_custom_methods = 10 [(xds.annotations.v3.field_status).work_in_progress = true];
 }
 
 message KeepaliveSettings {

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};
 };
 
 /**

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;

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;

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<BalsaParser>(type, this, max_headers_kb_ * 1024, enableTrailers());
+    parser_ = std::make_unique<BalsaParser>(type, this, max_headers_kb_ * 1024, enableTrailers(),
+                                            codec_settings_.allow_custom_methods_);
   } else {
     parser_ = std::make_unique<LegacyHttpParserImpl>(type, this);
   }

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;
 }
 

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;
   }

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{

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<ProtobufMessage::MockValidationVisitor> 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";

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