CVE-2024-23327: NULL Pointer Dereference (CWE-476)
value: Envoy is a high-performance edge/middle/service proxy. When PPv2 is enabled both on a listener and subsequent cluster, the Envoy instance will segfault when attempting to craft the upstream PPv2 header. This occurs when the downstream request has a command type of LOCAL and does not have the protocol block. This issue has been addressed in releases 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: The product dereferences a pointer that it expects to be valid but is NULL.
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/common/proxy_protocol/proxy_protocol_header.cc, source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc, test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc, test/extensions/transport_sockets/proxy_protocol/proxy_protocol_integration_test.cc, test/per_file_coverage.sh, tools/spelling/spelling_dictionary.txt
Commit date: 2024-02-09 17:52:19 +0000
Commit message: Fix crash from AWS NLB healthchecks when proxy protocol is enabled

Fix: [CVE-2024-23327](https://github.com/envoyproxy/envoy/security/advisories/GHSA-4h5x-x9vh-m29j)

Signed-off-by: Jacob Neil Taylor <me@jacobtaylor.id.au>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Ryan Northey <ryan@synca.io>

Diff: diff --git a/changelogs/current.yaml b/changelogs/current.yaml
index 207bc6e90bd..fa11395e7d8 100644
--- a/changelogs/current.yaml
+++ b/changelogs/current.yaml
@@ -46,10 +46,14 @@ bug_fixes:
     ``propagate_response_headers`` and retry configurations are enabled at the same time, and an upstream
     request is retried multiple times.
 - 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>`
 - area: http
   change: |
diff --git a/source/extensions/common/proxy_protocol/proxy_protocol_header.cc b/source/extensions/common/proxy_protocol/proxy_protocol_header.cc
index 0bbe17dd6d9..1c8537964f7 100644
--- a/source/extensions/common/proxy_protocol/proxy_protocol_header.cc
+++ b/source/extensions/common/proxy_protocol/proxy_protocol_header.cc
@@ -125,10 +125,19 @@ bool generateV2Header(const Network::ProxyProtocolData& proxy_proto_data, Buffer
       return false;
     }
   }
 
   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(),
                    src.version(), static_cast<uint16_t>(extension_length), out);
 
diff --git a/source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc b/source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc
index ce4df5066bd..fea08b31262 100644
--- a/source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc
+++ b/source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc
@@ -142,28 +142,41 @@ 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_) {
     // If this is a local_command, we are not to override address
     // Error check the source and destination fields. Most errors are caught by the address
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 af16be2cff7..ca85d04218d 100644
--- a/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc
+++ b/test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc
@@ -624,10 +624,38 @@ TEST_P(ProxyProtocolTest, V2LocalConnectionExtension) {
   }
   EXPECT_FALSE(server_connection_->connectionInfoProvider().localAddressRestored());
   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,
                                 0x54, 0x0a, 0x21, 0x21, 0x00, 0x04, 0x00, 0x08, 0x00, 0x02,
                                 'm',  'o',  'r',  'e',  ' ',  'd',  'a',  't',  'a'};
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
index 24c44e27a73..72637a7acd0 100644
--- a/test/extensions/transport_sockets/proxy_protocol/proxy_protocol_integration_test.cc
+++ b/test/extensions/transport_sockets/proxy_protocol/proxy_protocol_integration_test.cc
@@ -638,7 +638,48 @@ TEST_P(ProxyProtocolTLVsIntegrationTest, TestV2TLVProxyProtocolPassAll) {
 
   tcp_client->close();
   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
index 72e58a95620..9759ec2c265 100755
--- a/test/per_file_coverage.sh
+++ b/test/per_file_coverage.sh
@@ -22,10 +22,11 @@ declare -a KNOWN_LOW_COVERAGE=(
 "source/common/thread:0.0" # Death tests don't report LCOV
 "source/common/watchdog:58.6" # Death tests don't report LCOV
 "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"
 "source/extensions/filters/common/fault:94.5"
 "source/extensions/filters/common/rbac:90.5"
diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt
index 9de34d2bdb5..c0016586cee 100644
--- a/tools/spelling/spelling_dictionary.txt
+++ b/tools/spelling/spelling_dictionary.txt
@@ -991,10 +991,11 @@ negative
 netblock
 netblocks
 netfilter
 netlink
 netmask
+NLB
 NLMSG
 nonblocking
 noncopyable
 nonresponsive
 noop

Vulnerable code:     if (!proxy_protocol_header_.value().local_command_) {
      auto buf = reinterpret_cast<const uint8_t*>(buffer.rawSlice().mem_);
          "Parsed proxy protocol header, length: {}, buffer: {}, TLV length: {}, TLV buffer: {}",

    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);
Fixed code: - 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).
  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;
  }

    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 {
          "Parsed proxy protocol header, cmd: PROXY, length: {}, buffer: {}, TLV length: {}, TLV "
          "buffer: {}",
      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);
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(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());
}

"source/extensions/common/proxy_protocol:93.8" # Adjusted for security patch
NLB