Proxy protocol: sanitise non utf8 chars in TLVs

Fix [CVE-2024-23324](GHSA-gq3v-vvhj-96j6) Signed-off-by: Kateryna Nezdolii <[email protected]> Signed-off-by: Ryan Northey <[email protected]>
envoyproxy · Feb 9, 2024 · 29989f6 · 29989f6
1 parent bacd310
commit 29989f6
Show file tree

Hide file tree

Showing 3 changed files with 72 additions and 1 deletion.
diff --git a/changelogs/current.yaml b/changelogs/current.yaml
@@ -62,6 +62,10 @@ bug_fixes:
  change: |
  Fix crash due to uncaught exception when the operating system does not support an address type (such as IPv6) that is
  received in a proxy protocol header. Connections will instead be dropped/reset.
+- area: proxy_protocol
+ change: |
+ Fixed a bug where TLVs with non utf8 characters were inserted as protobuf values into filter metadata circumventing
+ ext_authz checks when ``failure_mode_allow`` is set to ``true``.
 - area: tls
  change: |
  Fix crash due to uncaught exception when the operating system does not support an address type (such as IPv6) that is

diff --git a/source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc b/source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc
@@ -24,6 +24,7 @@
 #include "source/common/network/address_impl.h"
 #include "source/common/network/proxy_protocol_filter_state.h"
 #include "source/common/network/utility.h"
+#include "source/common/protobuf/utility.h"
 #include "source/extensions/common/proxy_protocol/proxy_protocol_header.h"
 
 using envoy::config::core::v3::ProxyProtocolPassThroughTLVs;
@@ -440,7 +441,9 @@ bool Filter::parseTlvs(const uint8_t* buf, size_t len) {
  auto key_value_pair = config_->isTlvTypeNeeded(tlv_type);
  if (nullptr != key_value_pair) {
  ProtobufWkt::Value metadata_value;
- metadata_value.set_string_value(tlv_value.data(), tlv_value.size());
+ // Sanitize any non utf8 characters.
+ auto sanitised_tlv_value = MessageUtil::sanitizeUtf8String(tlv_value);
+ metadata_value.set_string_value(sanitised_tlv_value.data(), sanitised_tlv_value.size());
 
  std::string metadata_key = key_value_pair->metadata_namespace().empty()
  ? "envoy.filters.listener.proxy_protocol"

diff --git a/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc b/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc
@@ -1603,6 +1603,70 @@ TEST_P(ProxyProtocolTest, V2ExtractMultipleTlvsOfInterest) {
  disconnect();
 }
 
+TEST_P(ProxyProtocolTest, V2ExtractMultipleTlvsOfInterestAndSanitiseNonUtf8) {
+ // A well-formed ipv4/tcp with a pair of TLV extensions is accepted.
+ constexpr uint8_t buffer[] = {0x0d, 0x0a, 0x0d, 0x0a, 0x00, 0x0d, 0x0a, 0x51, 0x55, 0x49,
+ 0x54, 0x0a, 0x21, 0x11, 0x00, 0x39, 0x01, 0x02, 0x03, 0x04,
+ 0x00, 0x01, 0x01, 0x02, 0x03, 0x05, 0x00, 0x02};
+ // A TLV of type 0x00 with size of 4 (1 byte is value).
+ constexpr uint8_t tlv1[] = {0x00, 0x00, 0x01, 0xff};
+ // A TLV of type 0x02 with size of 10 bytes (7 bytes are value). Second and last bytes in the
+ // value are non utf8 characters.
+ constexpr uint8_t tlv_type_authority[] = {0x02, 0x00, 0x07, 0x66, 0xfe,
+ 0x6f, 0x2e, 0x63, 0x6f, 0xc1};
+ // A TLV of type 0x0f with size of 6 bytes (3 bytes are value).
+ constexpr uint8_t tlv3[] = {0x0f, 0x00, 0x03, 0xf0, 0x00, 0x0f};
+ // A TLV of type 0xea with size of 25 bytes (22 bytes are value). 7th and 21st bytes are non utf8
+ // characters.
+ constexpr uint8_t tlv_vpc_id[] = {0xea, 0x00, 0x16, 0x01, 0x76, 0x70, 0x63, 0x2d, 0x30,
+ 0xc0, 0x35, 0x74, 0x65, 0x73, 0x74, 0x32, 0x66, 0x61,
+ 0x36, 0x63, 0x36, 0x33, 0x68, 0xf9, 0x37};
+ constexpr uint8_t data[] = {'D', 'A', 'T', 'A'};
+
+ envoy::extensions::filters::listener::proxy_protocol::v3::ProxyProtocol proto_config;
+ auto rule_type_authority = proto_config.add_rules();
+ rule_type_authority->set_tlv_type(0x02);
+ rule_type_authority->mutable_on_tlv_present()->set_key("PP2 type authority");
+
+ auto rule_vpc_id = proto_config.add_rules();
+ rule_vpc_id->set_tlv_type(0xea);
+ rule_vpc_id->mutable_on_tlv_present()->set_key("PP2 vpc id");
+
+ connect(true, &proto_config);
+ write(buffer, sizeof(buffer));
+ dispatcher_->run(Event::Dispatcher::RunType::NonBlock);
+
+ write(tlv1, sizeof(tlv1));
+ write(tlv_type_authority, sizeof(tlv_type_authority));
+ write(tlv3, sizeof(tlv3));
+ write(tlv_vpc_id, sizeof(tlv_vpc_id));
+ write(data, sizeof(data));
+ expectData("DATA");
+
+ EXPECT_EQ(1, server_connection_->streamInfo().dynamicMetadata().filter_metadata_size());
+
+ auto metadata = server_connection_->streamInfo().dynamicMetadata().filter_metadata();
+ EXPECT_EQ(1, metadata.size());
+ EXPECT_EQ(1, metadata.count(ProxyProtocol));
+
+ auto fields = metadata.at(ProxyProtocol).fields();
+ EXPECT_EQ(2, fields.size());
+ EXPECT_EQ(1, fields.count("PP2 type authority"));
+ EXPECT_EQ(1, fields.count("PP2 vpc id"));
+
+ const char replacement = 0x21;
+ auto value_type_authority = fields.at("PP2 type authority").string_value();
+ // Non utf8 characters have been replaced with `0x21` (`!` character).
+ ASSERT_THAT(value_type_authority,
+ ElementsAre(0x66, replacement, 0x6f, 0x2e, 0x63, 0x6f, replacement));
+
+ auto value_vpc_id = fields.at("PP2 vpc id").string_value();
+ ASSERT_THAT(value_vpc_id,
+ ElementsAre(0x01, 0x76, 0x70, 0x63, 0x2d, 0x30, replacement, 0x35, 0x74, 0x65, 0x73,
+ 0x74, 0x32, 0x66, 0x61, 0x36, 0x63, 0x36, 0x33, 0x68, replacement, 0x37));
+ disconnect();
+}
+
 TEST_P(ProxyProtocolTest, V2WillNotOverwriteTLV) {
  // A well-formed ipv4/tcp with a pair of TLV extensions is accepted
  constexpr uint8_t buffer[] = {0x0d, 0x0a, 0x0d, 0x0a, 0x00, 0x0d, 0x0a, 0x51, 0x55, 0x49,