From: Joshua Rogers Date: Thu, 23 Oct 2025 10:11:27 +0000 (+0000) Subject: ICMP: Harden echo paths, fix overflows, UB, and leaks (#2199) X-Git-Tag: SQUID_7_4~16 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=5e1a2de66d881288f57330018c0ffe20481fc5bc;p=thirdparty%2Fsquid.git 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 889b3638b9..ed894dafd7 100644 --- 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, ¤t_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(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(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(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 ca94b1c7ea..9c18366ec2 100644 --- 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(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(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; @@ -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 d74ab735ec..e50b451c7b 100644 --- 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 8505b26111..88178aa151 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 "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(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 87dd7d5559..c52ec55742 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 126d84e827..d3b13e8e1c 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 "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;