ICMP: Harden echo paths, fix overflows, UB, and leaks (#2199)

ICMPv4 send now validates getAddrInfo results before touching
ai_addr, avoiding null-deref, and frees the address on error.
Logging no longer passes nullptr strings, preventing UB.

ICMPv4 recv gained strict bounds checks: reject packets shorter
than IP/ICMP headers, reject bogus iphdrlen, and require enough
bytes for echo meta. Payload length is computed from real data,
clamped to MAX_PAYLOAD, and malformed negatives are dropped. The
result size calculation now uses PINGER_PAYLOAD_SZ instead of the
wrong MAX_PKT4_SZ, fixing excess memory disclosure.

ICMPv6 send added the same getAddrInfo validation and safe log
string handling. The recv path now checks for minimal header
lengths, validates echo meta, fixes ident comparison with ntohs,
and clamps payload length safely before copying into preply.

The pinger handshake no longer uses strlen() on raw buffers;
it now echoes exactly the bytes received, avoiding OOB reads.

Squid Recv was changed to treat pingerReplyData as variable-
length. It validates base header size, available payload, and
non-negative psize, and rejects truncated or oversized datagrams,
fixing past OOB read risks.

Finally, netdbBinaryExchange now flushes its 4K buffer before
writing the next record, fixing a possible overflow at the end of
the buffer.

diff --git a/src/icmp/Icmp4.cc b/src/icmp/Icmp4.cc
index a753a17e99373217bf8adcaf4396f6bd0031a5a9..3a268c819c84692d837e0adceaf7e5722ac745db 100644 (file)
--- a/src/icmp/Icmp4.cc
+++ b/src/icmp/Icmp4.cc
@@ -14,6 +14,7 @@
 
 #if USE_ICMP
 
+#include "base/Assure.h"
 #include "compat/socket.h"
 #include "debug/Stream.h"
 #include "Icmp4.h"
@@ -118,7 +119,7 @@ Icmp4::SendEcho(Ip::Address &to, int opcode, const char *payload, int len)
     echo->opcode = (unsigned char) opcode;
     memcpy(&echo->tv, &current_time, sizeof(struct timeval));
 
-    icmp_pktsize += sizeof(struct timeval) + sizeof(char);
+    icmp_pktsize += sizeof(icmpEchoData) - MAX_PAYLOAD;
 
     if (payload) {
         if (len > MAX_PAYLOAD)
@@ -131,7 +132,9 @@ Icmp4::SendEcho(Ip::Address &to, int opcode, const char *payload, int len)
 
     icmp->icmp_cksum = CheckSum((unsigned short *) icmp, icmp_pktsize);
 
-    to.getAddrInfo(S);
+    to.getAddrInfo(S, AF_INET);
+    Assure(S);
+
     ((sockaddr_in*)S->ai_addr)->sin_port = 0;
     assert(icmp_pktsize <= MAX_PKT4_SZ);
 
@@ -146,10 +149,10 @@ Icmp4::SendEcho(Ip::Address &to, int opcode, const char *payload, int len)
 
     if (x < 0) {
         int xerrno = errno;
-        debugs(42, DBG_IMPORTANT, MYNAME << "ERROR: sending to ICMP packet to " << to << ": " << xstrerr(xerrno));
+        debugs(42, DBG_IMPORTANT, "ERROR: sending ICMP packet to " << to << ": " << xstrerr(xerrno));
     }
 
-    Log(to, ' ', nullptr, 0, 0);
+    Log(to, ' ', "", 0, 0);
     Ip::Address::FreeAddr(S);
 }
 
@@ -203,6 +206,11 @@ Icmp4::Recv(void)
     debugs(42, 8, n << " bytes from " << preply.from);
 
     ip = (struct iphdr *) (void *) pkt;
+    if (n < static_cast<int>(sizeof(*ip))) {
+        debugs(42, 2, "short packet: only " << n << " bytes; expecting at least " << sizeof(*ip) << "-byte IP header");
+        Ip::Address::FreeAddr(from);
+        return;
+    }
 
 #if HAVE_STRUCT_IPHDR_IP_HL
 
@@ -220,7 +228,18 @@ Icmp4::Recv(void)
 #endif
 #endif /* HAVE_STRUCT_IPHDR_IP_HL */
 
+    if (iphdrlen < 20 || n < iphdrlen) {
+        debugs(42, 2, "bogus IP header length " << iphdrlen << " in " << n << "-byte packet");
+        Ip::Address::FreeAddr(from);
+        return;
+    }
     icmp = (struct icmphdr *) (void *) (pkt + iphdrlen);
+    const int icmpAvail = n - iphdrlen;
+    if (icmpAvail < static_cast<int>(sizeof(*icmp))) {
+        debugs(42, 2, "short ICMP header: only " << icmpAvail << " bytes available; expecting at least " << sizeof(*icmp));
+        Ip::Address::FreeAddr(from);
+        return;
+    }
 
     if (icmp->icmp_type != ICMP_ECHOREPLY) {
         Ip::Address::FreeAddr(from);
@@ -234,6 +253,14 @@ Icmp4::Recv(void)
 
     echo = (icmpEchoData *) (void *) (icmp + 1);
 
+    const auto echoHdr = static_cast<int>(sizeof(icmpEchoData) - MAX_PAYLOAD);
+    const auto icmpDataLen = icmpAvail - sizeof(*icmp);
+    if (icmpDataLen < echoHdr) { // do not read past end of the packet
+        debugs(42, 2, "short ICMP echo data: " << icmpDataLen << " bytes; expecting " << echoHdr);
+        Ip::Address::FreeAddr(from);
+        return;
+    }
+
     preply.opcode = echo->opcode;
 
     preply.hops = ipHops(ip->ip_ttl);
@@ -242,7 +269,10 @@ Icmp4::Recv(void)
     memcpy(&tv, &echo->tv, sizeof(struct timeval));
     preply.rtt = tvSubMsec(tv, now);
 
-    preply.psize = n - iphdrlen - (sizeof(icmpEchoData) - MAX_PKT4_SZ);
+    // Payload length = (ICMP total data) - (opcode + timeval)
+    preply.psize = icmpDataLen - echoHdr;
+    if (preply.psize > MAX_PAYLOAD)
+        preply.psize = MAX_PAYLOAD;
 
     if (preply.psize < 0) {
         debugs(42, DBG_CRITICAL, "ERROR: Malformed ICMP packet.");
@@ -250,7 +280,7 @@ Icmp4::Recv(void)
         return;
     }
 
-    control.SendResult(preply, (sizeof(pingerReplyData) - MAX_PKT4_SZ + preply.psize) );
+    control.SendResult(preply, (sizeof(pingerReplyData) - PINGER_PAYLOAD_SZ + preply.psize));
 
     Log(preply.from, icmp->icmp_type, IcmpPacketType(icmp->icmp_type), preply.rtt, preply.hops);
     Ip::Address::FreeAddr(from);

diff --git a/src/icmp/Icmp6.cc b/src/icmp/Icmp6.cc
index 77e2843267d17b283ba71a42dbc962abc66ad1aa..266f47266e73c4fe3a8a30330bb30e1e7732c0fa 100644 (file)
--- a/src/icmp/Icmp6.cc
+++ b/src/icmp/Icmp6.cc
@@ -14,6 +14,7 @@
 
 #if USE_ICMP
 
+#include "base/Assure.h"
 #include "compat/socket.h"
 #include "debug/Stream.h"
 #include "Icmp6.h"
@@ -166,7 +167,9 @@ Icmp6::SendEcho(Ip::Address &to, int opcode, const char *payload, int len)
 
     icmp->icmp6_cksum = CheckSum((unsigned short *) icmp, icmp6_pktsize);
 
-    to.getAddrInfo(S);
+    to.getAddrInfo(S, AF_INET6);
+    Assure(S);
+
     ((sockaddr_in6*)S->ai_addr)->sin6_port = 0;
 
     assert(icmp6_pktsize <= MAX_PKT6_SZ);
@@ -182,11 +185,11 @@ Icmp6::SendEcho(Ip::Address &to, int opcode, const char *payload, int len)
 
     if (x < 0) {
         int xerrno = errno;
-        debugs(42, DBG_IMPORTANT, MYNAME << "ERROR: sending to ICMPv6 packet to " << to << ": " << xstrerr(xerrno));
+        debugs(42, DBG_IMPORTANT, "ERROR: sending ICMPv6 packet to " << to << ": " << xstrerr(xerrno));
     }
     debugs(42,9, "x=" << x);
 
-    Log(to, 0, nullptr, 0, 0);
+    Log(to, 0, "", 0, 0);
     Ip::Address::FreeAddr(S);
 }
 
@@ -228,6 +231,10 @@ Icmp6::Recv(void)
         Ip::Address::FreeAddr(from);
         return;
     }
+    if (n < static_cast<int>(sizeof(struct icmp6_hdr))) {
+        Ip::Address::FreeAddr(from);
+        return;
+    }
 
     preply.from = *from;
 
@@ -289,13 +296,18 @@ Icmp6::Recv(void)
         return;
     }
 
-    if (icmp6header->icmp6_id != icmp_ident) {
+    if (ntohs(icmp6header->icmp6_id) != static_cast<uint16_t>(icmp_ident)) {
         debugs(42, 8, "dropping Icmp6 read. IDENT check failed. ident=='" << icmp_ident << "'=='" << icmp6header->icmp6_id << "'");
         Ip::Address::FreeAddr(from);
         return;
     }
 
-    echo = (icmpEchoData *) (pkt + sizeof(icmp6_hdr));
+    const auto meta = static_cast<int>(sizeof(struct icmp6_hdr) + sizeof(struct timeval) + sizeof(unsigned char));
+    if (n < meta) {
+        Ip::Address::FreeAddr(from);
+        return;
+    }
+    echo = (icmpEchoData *)(pkt + sizeof(struct icmp6_hdr));
 
     preply.opcode = echo->opcode;
 
@@ -311,13 +323,14 @@ Icmp6::Recv(void)
      */
     preply.hops = 1;
 
-    preply.psize = n - /* sizeof(ip6_hdr) - */ sizeof(icmp6_hdr) - (sizeof(icmpEchoData) - MAX_PKT6_SZ);
+    auto payload_len = n - meta;
+    assert(payload_len >= 0);
+    if (payload_len > MAX_PAYLOAD)
+        payload_len = MAX_PAYLOAD;
 
-    /* Ensure the response packet has safe payload size */
-    if ( preply.psize > (unsigned short) MAX_PKT6_SZ) {
-        preply.psize = MAX_PKT6_SZ;
-    } else if ( preply.psize < (unsigned short)0) {
-        preply.psize = 0;
+    preply.psize = payload_len;
+    if (preply.psize > 0) {
+        memcpy(preply.payload, echo->payload, preply.psize);
     }
 
     Log(preply.from,

diff --git a/src/icmp/IcmpPinger.cc b/src/icmp/IcmpPinger.cc
index de5b05072d2f77974b30fe9c0c506eff15ac57a0..24df9d6469ba80daebbe1677a90835e7215890c1 100644 (file)
--- a/src/icmp/IcmpPinger.cc
+++ b/src/icmp/IcmpPinger.cc
@@ -122,7 +122,7 @@ IcmpPinger::Open(void)
         return -1;
     }
 
-    x = xsend(icmp_sock, buf, strlen(buf), 0);
+    x = xsend(icmp_sock, buf, x, 0);
     xerrno = errno;
 
     if (x < 3 || strncmp("OK\n", buf, 3)) {
@@ -152,10 +152,11 @@ void
 IcmpPinger::Close(void)
 {
 #if _SQUID_WINDOWS_
-
-    shutdown(icmp_sock, SD_BOTH);
-    xclose(icmp_sock);
-    icmp_sock = -1;
+    if (icmp_sock >= 0) {
+        shutdown(icmp_sock, SD_BOTH);
+        xclose(icmp_sock);
+        icmp_sock = -1;
+    }
 #endif
 
     /* also shutdown the helper engines */

diff --git a/src/icmp/IcmpSquid.cc b/src/icmp/IcmpSquid.cc
index 8d1c94f42ada989dcd9bce61c8a511d87e94b444..90a6cf61885bb7954cdfbc281cc07c350d202710 100644 (file)
--- a/src/icmp/IcmpSquid.cc
+++ b/src/icmp/IcmpSquid.cc
@@ -9,6 +9,7 @@
 /* DEBUG: section 37    ICMP Routines */
 
 #include "squid.h"
+#include "base/Assure.h"
 #include "comm.h"
 #include "comm/Loops.h"
 #include "compat/socket.h"
@@ -71,9 +72,10 @@ IcmpSquid::SendEcho(Ip::Address &to, int opcode, const char *payload, int len)
     else if (payload && len == 0)
         len = strlen(payload);
 
-    // XXX: If length specified or auto-detected is greater than the possible payload squid will die with an assert.
-    // TODO: This should perhapse be reduced to a truncated payload? or no payload. A WARNING is due anyway.
-    assert(len <= PINGER_PAYLOAD_SZ);
+    // All our callers supply a DNS name. PINGER_PAYLOAD_SZ must accommodate the
+    // longest DNS name Squid supports. TODO: Simplify and improve the rest of
+    // this code accordingly.
+    Assure(len <= PINGER_PAYLOAD_SZ);
 
     pecho.to = to;
 
@@ -151,6 +153,32 @@ IcmpSquid::Recv()
         return;
     }
 
+    const auto base = static_cast<int>(sizeof(preply) - sizeof(preply.payload));
+    if (n < base) {
+        debugs(37, 2, "short reply header (" << n << " < " << base << "); dropping");
+        return;
+    }
+    const auto avail = n - base;
+    if (avail > static_cast<int>(sizeof(preply.payload))) {
+        debugs(37, 2, "oversized reply payload (" << avail << "); dropping");
+        return;
+    }
+    if (preply.psize < 0) {
+        debugs(37, 2, "negative psize (" << preply.psize << "); dropping");
+        return;
+    }
+    if (preply.psize > avail) {
+        debugs(37, 2, "truncated reply (psize=" << preply.psize << ", avail=" << avail << "); dropping");
+        return;
+    }
+    // Accept variable-length replies: base header + psize bytes.
+    // We already validated 'n >= base' and 'preply.psize <= avail'.
+    // If the datagram was truncated in transit, drop it.
+    if (n < (base + preply.psize)) {
+        debugs(37, 2, "truncated reply datagram; dropping");
+        return;
+    }
+
     F = preply.from;
 
     F.port(0);

diff --git a/src/icmp/net_db.cc b/src/icmp/net_db.cc
index 6828404519dbfbd02cf55686abcad0161d1e24af..c49f85b06647f43daa6530bafa3d82c0aa88db1e 100644 (file)
--- a/src/icmp/net_db.cc
+++ b/src/icmp/net_db.cc
@@ -1127,6 +1127,11 @@ netdbBinaryExchange(StoreEntry * s)
         if ( !addr.isIPv4() )
             continue;
 
+        if (i + rec_sz > 4096) {
+            s->append(buf, i);
+            i = 0;
+        }
+
         buf[i] = (char) NETDB_EX_NETWORK;
         ++i;
 
@@ -1152,11 +1157,6 @@ netdbBinaryExchange(StoreEntry * s)
         memcpy(&buf[i], &j, sizeof(int));
 
         i += sizeof(int);
-
-        if (i + rec_sz > 4096) {
-            s->append(buf, i);
-            i = 0;
-        }
     }
 
     if (i > 0) {

diff --git a/src/icmp/pinger.cc b/src/icmp/pinger.cc
index 1a3f1c0df6831c8184be9a62dfffdc6ce3432e58..a1cf29f632434260e6dde2f84f34a6fd8803b426 100644 (file)
--- a/src/icmp/pinger.cc
+++ b/src/icmp/pinger.cc
@@ -45,6 +45,7 @@
 #if USE_ICMP
 
 #include "base/Stopwatch.h"
+#include "base/TextException.h"
 #include "compat/select.h"
 #include "compat/socket.h"
 #include "Icmp4.h"
@@ -95,6 +96,33 @@ Icmp6 icmp6;
 
 int icmp_pkts_sent = 0;
 
+/// reports std::terminate() cause (e.g., an uncaught or prohibited exception)
+static std::ostream &
+TerminationReason(std::ostream &os)
+{
+    if (std::current_exception())
+        os << CurrentException;
+    else
+        os << "An undetermined failure";
+    return os;
+}
+
+static void
+OnTerminate()
+{
+    // ignore recursive calls to avoid termination loops
+    static bool terminating = false;
+    if (terminating)
+        return;
+    terminating = true;
+
+    debugs(1, DBG_CRITICAL, "FATAL: " << TerminationReason);
+
+    control.Close(); // TODO: Here and elsewhere, rely on IcmpPinger class destructor instead.
+    Debug::PrepareToDie();
+    abort();
+}
+
 /**
  \ingroup pinger
  \par This is the pinger external process.
@@ -102,6 +130,9 @@ int icmp_pkts_sent = 0;
 int
 main(int, char **)
 {
+    // TODO: Apply this try/catch-less approach to address SquidMainSafe() XXX.
+    (void)std::set_terminate(&OnTerminate);
+
     fd_set R;
     int max_fd = 0;