From da242af8d3750a231bfd687d0a92cf2004dae988 Mon Sep 17 00:00:00 2001 From: Gert Doering Date: Sat, 14 Apr 2018 09:26:17 +0200 Subject: [PATCH] Fix potential double-free() in Interactive Service (CVE-2018-9336) 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 . CVE: 2018-9336 Signed-off-by: Gert Doering Acked-by: Selva Nair 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 (cherry picked from commit 1394192b210cb3c6624a7419bcf3ff966742e79b) --- src/openvpnserv/interactive.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c index 19be0dbdc..9cfc94e04 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 len; - BOOL ret = FALSE; WCHAR *data = NULL; DWORD size, 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; } -- 2.47.2