From eb9bd8a158f2122d2886f29784babfce1c2f9628 Mon Sep 17 00:00:00 2001 From: Lucy Date: Mon, 1 Jun 2026 13:05:46 -0500 Subject: [PATCH] sip_proxy: fix Content-Length stack buffer overflow in reassemble() - Add content_length_end == -1 guard to prevent length underflow - Add value_len <= 0 guard to reject malformed headers - Clamp copyOut to min(value_len, sizeof(len)-1) to prevent overflow - Replace std::atoi with absl::SimpleAtoi into uint32_t to reject negative, non-numeric, and out-of-range values - Add regression tests: DecodeOverlongContentLengthDoesNotOverflow, DecodeNegativeContentLengthIsRejected Signed-off-by: Lucy --- .../filters/network/source/decoder.cc | 67 +++++++++++++++---- .../filters/network/test/decoder_test.cc | 52 ++++++++++++++ 2 files changed, 105 insertions(+), 14 deletions(-) diff --git a/contrib/sip_proxy/filters/network/source/decoder.cc b/contrib/sip_proxy/filters/network/source/decoder.cc index f264405c85d0c..d94f5e40a28c9 100644 --- a/contrib/sip_proxy/filters/network/source/decoder.cc +++ b/contrib/sip_proxy/filters/network/source/decoder.cc @@ -1,5 +1,11 @@ #include "contrib/sip_proxy/filters/network/source/decoder.h" +#include +#include + +#include "absl/strings/ascii.h" +#include "absl/strings/numbers.h" + namespace Envoy { namespace Extensions { namespace NetworkFilters { @@ -112,22 +118,55 @@ int Decoder::reassemble(Buffer::Instance& data) { ssize_t content_length_end = remaining_data.search( "\r\n", strlen("\r\n"), content_length_start + strlen("Content-Length:"), content_pos); - // The "\n\r\n" is always included in remaining_data, so could not return -1 - // if (content_length_end == -1) { - // break; - // } - - char len[10]{}; // temporary storage - remaining_data.copyOut(content_length_start + strlen("Content-Length:"), - content_length_end - content_length_start - strlen("Content-Length:"), - len); + // Security: the trailing "\r\n" of the Content-Length header is NOT + // guaranteed to exist within the searched window for a malformed or + // truncated message (the previous code wrongly assumed it always did and + // removed this check). If it is missing, search() returns -1 and the + // length arithmetic below would underflow into a huge unsigned value that + // is then handed to copyOut(). Bail out and wait for more data instead. + if (content_length_end == -1) { + break; + } - clen = std::atoi(len); + // Length of the Content-Length header value (may include surrounding + // whitespace). This span is fully attacker controlled. + const ssize_t value_len = content_length_end - content_length_start - + static_cast(strlen("Content-Length:")); + if (value_len <= 0) { + // Malformed header such as "Content-Length:\r\n"; skip the message. + break; + } - // atoi return value >= 0, could not < 0 - // if (clen < static_cast(0)) { - // break; - // } + // Security fix (CWE-121 stack buffer overflow / CWE-787 out-of-bounds + // write): Buffer::copyOut() performs a raw memcpy bounded only by the + // *source* buffer length, never by the destination. The previous code + // passed the full, attacker-controlled `value_len` as the copy size into a + // fixed 10-byte stack buffer, so a Content-Length whose value was longer + // than the buffer (e.g. "Content-Length: 99999999999999999999") wrote + // attacker-controlled bytes past `len` on the stack. That is a remote + // stack-write primitive and a reliable DoS (stack-smashing detection + // aborts the process). Clamp the copy to the destination size and always + // keep room for the NUL terminator. A SIP Content-Length is only ever a + // handful of digits, so clamping cannot turn a legitimate value into a + // smaller-but-valid number (an over-long value is rejected by the parse + // below as out of range). + char len[16] = {}; // zero-initialized, so always NUL terminated after copy + const size_t copy_len = std::min(static_cast(value_len), sizeof(len) - 1); + remaining_data.copyOut(content_length_start + strlen("Content-Length:"), copy_len, len); + + // Parse defensively. std::atoi() has undefined behaviour on overflow and + // silently turns a negative value (e.g. "Content-Length: -1") into an + // enormous size_t, corrupting the full-message-length computation below. + // absl::SimpleAtoi() rejects non-numeric, negative and out-of-range input, + // returning false instead. + uint32_t parsed_clen = 0; + if (!absl::SimpleAtoi(absl::StripAsciiWhitespace(absl::string_view(len, copy_len)), + &parsed_clen)) { + // Non-numeric, negative or overflowing Content-Length: drop the message + // rather than trusting an attacker-supplied length. + break; + } + clen = parsed_clen; full_msg_len = content_pos + clen; } diff --git a/contrib/sip_proxy/filters/network/test/decoder_test.cc b/contrib/sip_proxy/filters/network/test/decoder_test.cc index 32eb2a1f7ae5a..bd69ee066d1c6 100644 --- a/contrib/sip_proxy/filters/network/test/decoder_test.cc +++ b/contrib/sip_proxy/filters/network/test/decoder_test.cc @@ -183,6 +183,58 @@ TEST_F(SipDecoderTest, DecodeINVITE) { EXPECT_EQ(0U, store_.counter("test.response").value()); } +// Regression test for a stack buffer overflow (out-of-bounds write primitive) +// and reliable remote DoS in Decoder::reassemble(). A Content-Length header +// whose value was longer than the fixed parse buffer used to be copied into +// that buffer verbatim via Buffer::copyOut(), which memcpys based on the source +// length only -- smashing the stack with attacker-controlled bytes. The decoder +// must now reject the oversized/out-of-range value without crashing and without +// dispatching the message. Under ASAN this test aborts on the unfixed code. +TEST_F(SipDecoderTest, DecodeOverlongContentLengthDoesNotOverflow) { + initializeFilter(yaml); + + const std::string SIP_OVERLONG_CONTENT_LENGTH = + "ACK sip:User.0000@tas01.defult.svc.cluster.local SIP/2.0\x0d\x0a" + "Call-ID: 1-3193@11.0.0.10\x0d\x0a" + "Via: SIP/2.0/TCP 11.0.0.10:15060;branch=z9hG4bK-3193-1-0\x0d\x0a" + "To: \x0d\x0a" + "From: ;tag=1\x0d\x0a" + "CSeq: 2 ACK\x0d\x0a" + "Contact: ;tag=1\x0d\x0a" + "Max-Forwards: 70\x0d\x0a" + "Content-Length: 999999999999999999999999999999\x0d\x0a" + "\x0d\x0a"; + buffer_.add(SIP_OVERLONG_CONTENT_LENGTH); + + // Must not crash (previously a stack-smashing write) and must not dispatch the + // malformed message downstream. + EXPECT_EQ(filter_->onData(buffer_, false), Network::FilterStatus::StopIteration); + EXPECT_EQ(0U, store_.counter("test.request").value()); +} + +// A negative Content-Length must likewise be rejected. std::atoi() previously +// returned -1, which silently became a huge size_t and corrupted the +// full-message-length computation. absl::SimpleAtoi() rejects it instead. +TEST_F(SipDecoderTest, DecodeNegativeContentLengthIsRejected) { + initializeFilter(yaml); + + const std::string SIP_NEGATIVE_CONTENT_LENGTH = + "ACK sip:User.0000@tas01.defult.svc.cluster.local SIP/2.0\x0d\x0a" + "Call-ID: 1-3193@11.0.0.10\x0d\x0a" + "Via: SIP/2.0/TCP 11.0.0.10:15060;branch=z9hG4bK-3193-1-0\x0d\x0a" + "To: \x0d\x0a" + "From: ;tag=1\x0d\x0a" + "CSeq: 2 ACK\x0d\x0a" + "Contact: ;tag=1\x0d\x0a" + "Max-Forwards: 70\x0d\x0a" + "Content-Length: -1\x0d\x0a" + "\x0d\x0a"; + buffer_.add(SIP_NEGATIVE_CONTENT_LENGTH); + + EXPECT_EQ(filter_->onData(buffer_, false), Network::FilterStatus::StopIteration); + EXPECT_EQ(0U, store_.counter("test.request").value()); +} + TEST_F(SipDecoderTest, DecodeRegister) { initializeFilter(yaml);