diff mbox series

[Openvpn-devel] Fix stack buffer overruns in NEXTADDR() macro:

Message ID 20200717171818.230371-1-matthias.andree@gmx.de
State Accepted
Delegated to: Gert Doering
Headers
Series [Openvpn-devel] Fix stack buffer overruns in NEXTADDR() macro: |

Commit Message

Matthias Andree July 17, 2020, 7:18 a.m. UTC 
  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

Comments

Gert Doering July 27, 2020, 4:14 a.m. UTC | #1 
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 mbox series

Patch

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))