| Message ID | 20200717171818.230371-1-matthias.andree@gmx.de |
|---|---|
| State | Accepted |
| Delegated to: | Gert Doering |
| Headers | show |
| Series | [Openvpn-devel] Fix stack buffer overruns in NEXTADDR() macro: | expand |
Your patch has been applied to the master and release/2.4 branch
(bugfix).
Thanks for the fix. Yes, this was "very obvious in hindsight".
To my defense, I can only argue that it's always been that way -
while I was the last one to touch that line (in 2ff366f78),
the *order* of operations was "always like this"...
- l = ROUNDUP(u.sa_len); memmove(cp, &(u), l); cp += l;\
+ l = ROUNDUP( ((struct sockaddr *)&(u))->sa_len); memmove(cp, &(u), l); cp += l;\
.. anyway. I have tested your fix on all platforms that use this
code path and which I had available, which is
- FreeBSD 7.4/amd64 (works)
- FreeBSD 11.3/amd64 (works)
- FreeBSD 12.1/amd64 (works)
- FreeBSD 12.1/i386 (works)
- FreeBSD 12.x/sparc64 (postponed, hardware down)
- FreeBSD 12.1/arm64 (rPI 3) (works)
- NetBSD 8.1/i386 (turns out to be broken before and after)
("GDG6: problem writing to routing socket", already trac #734)
- OpenBSD 6.5/amd64 (works)
- MacOS Mojave/amd64 (works)
- MacOS 10.5/i386 (works) [only tested 2.4]
and the code is excercised by "openvpn --show-gateway" (and by
the internal user of this, "do we need an IPv6 redirect route").
OpenSolaris, Linux and Windows use different code paths.
(The OpenSolaris NEXTADDR() actually has the same problem, just
nobody noticed it so far - so I'll send a followup patch to fix
it over there as well)
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@gmx.de>
Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <20200717171818.230371-1-matthias.andree@gmx.de>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg20461.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
diff --git a/src/openvpn/route.c b/src/openvpn/route.c index b57da5dd..24563ed6 100644 --- a/src/openvpn/route.c +++ b/src/openvpn/route.c @@ -3436,7 +3436,7 @@ struct rtmsg { #else /* if defined(TARGET_SOLARIS) */ #define NEXTADDR(w, u) \ if (rtm_addrs & (w)) { \ - l = ROUNDUP( ((struct sockaddr *)&(u))->sa_len); memmove(cp, &(u), l); cp += l; \ + l = ((struct sockaddr *)&(u))->sa_len; memmove(cp, &(u), l); cp += ROUNDUP(l); \ } #define ADVANCE(x, n) (x += ROUNDUP((n)->sa_len))
copy first, then round up the length when adding padding to the advance. Found by: GCC 9.3.0 (FreeBSD) Signed-off-by: Matthias Andree <matthias.andree@gmx.de> --- src/openvpn/route.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.25.4