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.

Author: MegaManSec

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,15 +269,18 @@ 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.");
         Ip::Address::FreeAddr(from);
         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);

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,

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 */

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);

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) {

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,13 +96,43 @@ 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.
  */
 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;