Fix Content-Length buffer overflow and add regression tests

contrib/sip_proxy/filters/network/source/decoder.cc

@@ -1,5 +1,11 @@
 #include "contrib/sip_proxy/filters/network/source/decoder.h"
 
+#include <algorithm>
+#include <cstdint>
+
+#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<ssize_t>(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<size_t>(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<size_t>(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;
     }

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: <sip:User.0000@tas01.defult.svc.cluster.local>\x0d\x0a"
+      "From: <sip:User.0001@tas01.defult.svc.cluster.local>;tag=1\x0d\x0a"
+      "CSeq: 2 ACK\x0d\x0a"
+      "Contact: <sip:User.0001@11.0.0.10:15060;transport=TCP>;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: <sip:User.0000@tas01.defult.svc.cluster.local>\x0d\x0a"
+      "From: <sip:User.0001@tas01.defult.svc.cluster.local>;tag=1\x0d\x0a"
+      "CSeq: 2 ACK\x0d\x0a"
+      "Contact: <sip:User.0001@11.0.0.10:15060;transport=TCP>;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);