From 5725a54a18083101a03abf70b54c8b29130376b3 Mon Sep 17 00:00:00 2001 From: Joshua Rogers Date: Thu, 23 Oct 2025 10:11:27 +0000 Subject: [PATCH] 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. --- src/icmp/Icmp4.cc | 42 ++++++++++++++++++++++++++++++++++++------ src/icmp/Icmp6.cc | 35 ++++++++++++++++++++++++----------- src/icmp/IcmpPinger.cc | 11 ++++++----- src/icmp/IcmpSquid.cc | 34 +++++++++++++++++++++++++++++++--- src/icmp/net_db.cc | 10 +++++----- src/icmp/pinger.cc | 31 +++++++++++++++++++++++++++++++ 6 files changed, 133 insertions(+), 30 deletions(-) diff --git a/src/icmp/Icmp4.cc b/src/icmp/Icmp4.cc index a753a17e99..3a268c819c 100644 --- 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, ¤t_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(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(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(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 77e2843267..266f47266e 100644 --- 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(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(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(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 de5b05072d..24df9d6469 100644 --- 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 8d1c94f42a..90a6cf6188 100644 --- 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(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(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 6828404519..c49f85b066 100644 --- 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 1a3f1c0df6..a1cf29f632 100644 --- 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; -- 2.47.3