stack buffer overruns from NEXTADDR() macro

[mandree]

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;
      |                                 ^~~~~~~

[mandree]

attempt to fix this bug by copying only the unrounded length of data before rounding up the skip pointer

[mandree]

Note this needs to be backported to 2.4.9.

[Gert Döring]

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.

[Gert Döring]

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".