From: Sergio Durigan Junior <github@sergiodj.net>
Date: Fri, 27 Aug 2021 10:37:50 +0000 (+0000)
Subject: Fix MAX_PKT{4,6}_SZ to account for icmpEchoData padding (#887)
X-Git-Tag: SQUID_6_0_1~291
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=2b9a2bba0f10e2aa9096883efc5b597a3a68bf5c;p=thirdparty%2Fsquid.git

Fix MAX_PKT{4,6}_SZ to account for icmpEchoData padding (#887)

The bug was exposed by GCC v11 on Ubuntu Impish:

    Icmp4.cc:116:11: error: array subscript icmpEchoData[0] is partly
        outside array bounds of char[282] [-Werror=array-bounds]
        echo->opcode = (unsigned char) opcode;

The array the compiler is talking about is the pkt buffer. That buffer
size (i.e. MAX_PKT4_SZ) was calculated under the faulty assumption that
a compiler cannot add padding after icmphdr (when doing "icmp+1") and/or
between icmpEchoData data members. When compiler padded, the old
MAX_PKT4_SZ math stopped working.

Same for ICMPv6.
---

diff --git a/CONTRIBUTORS b/CONTRIBUTORS
index e783fe13c1..e9c28fa048 100644
--- a/CONTRIBUTORS
+++ b/CONTRIBUTORS
@@ -415,6 +415,7 @@ Thank you!
     Sebastian Krahmer <krahmer@suse.com>
     Sebastien Wenske <sebastien@wenske.fr>
     Sergey Merzlikin <sm@smsoft.ru>
+    Sergio Durigan Junior <sergiodj@sergiodj.net>
     Sergio Rabellino <rabellino@di.unito.it>
     Shigechika Aikawa <shige@luck.imasy.or.jp>
     Silamael <Silamael@coronamundi.de>
diff --git a/src/icmp/Icmp.h b/src/icmp/Icmp.h
index dce63c21d1..ed187a0ef1 100644
--- a/src/icmp/Icmp.h
+++ b/src/icmp/Icmp.h
@@ -16,8 +16,8 @@
 #define PINGER_PAYLOAD_SZ   8192
 
 #define MAX_PAYLOAD 256 // WAS: SQUIDHOSTNAMELEN
-#define MAX_PKT4_SZ (MAX_PAYLOAD + sizeof(struct timeval) + sizeof (char) + sizeof(struct icmphdr) + 1)
-#define MAX_PKT6_SZ (MAX_PAYLOAD + sizeof(struct timeval) + sizeof (char) + sizeof(struct icmp6_hdr) + 1)
+#define MAX_PKT4_SZ (sizeof(struct icmphdr) + sizeof(struct icmpEchoData) + 1)
+#define MAX_PKT6_SZ (sizeof(struct icmp6_hdr) + sizeof(struct icmpEchoData) + 1)
 
 #if USE_ICMP
 
diff --git a/src/icmp/Icmp4.cc b/src/icmp/Icmp4.cc
index 25ff8a6fdf..df7a51506a 100644
--- a/src/icmp/Icmp4.cc
+++ b/src/icmp/Icmp4.cc
@@ -90,6 +90,8 @@ Icmp4::SendEcho(Ip::Address &to, int opcode, const char *payload, int len)
     size_t icmp_pktsize = sizeof(struct icmphdr);
     struct addrinfo *S = NULL;
 
+    static_assert(sizeof(*icmp) + sizeof(*echo) <= sizeof(pkt), "our custom ICMPv4 Echo payload fits the packet buffer");
+
     memset(pkt, '\0', MAX_PKT4_SZ);
 
     icmp = (struct icmphdr *) (void *) pkt;
@@ -111,7 +113,7 @@ Icmp4::SendEcho(Ip::Address &to, int opcode, const char *payload, int len)
     ++icmp_pkts_sent;
 
     // Construct ICMP packet data content
-    echo = (icmpEchoData *) (icmp + 1);
+    echo = reinterpret_cast<icmpEchoData *>(reinterpret_cast<char *>(pkt) + sizeof(*icmp));
     echo->opcode = (unsigned char) opcode;
     memcpy(&echo->tv, &current_time, sizeof(struct timeval));
 
diff --git a/src/icmp/Icmp6.cc b/src/icmp/Icmp6.cc
index c5fb004277..3c176bee20 100644
--- a/src/icmp/Icmp6.cc
+++ b/src/icmp/Icmp6.cc
@@ -124,6 +124,8 @@ Icmp6::SendEcho(Ip::Address &to, int opcode, const char *payload, int len)
     struct addrinfo *S = NULL;
     size_t icmp6_pktsize = 0;
 
+    static_assert(sizeof(*icmp) + sizeof(*echo) <= sizeof(pkt), "our custom ICMPv6 Echo payload fits the packet buffer");
+
     memset(pkt, '\0', MAX_PKT6_SZ);
     icmp = (struct icmp6_hdr *)pkt;
 
@@ -146,7 +148,7 @@ Icmp6::SendEcho(Ip::Address &to, int opcode, const char *payload, int len)
     icmp6_pktsize = sizeof(struct icmp6_hdr);
 
     // Fill Icmp6 ECHO data content
-    echo = (icmpEchoData *) (pkt + sizeof(icmp6_hdr));
+    echo = reinterpret_cast<icmpEchoData *>(reinterpret_cast<char *>(pkt) + sizeof(*icmp));
     echo->opcode = (unsigned char) opcode;
     memcpy(&echo->tv, &current_time, sizeof(struct timeval));