Opened 3 years ago
Closed 3 years ago
#1303 closed Bug / Defect (fixed)
stack buffer overruns from NEXTADDR() macro
Reported by: | mandree | Owned by: | Gert Döring |
---|---|---|---|
Priority: | critical | Milestone: | release 2.5 |
Component: | Generic / unclassified | Version: | OpenVPN git master branch (Community Ed) |
Severity: | Not set (select this one, unless your'e a OpenVPN developer) | Keywords: | |
Cc: |
Description
This is with clang 10 on FreeBSD 12.1-RELEASE-p7 amd64 and -fsanitize=address, as of Git dfb40edc
[mandree@freeryzen ~/openvpn-master-unpacked-build]$ src/openvpn/openvpn --show-gateway ================================================================= ==38618==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fffffffb91c at pc 0x00000032a48a bp 0x7fffffffb5b0 sp 0x7fffffffad78 READ of size 32 at 0x7fffffffb91c thread T0 #0 0x32a489 in __asan_memcpy /wrkdirs/usr/ports/devel/llvm10/work/compiler-rt-10.0.0.src/lib/asan/asan_interceptors_memintrinsics.cpp:22:3 #1 0x41a361 in get_default_gateway_ipv6 /usr/home/mandree/openvpn-master-unpacked-build/src/openvpn/../../../openvpn-master-unpacked/src/openvpn/route.c:3718:5 #2 0x3d875f in add_option /usr/home/mandree/openvpn-master-unpacked-build/src/openvpn/../../../openvpn-master-unpacked/src/openvpn/options.c:5228:9 #3 0x3d4fae in parse_argv /usr/home/mandree/openvpn-master-unpacked-build/src/openvpn/../../../openvpn-master-unpacked/src/openvpn/options.c:4881:13 #4 0x3cab05 in openvpn_main /usr/home/mandree/openvpn-master-unpacked-build/src/openvpn/../../../openvpn-master-unpacked/src/openvpn/openvpn.c:187:13 #5 0x3ca8a8 in main /usr/home/mandree/openvpn-master-unpacked-build/src/openvpn/../../../openvpn-master-unpacked/src/openvpn/openvpn.c:364:12 #6 0x2d2a7e in _start /usr/src/lib/csu/amd64/crt1.c:76:7 Address 0x7fffffffb91c is located in stack of thread T0 at offset 860 in frame #0 0x41a02f in get_default_gateway_ipv6 /usr/home/mandree/openvpn-master-unpacked-build/src/openvpn/../../../openvpn-master-unpacked/src/openvpn/route.c:3669 This frame has 3 object(s): [32, 696) 'm_rtmsg' (line 3671) [832, 860) 'so_dst' (line 3675) <== Memory access at offset 860 overflows this variable [896, 924) 'so_mask' (line 3675) HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork (longjmp and C++ exceptions *are* supported) SUMMARY: AddressSanitizer: stack-buffer-overflow /wrkdirs/usr/ports/devel/llvm10/work/compiler-rt-10.0.0.src/lib/asan/asan_interceptors_memintrinsics.cpp:22:3 in __asan_memcpy Shadow bytes around the buggy address: 0x4ffffffff6d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x4ffffffff6e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x4ffffffff6f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x4ffffffff700: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 f2 0x4ffffffff710: f2 f2 f2 f2 f2 f2 f2 f2 f2 f2 f2 f2 f2 f2 f2 f2 =>0x4ffffffff720: 00 00 00[04]f2 f2 f2 f2 00 00 00 04 f3 f3 f3 f3 0x4ffffffff730: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x4ffffffff740: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 00 00 f2 f2 0x4ffffffff750: 00 00 00 00 00 00 00 00 00 00 00 00 00 f2 f2 f2 0x4ffffffff760: f2 f2 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x4ffffffff770: 00 00 00 00 00 00 00 00 00 00 00 00 04 f2 f2 f2 Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb Shadow gap: cc ==38618==ABORTING
gcc 10.1.0 (and also 9.3.0) on the same FreeBSD complain(s) like this:
../../../openvpn-master-unpacked/src/openvpn/route.c: In function 'get_default_gateway_ipv6': ../../../openvpn-master-unpacked/src/openvpn/route.c:3439:58: warning: '__builtin_memcpy' forming offset [28, 31] is out of the bounds [0, 28] of object 'so_dst' with type 'struct sockaddr_in6' [-Warray-bounds] 3439 | l = ROUNDUP( ((struct sockaddr *)&(u))->sa_len); memmove(cp, &(u), l); cp += l; \ | ^~~~~~~~~~~~~~~~~~~~ ../../../openvpn-master-unpacked/src/openvpn/route.c:3718:5: note: in expansion of macro 'NEXTADDR' 3718 | NEXTADDR(RTA_DST, so_dst); | ^~~~~~~~ ../../../openvpn-master-unpacked/src/openvpn/route.c:3675:25: note: 'so_dst' declared here 3675 | struct sockaddr_in6 so_dst, so_mask; | ^~~~~~ ../../../openvpn-master-unpacked/src/openvpn/route.c:3439:58: warning: '__builtin_memcpy' forming offset [28, 31] is out of the bounds [0, 28] of object 'so_dst' with type 'struct sockaddr_in6' [-Warray-bounds] 3439 | l = ROUNDUP( ((struct sockaddr *)&(u))->sa_len); memmove(cp, &(u), l); cp += l; \ | ^~~~~~~~~~~~~~~~~~~~ ../../../openvpn-master-unpacked/src/openvpn/route.c:3718:5: note: in expansion of macro 'NEXTADDR' 3718 | NEXTADDR(RTA_DST, so_dst); | ^~~~~~~~ ../../../openvpn-master-unpacked/src/openvpn/route.c:3675:25: note: 'so_dst' declared here 3675 | struct sockaddr_in6 so_dst, so_mask; | ^~~~~~ ../../../openvpn-master-unpacked/src/openvpn/route.c:3439:58: warning: '__builtin_memcpy' forming offset [28, 31] is out of the bounds [0, 28] of object 'so_mask' with type 'struct sockaddr_in6' [-Warray-bounds] 3439 | l = ROUNDUP( ((struct sockaddr *)&(u))->sa_len); memmove(cp, &(u), l); cp += l; \ | ^~~~~~~~~~~~~~~~~~~~ ../../../openvpn-master-unpacked/src/openvpn/route.c:3719:5: note: in expansion of macro 'NEXTADDR' 3719 | NEXTADDR(RTA_NETMASK, so_mask); | ^~~~~~~~ ../../../openvpn-master-unpacked/src/openvpn/route.c:3675:33: note: 'so_mask' declared here 3675 | struct sockaddr_in6 so_dst, so_mask; | ^~~~~~~
Attachments (1)
Change History (5)
comment:1 Changed 3 years ago by
Milestone: | → release 2.5 |
---|---|
Owner: | set to Gert Döring |
Status: | new → accepted |
Changed 3 years ago by
Attachment: | 0001-Fix-stack-buffer-overruns-in-NEXTADDR-macro.patch added |
---|
comment:3 Changed 3 years ago by
The patch looks good and should work across all platforms that use this (FreeBSD, NetBSD, OpenBSD, MacOS).
Not sure where I got the NEXTADDR() macro from originally, but your patch is very obviously correct (roundup the increment and not the number of bytes moved) and my code is very obviously not correct.
Thanks for looking into this.
Testing will take a few days with all the other patches flying back and forth right now.
comment:4 Changed 3 years ago by
Resolution: | → fixed |
---|---|
Status: | accepted → closed |
commit 5fde831c580775aa5c1fe3539b06260d994eee10 (master)
commit 7c428ca19a8df6b9630734eafce1132f457be951 (release/2.4)
Author: Matthias Andree
Date: Fri Jul 17 19:18:18 2020 +0200
Fix stack buffer overruns in NEXTADDR() macro:
Signed-off-by: Matthias Andree <matthias.andree@…>
Acked-by: Gert Doering <gert@…>
Message-Id: <20200717171818.230371-1-matthias.andree@…>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg20461.html
Signed-off-by: Gert Doering <gert@…>
The only platform where the resulting code doesn't work (among the ones I tested) is NetBSD 8.1 / i386, but the existence of #734 demonstrates that this is not a *new* problem. I assume alignment / word size in talking to the route socket, but haven't found time to investigate.
On FreeBSD, this works amd64 and i386, on MacOS it works amd64 and i386, so "it's not all wrong".
attempt to fix this bug by copying only the unrounded length of data before rounding up the skip pointer