CVE-2024-23324: Insufficient Information (NVD-CWE-noinfo)
value: Envoy is a high-performance edge/middle/service proxy. External authentication can be bypassed by downstream connections. Downstream clients can force invalid gRPC requests to be sent to ext_authz, circumventing ext_authz checks when failure_mode_allow is set to true. This issue has been addressed in released 1.29.1, 1.28.1, 1.27.3, and 1.26.7. Users are advised to upgrade. There are no known workarounds for this vulnerability.

CWE description: There is insufficient information about the issue to classify it; details are unkown or unspecified.
CVSS v3 base score: 7.5
Published: 2024-02-09T23:15Z
Repository: https://github.com/envoyproxy/envoy
Language: C++
Files changed: changelogs/current.yaml, source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc, test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc
Commit date: 2024-02-09 17:52:19 +0000
Commit message: Proxy protocol: sanitise non utf8 chars in TLVs

Fix [CVE-2024-23324](https://github.com/envoyproxy/envoy/security/advisories/GHSA-gq3v-vvhj-96j6)

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

Diff: diff --git a/changelogs/current.yaml b/changelogs/current.yaml
index 193ebcf39e5..e8b5f0798a4 100644
--- a/changelogs/current.yaml
+++ b/changelogs/current.yaml
@@ -60,10 +60,14 @@ bug_fixes:
     Fixed crash when HTTP request idle and per try timeouts occurs within backoff interval.
 - area: proxy_protocol
   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
     received in an mTLS client cert IP SAN. These SANs will be ignored. This applies only when using formatter
     ``%DOWNSTREAM_PEER_IP_SAN%``.
diff --git a/source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc b/source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc
index 892ad0288b0..265eb570a0b 100644
--- a/source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc
+++ b/source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc
@@ -22,10 +22,11 @@
 #include "source/common/common/safe_memcpy.h"
 #include "source/common/common/utility.h"
 #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;
 using Envoy::Extensions::Common::ProxyProtocol::PROXY_PROTO_V1_SIGNATURE;
 using Envoy::Extensions::Common::ProxyProtocol::PROXY_PROTO_V1_SIGNATURE_LEN;
@@ -438,11 +439,13 @@ bool Filter::parseTlvs(const uint8_t* buf, size_t len) {
     // Only save to dynamic metadata if this type of TLV is needed.
     absl::string_view tlv_value(reinterpret_cast<char const*>(buf + idx), tlv_value_length);
     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"
                                      : key_value_pair->metadata_namespace();
 
diff --git a/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc b/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc
index 61adbd96ff5..26f55921fa3 100644
--- a/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc
+++ b/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc
@@ -1601,10 +1601,74 @@ TEST_P(ProxyProtocolTest, V2ExtractMultipleTlvsOfInterest) {
               ElementsAre(0x01, 0x76, 0x70, 0x63, 0x2d, 0x30, 0x32, 0x35, 0x74, 0x65, 0x73, 0x74,
                           0x32, 0x66, 0x61, 0x36, 0x63, 0x36, 0x33, 0x68, 0x61, 0x37));
   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,
                                 0x54, 0x0a, 0x21, 0x11, 0x00, 0x2a, 0x01, 0x02, 0x03, 0x04,
                                 0x00, 0x01, 0x01, 0x02, 0x03, 0x05, 0x00, 0x02};

Vulnerable code:       metadata_value.set_string_value(tlv_value.data(), tlv_value.size());
Fixed code: - 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``.
#include "source/common/protobuf/utility.h"
      // 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());
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();
}