[Openvpn-devel,v2] Fix potential double-free() in Interactive Service (CVE-2018-9336)

Message ID 20180414072617.25075-1-gert@greenie.muc.de
State Accepted
Headers show
Series
  • [Openvpn-devel,v2] Fix potential double-free() in Interactive Service (CVE-2018-9336)
Related show

Commit Message

Gert Doering April 14, 2018, 7:26 a.m.
Malformed input data on the service pipe towards the OpenVPN interactive
service (normally used by the OpenVPN GUI to request openvpn instances
from the service) can result in a double free() in the error handling code.

This usually only leads to a process crash (DoS by an unprivileged local
account) but since it could possibly lead to memory corruption if
happening while multiple other threads are active at the same time,
CVE-2018-9336 has been assigned to acknowledge this risk.

Fix by ensuring that sud->directory is set to NULL in GetStartUpData()
for all error cases (thus not being free()ed in FreeStartupData()).

Rewrite control flow to use explicit error label for error exit.

Discovered and reported by Jacob Baines <jbaines@tenable.com>.

CVE: 2018-9336

Signed-off-by: Gert Doering <gert@greenie.muc.de>

--
v2: reword commit message, no code changes
---
 src/openvpnserv/interactive.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

Comments

Selva Nair April 14, 2018, 4:44 p.m. | #1
On Sat, Apr 14, 2018 at 3:26 AM, Gert Doering <gert@greenie.muc.de> wrote:

> Malformed input data on the service pipe towards the OpenVPN interactive
> service (normally used by the OpenVPN GUI to request openvpn instances
> from the service) can result in a double free() in the error handling code.
>
> This usually only leads to a process crash (DoS by an unprivileged local
> account) but since it could possibly lead to memory corruption if
> happening while multiple other threads are active at the same time,
> CVE-2018-9336 has been assigned to acknowledge this risk.
>
> Fix by ensuring that sud->directory is set to NULL in GetStartUpData()
> for all error cases (thus not being free()ed in FreeStartupData()).
>
> Rewrite control flow to use explicit error label for error exit.
>
> Discovered and reported by Jacob Baines <jbaines@tenable.com>.
>
> CVE: 2018-9336
>
> Signed-off-by: Gert Doering <gert@greenie.muc.de>
>
> --
> v2: reword commit message, no code changes


Just for completeness: all good so ACK again.

Selva
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Sat, Apr 14, 2018 at 3:26 AM, Gert Doering <span dir="ltr">&lt;<a href="mailto:gert@greenie.muc.de" target="_blank">gert@greenie.muc.de</a>&gt;</span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Malformed input data on the service pipe towards the OpenVPN interactive<br>
service (normally used by the OpenVPN GUI to request openvpn instances<br>
from the service) can result in a double free() in the error handling code.<br>
<br>
This usually only leads to a process crash (DoS by an unprivileged local<br>
account) but since it could possibly lead to memory corruption if<br>
happening while multiple other threads are active at the same time,<br>
CVE-2018-9336 has been assigned to acknowledge this risk.<br>
<br>
Fix by ensuring that sud-&gt;directory is set to NULL in GetStartUpData()<br>
for all error cases (thus not being free()ed in FreeStartupData()).<br>
<br>
Rewrite control flow to use explicit error label for error exit.<br>
<br>
Discovered and reported by Jacob Baines &lt;<a href="mailto:jbaines@tenable.com">jbaines@tenable.com</a>&gt;.<br>
<br>
CVE: 2018-9336<br>
<br>
Signed-off-by: Gert Doering &lt;<a href="mailto:gert@greenie.muc.de">gert@greenie.muc.de</a>&gt;<br>
<br>
--<br>
v2: reword commit message, no code changes</blockquote><div><br></div><div>Just for completeness: all good so ACK again. </div><div><br></div><div>Selva</div></div></div></div>
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Gert Doering April 19, 2018, 3:24 p.m. | #2
Your patch has been applied to the master and release/2.4 branch.

commit 1394192b210cb3c6624a7419bcf3ff966742e79b (master)
commit da242af8d3750a231bfd687d0a92cf2004dae988 (release/2.4)
Author: Gert Doering
Date:   Sat Apr 14 09:26:17 2018 +0200

     Fix potential double-free() in Interactive Service (CVE-2018-9336)

     Signed-off-by: Gert Doering <gert@greenie.muc.de>
     Acked-by: Selva Nair <selva.nair@gmail.com>
     Message-Id: <20180414072617.25075-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/search?l=mid&q=20180414072617.25075-1-gert@greenie.muc.de
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Gert Doering April 25, 2018, 6:33 a.m. | #3
Hi,

On Sat, Apr 14, 2018 at 09:26:17AM +0200, Gert Doering wrote:
> Malformed input data on the service pipe towards the OpenVPN interactive
> service (normally used by the OpenVPN GUI to request openvpn instances
> from the service) can result in a double free() in the error handling code.
[..]

Due to the sensitive nature of the patch, it was held under embargo rules
- that is "only sent to the security@ list, privately ACKed, commits not
announced to the public list right away".

I do have an ACK for this from Selva, and have my usually "it went it
with commit ID..." mail, but for whatever reason I cannot re-send my own
mail to the -devel list, and I think SPF is stopping me from bouncing
Selva's mail...  which is a bit of an annoyance.

So I just forward the e-mails below, PGP sign all this (and the commits
are PGP-signed as well), and leave verification of "is the commit the
same as in the repo?  is it what Selva ACKed?" to the interested reader.


-------------------------------------------------
From: Selva Nair <selva.nair@gmail.com>
Date: Sat, 14 Apr 2018 12:44:57 -0400
Message-ID: <CAKuzo_h-GusttjiFUeV2tivFAKHFdqTnDseg9KiVru5ftWszyg@mail.gmail.com>
Subject: Re: [PATCH v2] Fix potential double-free() in Interactive Service (CVE-2018-9336)
To: Gert Doering <gert@greenie.muc.de>
Cc: security@openvpn.net, jbaines@tenable.com

On Sat, Apr 14, 2018 at 3:26 AM, Gert Doering <gert@greenie.muc.de> wrote:

> Malformed input data on the service pipe towards the OpenVPN interactive
> service (normally used by the OpenVPN GUI to request openvpn instances
> from the service) can result in a double free() in the error handling code.
>
> This usually only leads to a process crash (DoS by an unprivileged local
> account) but since it could possibly lead to memory corruption if
> happening while multiple other threads are active at the same time,
> CVE-2018-9336 has been assigned to acknowledge this risk.
>
> Fix by ensuring that sud->directory is set to NULL in GetStartUpData()
> for all error cases (thus not being free()ed in FreeStartupData()).
>
> Rewrite control flow to use explicit error label for error exit.
>
> Discovered and reported by Jacob Baines <jbaines@tenable.com>.
>
> CVE: 2018-9336
>
> Signed-off-by: Gert Doering <gert@greenie.muc.de>
>
> --
> v2: reword commit message, no code changes


Just for completeness: all good so ACK again.

Selva

-------------------------------------------------
Date: Thu, 19 Apr 2018 17:24:49 +0200 (CEST)
Message-Id: <201804191524.w3JFOnGd007014@chekov.greenie.muc.de>
From: Gert Doering <gert@greenie.muc.de>
To: Gert Doering <gert@greenie.muc.de>
Cc: openvpn-devel@lists.sourceforge.net
Subject: [PATCH applied] Re: Fix potential double-free() in Interactive Service (CVE-2018-9336)

Your patch has been applied to the master and release/2.4 branch.

commit 1394192b210cb3c6624a7419bcf3ff966742e79b (master)
commit da242af8d3750a231bfd687d0a92cf2004dae988 (release/2.4)
Author: Gert Doering
Date:   Sat Apr 14 09:26:17 2018 +0200

     Fix potential double-free() in Interactive Service (CVE-2018-9336)

     Signed-off-by: Gert Doering <gert@greenie.muc.de>
     Acked-by: Selva Nair <selva.nair@gmail.com>
     Message-Id: <20180414072617.25075-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/search?l=mid&q=20180414072617.25075-1-gert@greenie.muc.de
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index fbc32f90..861f5e70 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -453,7 +453,6 @@  static BOOL
 GetStartupData(HANDLE pipe, STARTUP_DATA *sud)
 {
     size_t size, len;
-    BOOL ret = FALSE;
     WCHAR *data = NULL;
     DWORD bytes, read;
 
@@ -462,7 +461,7 @@  GetStartupData(HANDLE pipe, STARTUP_DATA *sud)
     {
         MsgToEventLog(M_SYSERR, TEXT("PeekNamedPipeAsync failed"));
         ReturnLastError(pipe, L"PeekNamedPipeAsync");
-        goto out;
+        goto err;
     }
 
     size = bytes / sizeof(*data);
@@ -470,7 +469,7 @@  GetStartupData(HANDLE pipe, STARTUP_DATA *sud)
     {
         MsgToEventLog(M_SYSERR, TEXT("malformed startup data: 1 byte received"));
         ReturnError(pipe, ERROR_STARTUP_DATA, L"GetStartupData", 1, &exit_event);
-        goto out;
+        goto err;
     }
 
     data = malloc(bytes);
@@ -478,7 +477,7 @@  GetStartupData(HANDLE pipe, STARTUP_DATA *sud)
     {
         MsgToEventLog(M_SYSERR, TEXT("malloc failed"));
         ReturnLastError(pipe, L"malloc");
-        goto out;
+        goto err;
     }
 
     read = ReadPipeAsync(pipe, data, bytes, 1, &exit_event);
@@ -486,14 +485,14 @@  GetStartupData(HANDLE pipe, STARTUP_DATA *sud)
     {
         MsgToEventLog(M_SYSERR, TEXT("ReadPipeAsync failed"));
         ReturnLastError(pipe, L"ReadPipeAsync");
-        goto out;
+        goto err;
     }
 
     if (data[size - 1] != 0)
     {
         MsgToEventLog(M_ERR, TEXT("Startup data is not NULL terminated"));
         ReturnError(pipe, ERROR_STARTUP_DATA, L"GetStartupData", 1, &exit_event);
-        goto out;
+        goto err;
     }
 
     sud->directory = data;
@@ -503,7 +502,7 @@  GetStartupData(HANDLE pipe, STARTUP_DATA *sud)
     {
         MsgToEventLog(M_ERR, TEXT("Startup data ends at working directory"));
         ReturnError(pipe, ERROR_STARTUP_DATA, L"GetStartupData", 1, &exit_event);
-        goto out;
+        goto err;
     }
 
     sud->options = sud->directory + len;
@@ -513,16 +512,16 @@  GetStartupData(HANDLE pipe, STARTUP_DATA *sud)
     {
         MsgToEventLog(M_ERR, TEXT("Startup data ends at command line options"));
         ReturnError(pipe, ERROR_STARTUP_DATA, L"GetStartupData", 1, &exit_event);
-        goto out;
+        goto err;
     }
 
     sud->std_input = sud->options + len;
-    data = NULL; /* don't free data */
-    ret = TRUE;
+    return TRUE;
 
-out:
+err:
+    sud->directory = NULL;		/* caller must not free() */
     free(data);
-    return ret;
+    return FALSE;
 }