Skip to content

Commit

Permalink
Fix crash from AWS NLB healthchecks when proxy protocol is enabled
Browse files Browse the repository at this point in the history
Fix: [CVE-2024-23327](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>
  • Loading branch information
jacobneiltaylor authored and phlax committed Feb 9, 2024
1 parent 61c9a47 commit 63895ea
Show file tree
Hide file tree
Showing 7 changed files with 107 additions and 10 deletions.
4 changes: 4 additions & 0 deletions changelogs/current.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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>`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
33 changes: 23 additions & 10 deletions source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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_) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
1 change: 1 addition & 0 deletions test/per_file_coverage.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
1 change: 1 addition & 0 deletions tools/spelling/spelling_dictionary.txt
Original file line number Diff line number Diff line change
Expand Up @@ -993,6 +993,7 @@ netblocks
netfilter
netlink
netmask
NLB
NLMSG
nonblocking
noncopyable
Expand Down

1 comment on commit 63895ea

@Lionz01
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


Please sign in to comment.