Proxy protocol: sanitise non utf8 chars in TLVs

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>
Signed-off-by: Ryan Northey <ryan@synca.io>

changelogs/current.yaml

@@ -40,6 +40,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

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"

test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc

@@ -1583,6 +1583,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,