Signed-off-by: Stefan Dösinger stefan@codeweavers.com
---
This fixes random crashes in AllocateAndGetIpNetTableFromStack on MacOS Catalina. Netmask sockaddr_ins only have enough bytes to store the bits of the netmask that are non-zero, which are 0 for the default route - this leads to a 4 byte struct in the array.
As a result we read the gateway wrong on 64 bit builds. That was survivable at least as far as memory allocations go, but since Catalina there is one more struct sockaddr in the route info for some routes - RTA_IFA. Thanks to the wrong offset from the netmask we added a byte originating from the gateway's IP address to the RTA_IFA pointer and tried to read the address only to realize later that we don't know what to do anyway. Depending on the IP address and if the route in question is towards the end of the routing table we might read beyond the memory buffer.
A similar problem occurs with AF_LINK sockaddrs which have a size of 20 bytes, which we incorrectly aligned to 24.
See https://opensource.apple.com/source/network_cmds/network_cmds-596/netstat.tp.... --- dlls/iphlpapi/ipstats.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/iphlpapi/ipstats.c b/dlls/iphlpapi/ipstats.c index efb4d64c90..94fb4af5fa 100644 --- a/dlls/iphlpapi/ipstats.c +++ b/dlls/iphlpapi/ipstats.c @@ -145,7 +145,7 @@
#ifndef ROUNDUP #define ROUNDUP(a) \ - ((a) > 0 ? (1 + (((a) - 1) | (sizeof(long) - 1))) : sizeof(long)) + ((a) > 0 ? (1 + (((a) - 1) | (sizeof(uint32_t) - 1))) : sizeof(uint32_t)) #endif #ifndef ADVANCE #define ADVANCE(x, n) (x += ROUNDUP(((struct sockaddr *)n)->sa_len))
It should not happen any more after the previous patch but it is subtle enough that I think being verbose about it makes sense.
Signed-off-by: Stefan Dösinger stefan@codeweavers.com --- dlls/iphlpapi/ipstats.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/dlls/iphlpapi/ipstats.c b/dlls/iphlpapi/ipstats.c index 94fb4af5fa..fe4d3f4588 100644 --- a/dlls/iphlpapi/ipstats.c +++ b/dlls/iphlpapi/ipstats.c @@ -1539,6 +1539,12 @@ DWORD WINAPI AllocateAndGetIpForwardTableFromStack(PMIB_IPFORWARDTABLE *ppIpForw continue;
sa = (struct sockaddr *)addrPtr; + if (addrPtr + sa->sa_len > next + rtm->rtm_msglen) + { + ERR ("struct sockaddr extends beyond the route message, %p > %p\n", + addrPtr + sa->sa_len, next + rtm->rtm_msglen ); + } + ADVANCE (addrPtr, sa);
/* default routes are encoded by length-zero sockaddr */
Signed-off-by: Stefan Dösinger stefan@codeweavers.com
---
See the definition of p_sockaddr in https://opensource.apple.com/source/network_cmds/network_cmds-596/netstat.tp... and what it does with the mask parameter. --- dlls/iphlpapi/ipstats.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/dlls/iphlpapi/ipstats.c b/dlls/iphlpapi/ipstats.c index fe4d3f4588..38f5927cc0 100644 --- a/dlls/iphlpapi/ipstats.c +++ b/dlls/iphlpapi/ipstats.c @@ -1508,6 +1508,7 @@ DWORD WINAPI AllocateAndGetIpForwardTableFromStack(PMIB_IPFORWARDTABLE *ppIpForw for (next = buf; next < lim; next += rtm->rtm_msglen) { int i; + sa_family_t dst_family = AF_UNSPEC;
rtm = (struct rt_msghdr *)next;
@@ -1551,7 +1552,10 @@ DWORD WINAPI AllocateAndGetIpForwardTableFromStack(PMIB_IPFORWARDTABLE *ppIpForw if (sa->sa_len == 0) { addr = 0; }else { - switch(sa->sa_family) { + /* Apple's netstat prints the netmask together with the destination + * and only looks at the destination's address family. The netmask's + * sa_family sometimes contains the non-existent value 0xff. */ + switch(i == RTA_NETMASK ? dst_family : sa->sa_family) { case AF_INET: { struct sockaddr_in *sin = (struct sockaddr_in *)sa; addr = sin->sin_addr.s_addr; @@ -1575,7 +1579,10 @@ DWORD WINAPI AllocateAndGetIpForwardTableFromStack(PMIB_IPFORWARDTABLE *ppIpForw
switch (i) { - case RTA_DST: row.dwForwardDest = addr; break; + case RTA_DST: + row.dwForwardDest = addr; + dst_family = sa->sa_family; + break; case RTA_GATEWAY: row.dwForwardNextHop = addr; break; case RTA_NETMASK: row.dwForwardMask = addr; break; default:
Signed-off-by: Stefan Dösinger stefan@codeweavers.com
---
This patch is not absolutely necessary, but it allows us to drop the special handling of sa_len = 0. For any non-zero netmask the alignment makes sure we can read the full sin_addr, although there is no guarantee that the extra bytes contain zeroes. --- dlls/iphlpapi/ipstats.c | 52 +++++++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 25 deletions(-)
diff --git a/dlls/iphlpapi/ipstats.c b/dlls/iphlpapi/ipstats.c index 38f5927cc0..3d9718d1ec 100644 --- a/dlls/iphlpapi/ipstats.c +++ b/dlls/iphlpapi/ipstats.c @@ -1548,33 +1548,35 @@ DWORD WINAPI AllocateAndGetIpForwardTableFromStack(PMIB_IPFORWARDTABLE *ppIpForw
ADVANCE (addrPtr, sa);
- /* default routes are encoded by length-zero sockaddr */ - if (sa->sa_len == 0) { - addr = 0; - }else { - /* Apple's netstat prints the netmask together with the destination - * and only looks at the destination's address family. The netmask's - * sa_family sometimes contains the non-existent value 0xff. */ - switch(i == RTA_NETMASK ? dst_family : sa->sa_family) { - case AF_INET: { - struct sockaddr_in *sin = (struct sockaddr_in *)sa; - addr = sin->sin_addr.s_addr; - break; - } + /* Apple's netstat prints the netmask together with the destination + * and only looks at the destination's address family. The netmask's + * sa_family sometimes contains the non-existent value 0xff. */ + switch(i == RTA_NETMASK ? dst_family : sa->sa_family) { + case AF_INET: { + /* Netmasks (and possibly other addresses) have only enough size + * to represent the non-zero bits, e.g. a netmask of 255.0.0.0 has + * 56 bytes (1 sa_len, 1 sa_family, 2 sa_port and 1 for the first + * byte of sin_addr). Due to the alignment constraint we can de + * facto read the full 4 bytes of sin_addr (except for the case of + * netmask 0). Don't assume though that the extra bytes are zeroed. */ + struct sockaddr_in sin = {0}; + memcpy(&sin, sa, sa->sa_len); + addr = sin.sin_addr.s_addr; + break; + } #ifdef AF_LINK - case AF_LINK: - if(i == RTA_GATEWAY && row.u1.ForwardType == MIB_IPROUTE_TYPE_DIRECT) { - /* For direct route we may simply use dest addr as next hop */ - C_ASSERT(RTA_DST < RTA_GATEWAY); - addr = row.dwForwardDest; - break; - } - /* fallthrough */ -#endif - default: - WARN ("Received unsupported sockaddr family 0x%x\n", sa->sa_family); - addr = 0; + case AF_LINK: + if(i == RTA_GATEWAY && row.u1.ForwardType == MIB_IPROUTE_TYPE_DIRECT) { + /* For direct route we may simply use dest addr as next hop */ + C_ASSERT(RTA_DST < RTA_GATEWAY); + addr = row.dwForwardDest; + break; } + /* fallthrough */ +#endif + default: + WARN ("Received unsupported sockaddr family 0x%x\n", sa->sa_family); + addr = 0; }
switch (i)
Am 12.04.2020 um 14:24 schrieb Stefan Dösinger stefan@codeweavers.com:
* 56 bytes (1 sa_len, 1 sa_family, 2 sa_port and 1 for the first
That's a typo, it should be "5 bytes", I'll resend this patch after giving Jacek some change for feedback and/or accepting the other patches.
-
Stefan Dösinger