From d3ec653cf5930298b52569fd0de0f2bdfd84b380 Mon Sep 17 00:00:00 2001
From: Selva Nair <selva.nair@gmail.com>
Date: Thu, 4 Feb 2016 23:19:59 -0500
Subject: [PATCH 1/2] Do not use interactive service if running as admin

Commit 791aea49e6cdab83c5ac9ca203104859a57f4260 cherry-picked from
master. Commit message altered to make it more relevant for this version.

Connecting to a named pipe server while running with admin rights is not
secure in some windows versions. Even if interactive service is not installed,
the GUI attempts to connect to the service pipe making it possible to exploit
this. Windows XP is known to be vulnerable. See also
http://nsylvain.blogspot.ca/2008/01/namedpipe-impersonation-attack.html

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 misc.c    | 26 ++++++++++++++++++++++++++
 misc.h    |  2 ++
 openvpn.c |  5 +++--
 3 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/misc.c b/misc.c
index e21dd93..06ff1d4 100644
--- a/misc.c
+++ b/misc.c
@@ -190,3 +190,29 @@ ForceForegroundWindow(HWND hWnd)
 
     return ret;
 }
+
+/*
+ * Check user has admin rights
+ * Taken from https://msdn.microsoft.com/en-us/library/windows/desktop/aa376389(v=vs.85).aspx
+ * Returns true if the calling process token has the local Administrators group enabled
+ * in its SID.  Assumes the caller is not impersonating and has access to open its own 
+ * process token.
+ */
+BOOL IsUserAdmin(VOID)
+{
+    BOOL b;
+    SID_IDENTIFIER_AUTHORITY NtAuthority = {SECURITY_NT_AUTHORITY};
+    PSID AdministratorsGroup;
+
+    b = AllocateAndInitializeSid (&NtAuthority, 2, SECURITY_BUILTIN_DOMAIN_RID,
+                                  DOMAIN_ALIAS_RID_ADMINS, 0, 0, 0, 0, 0, 0,
+                                  &AdministratorsGroup);
+    if(b)
+    {
+        if (!CheckTokenMembership(NULL, AdministratorsGroup, &b))
+            b = FALSE;
+        FreeSid(AdministratorsGroup);
+    }
+
+    return(b);
+}
diff --git a/misc.h b/misc.h
index 615ba73..8792755 100644
--- a/misc.h
+++ b/misc.h
@@ -30,4 +30,6 @@ BOOL streq(LPCSTR, LPCSTR);
 BOOL wcsbegins(LPCWSTR, LPCWSTR);
 
 BOOL ForceForegroundWindow(HWND);
+
+BOOL IsUserAdmin(VOID);
 #endif
diff --git a/openvpn.c b/openvpn.c
index 7ccf45f..213a189 100644
--- a/openvpn.c
+++ b/openvpn.c
@@ -692,10 +692,11 @@ StartOpenVPN(connection_t *c)
         (o.proxy_source != config ? _T("--management-query-proxy ") : _T("")));
 
     /* Try to open the service pipe */
-    service = CreateFile(_T("\\\\.\\pipe\\openvpn\\service"),
+    if (!IsUserAdmin())
+      service = CreateFile(_T("\\\\.\\pipe\\openvpn\\service"),
                 GENERIC_READ | GENERIC_WRITE, 0, NULL, OPEN_EXISTING, 0, NULL);
 
-    if (service != INVALID_HANDLE_VALUE)
+    if (service && service != INVALID_HANDLE_VALUE)
     {
         DWORD size = _tcslen(c->config_dir) + _tcslen(options) + sizeof(c->manage.password) + 3;
         TCHAR startup_info[1024];

From 19afc9f70b3297262de6c37b986e81bcce3e6379 Mon Sep 17 00:00:00 2001
From: Selva Nair <selva.nair@gmail.com>
Date: Sat, 13 Feb 2016 22:34:25 -0500
Subject: [PATCH 2/2] Put --log first in the command line

This is needed to avoid early messages going to stdout leaving no
trace of errors when openvpn exits before management interface is up.
It also ensures that any --log directives in the config do not override
the log file location.

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 openvpn.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/openvpn.c b/openvpn.c
index 213a189..4a80e0c 100644
--- a/openvpn.c
+++ b/openvpn.c
@@ -682,12 +682,13 @@ StartOpenVPN(connection_t *c)
     /* Create a management interface password */
     GetRandomPassword(c->manage.password, sizeof(c->manage.password) - 1);
 
-    /* Construct command line */
-    _sntprintf_0(cmdline, _T("openvpn --config \"%s\" "
-        "--setenv IV_GUI_VER \"%S\" --service %s 0 --log%s \"%s\" --auth-retry interact "
+    /* Construct command line -- put log first */
+    _sntprintf_0(cmdline, _T("openvpn --log%s \"%s\" --config \"%s\" "
+        "--setenv IV_GUI_VER \"%S\" --service %s 0 --auth-retry interact "
         "--management %S %hd stdin --management-query-passwords %s"
-        "--management-hold"), c->config_file, PACKAGE_STRING, exit_event_name,
+        "--management-hold"),
         (o.append_string[0] == '1' ? _T("-append") : _T("")), c->log_path,
+        c->config_file, PACKAGE_STRING, exit_event_name,
         inet_ntoa(c->manage.skaddr.sin_addr), ntohs(c->manage.skaddr.sin_port),
         (o.proxy_source != config ? _T("--management-query-proxy ") : _T("")));