diff mbox series

[Openvpn-devel,3/8] Ensure that argument to parse_line has always space for final sentinel

Message ID 20221215190143.2107896-4-arne@rfc2549.org
State Accepted
Headers
Series Improvement/fixes based on Trail of Bits audit |

Commit Message

Arne Schwabe Dec. 15, 2022, 7:01 p.m. UTC 
  This fixes two places were we do not have enough space in the array
of parameters given to parse_line for the final NULL parameter that
signal the end of the parsed argument errors.

Both these cases can lead to a buffer overflow. But both of these
cases require root/admin access to OpenVPN:

- parse_argv, only able to trigger if starting openvpn from the command
  line, at this point you cannot  gain more privileges than you already
  have.

  Way to reproduce, compile with ASAN and run:

       openvpn --tls-verify a a a a a a a a a a a a a a a

- remove_iroutes_from_push_route_list

	This operates on the list of pushed entries that is generated
	by the server itself. So trigger this, you need to have control
	over config, management interface, a plugin or cdd files.

The parse_argv problem was found by Trial of Bits. I found the
remove_iroutes_from_push_route_list problem by looking for similar
problems.

Reported-By: Trial of Bits (TOB-OVPN-4)
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/options.c | 9 ++++-----
 src/openvpn/push.c    | 4 ++--
 2 files changed, 6 insertions(+), 7 deletions(-)

Comments

Gert Doering Dec. 16, 2022, 11:43 a.m. UTC | #1 
Acked-by: Gert Doering <gert@greenie.muc.de>

Straightforward enough :-) - still, run through the test mill.  I have
no ASAN environment around right now, so couldn't verify the actual
overflow "before" - but stare-at-code confirms the problem and the fix.

Your patch has been applied to the master, release/2.6, release/2.5
and release/2.4 branch (and has been t_client tested on 2.4+2.5)

commit 749beb6d0cb9f8628997bb656ba2f64e31cac377 (master)
commit d75462fc70a2b0b26fb10063b15b6bfc5c8161db (release/2.6)
commit 01bed788b0ec4007591b81398b56b5f9632ca33c (release/2.5)
commit a12adfa01b2486bff32c9bb3aedf81716ad14c58 (release/2.4)
Author: Arne Schwabe
Date:   Thu Dec 15 20:01:38 2022 +0100

     Ensure that argument to parse_line has always space for final sentinel

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20221215190143.2107896-4-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25734.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index e48e4b459..1d6c0572c 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -5310,8 +5310,6 @@  parse_argv(struct options *options,
            unsigned int *option_types_found,
            struct env_set *es)
 {
-    int i, j;
-
     /* usage message */
     if (argc <= 1)
     {
@@ -5321,7 +5319,7 @@  parse_argv(struct options *options,
     /* config filename specified only? */
     if (argc == 2 && strncmp(argv[1], "--", 2))
     {
-        char *p[MAX_PARMS];
+        char *p[MAX_PARMS+1];
         CLEAR(p);
         p[0] = "config";
         p[1] = argv[1];
@@ -5331,9 +5329,9 @@  parse_argv(struct options *options,
     else
     {
         /* parse command line */
-        for (i = 1; i < argc; ++i)
+        for (int i = 1; i < argc; ++i)
         {
-            char *p[MAX_PARMS];
+            char *p[MAX_PARMS+1];
             CLEAR(p);
             p[0] = argv[i];
             if (strncmp(p[0], "--", 2))
@@ -5345,6 +5343,7 @@  parse_argv(struct options *options,
                 p[0] += 2;
             }
 
+            int j;
             for (j = 1; j < MAX_PARMS; ++j)
             {
                 if (i + j < argc)
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index f8c747d44..ad2f3c656 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -1096,13 +1096,13 @@  remove_iroutes_from_push_route_list(struct options *o)
         /* cycle through the push list */
         while (e)
         {
-            char *p[MAX_PARMS];
+            char *p[MAX_PARMS+1];
             bool enable = true;
 
             /* parse the push item */
             CLEAR(p);
             if (e->enable
-                && parse_line(e->option, p, SIZE(p), "[PUSH_ROUTE_REMOVE]", 1, D_ROUTE_DEBUG, &gc))
+                && parse_line(e->option, p, SIZE(p)-1, "[PUSH_ROUTE_REMOVE]", 1, D_ROUTE_DEBUG, &gc))
             {
                 /* is the push item a route directive? */
                 if (p[0] && !strcmp(p[0], "route") && !p[3] && o->iroutes)