[PATCH 0/1] MR9085: iphlpapi: Only read the part of dst that's valid.

[Yuxuan Shui (＠yshui)]

Although dst is a SOCKETADDR_INET *, the object it points to might be smaller than a full
SOCKETADDR_INET. One such example is GetBestInterface -> GetBestInterfaceEx -> GetBestRoute2, where
a socketaddr_in * (16 bytes) is casted to SOCKETADDR_INET * (28 bytes).

This means reading an full SOCKETADDR_INET out of dst could read out-of-bound.

Found by ASan.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/9085

[PATCH 1/1] iphlpapi: Only read the part of dst that's valid.

[Yuxuan Shui]

From: Yuxuan Shui <yshui(a)codeweavers.com>

Although dst is a SOCKETADDR_INET *, the object it points to might be smaller than a full
SOCKETADDR_INET. One such example is GetBestInterface -> GetBestInterfaceEx -> GetBestRoute2, where
a socketaddr_in * (16 bytes) is casted to SOCKETADDR_INET * (28 bytes).

This means reading an full SOCKETADDR_INET out of dst could read out-of-bound.

Found by ASan.
---
 dlls/iphlpapi/iphlpapi_main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/dlls/iphlpapi/iphlpapi_main.c b/dlls/iphlpapi/iphlpapi_main.c
index 83c6e8b53b8..db3d61974e9 100644
--- a/dlls/iphlpapi/iphlpapi_main.c
+++ b/dlls/iphlpapi/iphlpapi_main.c
@@ -4842,7 +4842,9 @@ DWORD WINAPI GetBestRoute2( NET_LUID *luid, NET_IFINDEX index,
 
         WSAStartup ( ver, &data );
         if ((s = socket( dst->si_family, SOCK_DGRAM, IPPROTO_UDP )) == -1) goto skip_system_route;
-        dst_addr = *dst;
+        if (v6) dst_addr.Ipv6 = dst->Ipv6;
+        else dst_addr.Ipv4 = dst->Ipv4;
+        dst_addr.si_family = dst->si_family;
         if (is_addr_unspecified( dst, v6 ))
         {
             /* GetBestRoute2() should return default route in this case while connect() while unspecified address for
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/9085

[Paul Gofman (＠gofman)]

I think the bug here is most likely in GetBestInterface() and should be fixed there (by using SOCKADDR_INET instead of struct sockaddr_in), and likely no bug in GetBestRoute2.

TL;DL; Note GetBestRoute2 even zeroes output bestaddress in full and that is backed by the tests. Whether GetBestRoute2 accesses src and dst in full or shorter part in case of ipv4 is currently not tested, but I think it more likely does on Windows (and if I didn't have to fixup dst address in the concerned place I would just do 'connect( s, (struct sockaddr *)dst, sizeof(*dst) )' and that access would happen in ws2_32 where it would copy the address for DeviceIoControl). That 'SOCKADDR_INET *' passed for dst, not a void pointer with explicit length, so from general sense it is totally valid to access the whole structure, and the implementation would naturally do it if it would, e. g., passing the address to nsiproxy or ws2_32 without interpreting here.

Small chance that it doesn't access it on Windows of course, but I'd suggest we don't overdo it by testing this implementation detail on Windows in the absense of practical motivation, avoid adding extra code to GetBestRoute2 and just use the SOCKADDR_INET structure in GetBestInterface().

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/9085#note_117328