Fix crash from AWS NLB healthchecks when proxy protocol is enabled

Fix: [CVE-2024-23327](GHSA-4h5x-x9vh-m29j) Signed-off-by: Jacob Neil Taylor <[email protected]> Signed-off-by: Greg Greenway <[email protected]> Signed-off-by: Ryan Northey <[email protected]>
envoyproxy · Feb 9, 2024 · 63895ea · 63895ea · Lionz01 · Apr 21, 2024
1 parent 61c9a47
commit 63895ea
Show file tree

Hide file tree

Showing 7 changed files with 107 additions and 10 deletions.
diff --git a/changelogs/current.yaml b/changelogs/current.yaml
@@ -48,6 +48,10 @@ bug_fixes:
 - area: tracing
  change: |
  Prevent Envoy from crashing at start up when the OpenTelemetry environment resource detector cannot detect any attributes.
+- area: proxy protocol
+ change: |
+ Fixed a crash when Envoy is configured for PROXY protocol on both a listener and cluster, and the listener receives
+ a PROXY protocol header with address type LOCAL (typically used for health checks).
 
 removed_config_or_runtime:
 # *Normally occurs at the end of the* :ref:`deprecation period <deprecated>`

diff --git a/source/extensions/common/proxy_protocol/proxy_protocol_header.cc b/source/extensions/common/proxy_protocol/proxy_protocol_header.cc
@@ -127,6 +127,15 @@ bool generateV2Header(const Network::ProxyProtocolData& proxy_proto_data, Buffer
  }
 
  ASSERT(extension_length <= std::numeric_limits<uint16_t>::max());
+ if (proxy_proto_data.src_addr_ == nullptr || proxy_proto_data.src_addr_->ip() == nullptr) {
+ IS_ENVOY_BUG("Missing or incorrect source IP in proxy_proto_data_");
+ return false;
+ }
+ if (proxy_proto_data.dst_addr_ == nullptr || proxy_proto_data.dst_addr_->ip() == nullptr) {
+ IS_ENVOY_BUG("Missing or incorrect dest IP in proxy_proto_data_");
+ return false;
+ }
+
  const auto& src = *proxy_proto_data.src_addr_->ip();
  const auto& dst = *proxy_proto_data.dst_addr_->ip();
  generateV2Header(src.addressAsString(), dst.addressAsString(), src.port(), dst.port(),

diff --git a/source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc b/source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc
@@ -144,24 +144,37 @@ ReadOrParseState Filter::parseBuffer(Network::ListenerFilterBuffer& buffer) {
  if (proxy_protocol_header_.has_value() &&
  !cb_->filterState().hasData<Network::ProxyProtocolFilterState>(
  Network::ProxyProtocolFilterState::key())) {
- if (!proxy_protocol_header_.value().local_command_) {
- auto buf = reinterpret_cast<const uint8_t*>(buffer.rawSlice().mem_);
+ auto buf = reinterpret_cast<const uint8_t*>(buffer.rawSlice().mem_);
+ if (proxy_protocol_header_.value().local_command_) {
+ ENVOY_LOG(trace, "Parsed proxy protocol header, cmd: LOCAL, length: {}, buffer: {}",
+ proxy_protocol_header_.value().wholeHeaderLength(),
+ Envoy::Hex::encode(buf, proxy_protocol_header_.value().wholeHeaderLength()));
+
+ cb_->filterState().setData(
+ Network::ProxyProtocolFilterState::key(),
+ std::make_unique<Network::ProxyProtocolFilterState>(Network::ProxyProtocolData{
+ socket.connectionInfoProvider().remoteAddress(),
+ socket.connectionInfoProvider().localAddress(), parsed_tlvs_}),
+ StreamInfo::FilterState::StateType::Mutable,
+ StreamInfo::FilterState::LifeSpan::Connection);
+ } else {
  ENVOY_LOG(
  trace,
- "Parsed proxy protocol header, length: {}, buffer: {}, TLV length: {}, TLV buffer: {}",
+ "Parsed proxy protocol header, cmd: PROXY, length: {}, buffer: {}, TLV length: {}, TLV "
+ "buffer: {}",
  proxy_protocol_header_.value().wholeHeaderLength(),
  Envoy::Hex::encode(buf, proxy_protocol_header_.value().wholeHeaderLength()),
  proxy_protocol_header_.value().extensions_length_,
  Envoy::Hex::encode(buf + proxy_protocol_header_.value().headerLengthWithoutExtension(),
  proxy_protocol_header_.value().extensions_length_));
+ cb_->filterState().setData(
+ Network::ProxyProtocolFilterState::key(),
+ std::make_unique<Network::ProxyProtocolFilterState>(Network::ProxyProtocolData{
+ proxy_protocol_header_.value().remote_address_,
+ proxy_protocol_header_.value().local_address_, parsed_tlvs_}),
+ StreamInfo::FilterState::StateType::Mutable,
+ StreamInfo::FilterState::LifeSpan::Connection);
  }
-
- cb_->filterState().setData(
- Network::ProxyProtocolFilterState::key(),
- std::make_unique<Network::ProxyProtocolFilterState>(Network::ProxyProtocolData{
- proxy_protocol_header_.value().remote_address_,
- proxy_protocol_header_.value().local_address_, parsed_tlvs_}),
- StreamInfo::FilterState::StateType::Mutable, StreamInfo::FilterState::LifeSpan::Connection);
  }
 
  if (proxy_protocol_header_.has_value() && !proxy_protocol_header_.value().local_command_) {

diff --git a/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc b/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc
@@ -626,6 +626,34 @@ TEST_P(ProxyProtocolTest, V2LocalConnectionExtension) {
  disconnect();
 }
 
+TEST_P(ProxyProtocolTest, V2LocalConnectionFilterState) {
+ // A well-formed local proxy protocol v2 header sampled from an AWS NLB healthcheck request,
+ // no address, 1 TLV is present.
+ constexpr uint8_t buffer[] = {0x0d, 0x0a, 0x0d, 0x0a, 0x00, 0x0d, 0x0a, 0x51, 0x55, 0x49, 0x54,
+ 0x0a, 0x20, 0x00, 0x00, 0x07, 0x00, 0x00, 0x04, 0x0a, 0x0b, 0x0c,
+ 0x0d, 'm', 'o', 'r', 'e', 'd', 'a', 't', 'a'};
+ envoy::extensions::filters::listener::proxy_protocol::v3::ProxyProtocol proto_config;
+ connect(true, &proto_config);
+ write(buffer, sizeof(buffer));
+ expectData("moredata");
+
+ auto& filter_state = server_connection_->streamInfo().filterState();
+ const auto& proxy_proto_data = filter_state
+ ->getDataReadOnly<Network::ProxyProtocolFilterState>(
+ Network::ProxyProtocolFilterState::key())
+ ->value();
+
+ if (server_connection_->connectionInfoProvider().remoteAddress()->ip()->version() ==
+ Envoy::Network::Address::IpVersion::v6) {
+ EXPECT_EQ(proxy_proto_data.dst_addr_->ip()->addressAsString(), "::1");
+ } else if (server_connection_->connectionInfoProvider().remoteAddress()->ip()->version() ==
+ Envoy::Network::Address::IpVersion::v4) {
+ EXPECT_EQ(proxy_proto_data.dst_addr_->ip()->addressAsString(), "127.0.0.1");
+ }
+ EXPECT_FALSE(server_connection_->connectionInfoProvider().localAddressRestored());
+ disconnect();
+}
+
 TEST_P(ProxyProtocolTest, V2ShortV4) {
  // An ipv4/tcp connection that has incorrect addr-len encoded
  constexpr uint8_t buffer[] = {0x0d, 0x0a, 0x0d, 0x0a, 0x00, 0x0d, 0x0a, 0x51, 0x55, 0x49,

diff --git a/test/extensions/transport_sockets/proxy_protocol/proxy_protocol_integration_test.cc b/test/extensions/transport_sockets/proxy_protocol/proxy_protocol_integration_test.cc
@@ -640,5 +640,46 @@ TEST_P(ProxyProtocolTLVsIntegrationTest, TestV2TLVProxyProtocolPassAll) {
  ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect());
 }
 
+TEST_P(ProxyProtocolTLVsIntegrationTest, TestV2ProxyProtocolPassWithTypeLocal) {
+ setup(true, {}, {});
+ initialize();
+
+ IntegrationTcpClientPtr tcp_client = makeTcpConnection(lookupPort("listener_0"));
+
+ // A well-formed proxy protocol v2 header sampled from an AWS NLB healthcheck request, with
+ // command type 'LOCAL' (0 for the low 4 bits of the 13th octet).
+ constexpr uint8_t v2_protocol[] = {0x0d, 0x0a, 0x0d, 0x0a, 0x00, 0x0d, 0x0a, 0x51,
+ 0x55, 0x49, 0x54, 0x0a, 0x20, 0x00, 0x00, 0x00,
+ 'm', 'o', 'r', 'e', 'd', 'a', 't', 'a'};
+ Buffer::OwnedImpl buffer(v2_protocol, sizeof(v2_protocol));
+ ASSERT_TRUE(tcp_client->write(buffer.toString()));
+ ASSERT_TRUE(fake_upstreams_[0]->waitForRawConnection(fake_upstream_connection_));
+ std::string header_start;
+ // - signature
+ // - version and command type, address family and protocol, length of addresses
+ // - src address, dest address
+ if (GetParam() == Envoy::Network::Address::IpVersion::v4) {
+ const char data[] = {0x0d, 0x0a, 0x0d, 0x0a, 0x00, 0x0d, 0x0a, 0x51, 0x55, 0x49, 0x54, 0x0a,
+ 0x21, 0x11, 0x00, 0x0c, 0x7f, 0x00, 0x00, 0x01, 0x7f, 0x00, 0x00, 0x01};
+ header_start = std::string(data, sizeof(data));
+ } else {
+ const char data[] = {0x0d, 0x0a, 0x0d, 0x0a, 0x00, 0x0d, 0x0a, 0x51, 0x55, 0x49, 0x54, 0x0a,
+ 0x21, 0x21, 0x00, 0x24, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01};
+ header_start = std::string(data, sizeof(data));
+ }
+
+ constexpr absl::string_view more_data("moredata");
+ const size_t offset = header_start.length() + (2 * sizeof(uint16_t)); // Skip over the ports
+ std::string observed_data;
+ ASSERT_TRUE(fake_upstream_connection_->waitForData(offset + more_data.length(), &observed_data));
+ EXPECT_THAT(observed_data, testing::StartsWith(header_start));
+ EXPECT_EQ(more_data, absl::string_view(&observed_data[offset], more_data.length()));
+
+ tcp_client->close();
+ ASSERT_TRUE(fake_upstream_connection_->waitForDisconnect());
+}
+
 } // namespace
 } // namespace Envoy
diff --git a/test/per_file_coverage.sh b/test/per_file_coverage.sh
@@ -24,6 +24,7 @@ declare -a KNOWN_LOW_COVERAGE=(
 "source/exe:90.3"
 "source/extensions/clusters/common:91.5" # This can be increased again once `#24903` lands
 "source/extensions/common:93.0" #flaky: be careful adjusting
+"source/extensions/common/proxy_protocol:93.8" # Adjusted for security patch
 "source/extensions/common/tap:94.5"
 "source/extensions/common/wasm:88.0" # flaky: be careful adjusting
 "source/extensions/common/wasm/ext:92.0"

diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt
@@ -993,6 +993,7 @@ netblocks
 netfilter
 netlink
 netmask
+NLB
 NLMSG
 nonblocking
 noncopyable