From 3e6e83d166c2ec70aa07a60be088ac76a60734a9 Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Fri, 12 Jan 2024 13:43:40 +0000 Subject: [PATCH] Proxy protocol: sanitise non utf8 chars in TLVs Signed-off-by: Kateryna Nezdolii Signed-off-by: Ryan Northey --- changelogs/current.yaml | 4 ++ .../listener/proxy_protocol/proxy_protocol.cc | 5 +- .../proxy_protocol/proxy_protocol_test.cc | 64 +++++++++++++++++++ 3 files changed, 72 insertions(+), 1 deletion(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index e706d75737db..8cd88bff6042 100644 --- a/changelogs/current.yaml +++ b/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 diff --git a/source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc b/source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc index 892ad0288b0b..265eb570a0b3 100644 --- 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 index 57ab7142cd56..d54538032506 100644 --- a/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc +++ b/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,