[M] Change in openvpn[master]: win: don't use NULL DACL with system objects

cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/1889?usp=email )

Change subject: win: don't use NULL DACL with system objects
......................................................................

win: don't use NULL DACL with system objects

The netsh semaphore used a NULL DACL, which allowed any user on a shared
system to block openvpn from running. Instances would time out and shut
down.

Similar with the --service exit event. Any user could signal it and shut
down the instance if the event name is known or can be retrieved from
the running process.

To prevent both, the objects are created with a DACL which allows access
to the creating user only. In case of the netsh semaphore this means that
only the first user running openvpn can run more instances. Other
accounts doing so will error out. The interactive service can be used to
prevent this from happening, since the netsh semaphore is only used when
the openvpn process runs privileged operations directly.

Github: OpenVPN/openvpn-private-issues#167
CVE: 2026-82312
Reported-By: DEBRAJ BASAK <https://in.linkedin.com/in/debrajbasak>
Change-Id: I787a7067f6ff040c6a1f43f384321bddf5efc97b
Signed-off-by: Heiko Hund <[email protected]>
Acked-by: Lev Stipakov <[email protected]>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1889
Message-Id: <[email protected]>
URL: https://www.mail-archive.com/[email protected]/msg38886.html
Signed-off-by: Gert Doering <[email protected]>
---
M src/openvpn/win32.c
M src/openvpn/win32.h
2 files changed, 107 insertions(+), 3 deletions(-)




diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c
index 44b010d..dd88a22 100644
--- a/src/openvpn/win32.c
+++ b/src/openvpn/win32.c
@@ -35,6 +35,8 @@
 
 #include <minwindef.h>
 #include <winsock2.h>
+#include <accctrl.h>
+#include <aclapi.h>
 
 #include "buffer.h"
 #include "error.h"
@@ -146,6 +148,13 @@
     pause_exit_enabled = true;
 }
 
+/**
+ * @brief Initializes security attributes with a NULL DACL, allowing
+ *        unrestricted access to the resulting object.
+ *
+ * @param obj Security attributes structure to initialize.
+ * @return true on success, false otherwise.
+ */
 bool
 init_security_attributes_allow_all(struct security_attributes *obj)
 {
@@ -165,6 +174,93 @@
     return true;
 }
 
+/**
+ * @brief Initializes security attributes with a DACL restricted to the
+ *        current process user.
+ *
+ * The resulting DACL grants GENERIC_ALL access to the calling user only,
+ * so the created object cannot be opened, signaled or otherwise accessed
+ * by other users on the system. The allocated DACL must be released with
+ * free_security_attributes() once the security attributes are no longer
+ * needed.
+ *
+ * @param obj Security attributes structure to initialize.
+ * @return true on success, false otherwise.
+ */
+static bool
+init_security_attributes_allow_user(struct security_attributes *obj)
+{
+    bool ret = false;
+
+    CLEAR(*obj);
+    obj->sa.nLength = sizeof(SECURITY_ATTRIBUTES);
+    obj->sa.lpSecurityDescriptor = &obj->sd;
+    obj->sa.bInheritHandle = FALSE;
+
+    if (!InitializeSecurityDescriptor(&obj->sd, SECURITY_DESCRIPTOR_REVISION))
+    {
+        return ret;
+    }
+
+    HANDLE token = NULL;
+    if (!OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &token))
+    {
+        return ret;
+    }
+
+    PTOKEN_USER info = NULL;
+    DWORD info_len = 0;
+    if (!GetTokenInformation(token, TokenUser, info, info_len, &info_len)
+        && GetLastError() != ERROR_INSUFFICIENT_BUFFER)
+    {
+        goto out;
+    }
+
+    info = malloc(info_len);
+    if (!info || !GetTokenInformation(token, TokenUser, info, info_len, &info_len))
+    {
+        goto out;
+    }
+
+    EXPLICIT_ACCESS ea = { 0 };
+    ea.grfAccessPermissions = GENERIC_ALL;
+    ea.grfAccessMode = SET_ACCESS;
+    ea.grfInheritance = NO_INHERITANCE;
+    ea.Trustee.TrusteeForm = TRUSTEE_IS_SID;
+    ea.Trustee.TrusteeType = TRUSTEE_IS_USER;
+    ea.Trustee.ptstrName = (LPTSTR)info->User.Sid;
+
+    if (SetEntriesInAcl(1, &ea, NULL, &obj->dacl) != ERROR_SUCCESS)
+    {
+        goto out;
+    }
+
+    if (SetSecurityDescriptorDacl(&obj->sd, TRUE, obj->dacl, FALSE))
+    {
+        ret = true;
+    }
+
+out:
+    free(info);
+    CloseHandle(token);
+    return ret;
+}
+
+/**
+ * @brief Releases resources allocated by init_security_attributes_allow_user().
+ *
+ * @param obj Security attributes structure to release.
+ */
+static void
+free_security_attributes(struct security_attributes *obj)
+{
+    if (obj->dacl)
+    {
+        LocalFree(obj->dacl);
+        obj->dacl = NULL;
+    }
+}
+
 void
 overlapped_io_init(struct overlapped_io *o, const struct frame *frame, BOOL event_state)
 {
@@ -504,7 +600,7 @@
         struct gc_arena gc = gc_new();
         const wchar_t *exit_event_nameW = wide_string(exit_event_name, &gc);
 
-        if (!init_security_attributes_allow_all(&sa))
+        if (!init_security_attributes_allow_user(&sa))
         {
             msg(M_ERR, "Error: win32_signal_open: init SA failed");
         }
@@ -526,6 +622,7 @@
                 ws->mode = WSO_MODE_SERVICE;
             }
         }
+        free_security_attributes(&sa);
         gc_free(&gc);
     }
     /* set the ctrl handler in both console and service modes */
@@ -751,14 +848,15 @@
     s->name = name;
     s->hand = NULL;
 
-    if (init_security_attributes_allow_all(&sa))
+    if (init_security_attributes_allow_user(&sa))
     {
         s->hand = CreateSemaphore(&sa.sa, 1, 1, name);
     }
+    free_security_attributes(&sa);
 
     if (s->hand == NULL)
     {
-        msg(M_WARN | M_ERRNO, "WARNING: Cannot create Win32 semaphore '%s'", name);
+        msg(M_ERR, "Cannot create Win32 semaphore '%s'", name);
     }
     else
     {
diff --git a/src/openvpn/win32.h b/src/openvpn/win32.h
index ef32062..8be3d96 100644
--- a/src/openvpn/win32.h
+++ b/src/openvpn/win32.h
@@ -63,6 +63,7 @@
 {
     SECURITY_ATTRIBUTES sa;
     SECURITY_DESCRIPTOR sd;
+    PACL dacl;
 };
 
 #define HANDLE_DEFINED(h) ((h) != NULL && (h) != INVALID_HANDLE_VALUE)
@@ -262,6 +263,11 @@
  *
  * It seems you can't run more than one instance
  * of netsh on the same machine at the same time.
+ *
+ * Its DACL is restricted to the creating user to prevent an unprivileged
+ * local user from starving it and DoS'ing running instances. This means
+ * different user accounts running OpenVPN directly, not via the interactive
+ * service, will make all but the first user's instances exit.
  */
 
 extern struct semaphore netcmd_semaphore;

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1889?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: merged
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I787a7067f6ff040c6a1f43f384321bddf5efc97b
Gerrit-Change-Number: 1889
Gerrit-PatchSet: 2
Gerrit-Owner: cron2 <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-Reviewer: stipa <[email protected]>
Gerrit-CC: d12fk <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>

_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel