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 889b3638b90167ee18c9717ee2f1da17dd916e4b..ed894dafd7f55ac6dc0cb297c441b049db94a94c 100644 (file)
--- a/src/icmp/Icmp4.cc
+++ b/src/icmp/Icmp4.cc
@@ -14,6 +14,7 @@
 
 #if USE_ICMP
 
+#include "base/Assure.h"
 #include "debug/Stream.h"
 #include "Icmp4.h"
 #include "IcmpPinger.h"
@@ -117,7 +118,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)
@@ -130,7 +131,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);
 
@@ -145,10 +148,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);
 }
 
@@ -202,6 +205,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
 
@@ -219,7 +227,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);
@@ -233,6 +252,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);
@@ -241,7 +268,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.");
@@ -249,7 +279,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 ca94b1c7ea094ccbca9e2f9d35e19528cc7dc1b1..9c18366ec24d6a0094038fe3a984324051d129d4 100644 (file)
--- a/src/icmp/Icmp6.cc
+++ b/src/icmp/Icmp6.cc
@@ -14,6 +14,7 @@
 
 #if USE_ICMP
 
+#include "base/Assure.h"
 #include "debug/Stream.h"
 #include "Icmp6.h"
 #include "IcmpPinger.h"
@@ -165,7 +166,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);
@@ -181,11 +184,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);
 }
 
@@ -227,6 +230,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;
 
@@ -288,13 +295,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;
 
@@ -310,13 +322,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 d74ab735ec1d617c2893500a26e992efeb82fdd6..e50b451c7b3ac3a1341f73d703bb23755d39085d 100644 (file)
--- a/src/icmp/IcmpPinger.cc
+++ b/src/icmp/IcmpPinger.cc
@@ -150,10 +150,11 @@ void
 IcmpPinger::Close(void)
 {
 #if _SQUID_WINDOWS_
-
-    shutdown(icmp_sock, SD_BOTH);
-    close(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 8505b2611191a48f99f8f179d1241d0724aa827f..88178aa1511f3411d73b8b2977f233da5c2a04b8 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 "defines.h"
@@ -70,9 +71,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;
 
@@ -150,6 +152,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 87dd7d5559a21a04d85d047b8e06a9431793e2fe..c52ec5574223279d7b5bf1e8c2fdc5cde43ec774 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 126d84e827b92843a92fbcb00e4708ba2ad237bd..d3b13e8e1c2f220c4d2982639a8bdb7cef965b94 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 "Icmp4.h"
 #include "Icmp6.h"
 #include "IcmpPinger.h"
@@ -93,6 +94,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.
@@ -100,6 +128,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 x;
     int max_fd = 0;