interactive.c: harden pipe handling against misbehaving clients

 - Handle ConnectNamedPipe ERROR_NO_DATA as a normal
   connect/drop race: log the drop, disconnect/reset
   that instance, and keep listening instead of letting
   a trivial local DoS stop the service.

 - Add a timed peek for startup data so a client that
   connects and sends nothing is timed out (IO_TIMEOUT)
   and rejected, instead of leaving a worker thread blocked
   forever and piling up handles.

 - Protect the accept loop from resource exhaustion: before
   spawning a worker, check the wait set and reject the client
   if adding another handle would exceed MAXIMUM_WAIT_OBJECTS;
   also skip FlushFileBuffers when no startup data was received
   to avoid hangs on silent clients.

Without these fixes, a malicious local windows user can make the OpenVPN
Interactive Service exit-on-error, thus breaking all OpenVPN connections
until the service is restarted (or the system rebooted).  Thus this has
been classified as "local denial of service" and CVE-2025-13751 has been
assigned.

CVE: 2025-13751
Change-Id: Id6a13b0c8124117bcea2926b16607ef39344015a
Signed-off-by: Lev Stipakov <lev@openvpn.net>
Acked-by: Selva Nair <selva.nair@gmail.com>

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index 220c04278d338a11c3f557c58b1ad16097bd306c..6f04f6b5b26a90859b09ecfa10730c5f1768b32e 100644 (file)
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -208,6 +208,7 @@ ResetOverlapped(LPOVERLAPPED overlapped)
 typedef enum
 {
     peek,
+    peek_timed,
     read,
     write
 } async_op_t;
@@ -260,7 +261,7 @@ AsyncPipeOp(async_op_t op, HANDLE pipe, LPVOID buffer, DWORD size, DWORD count,
         goto out;
     }
 
-    if (op == peek)
+    if (op == peek || op == peek_timed)
     {
         PeekNamedPipe(pipe, NULL, 0, NULL, &bytes, NULL);
     }
@@ -281,6 +282,12 @@ PeekNamedPipeAsync(HANDLE pipe, DWORD count, LPHANDLE events)
     return AsyncPipeOp(peek, pipe, NULL, 0, count, events);
 }
 
+static DWORD
+PeekNamedPipeAsyncTimed(HANDLE pipe, DWORD count, LPHANDLE events)
+{
+    return AsyncPipeOp(peek_timed, pipe, NULL, 0, count, events);
+}
+
 static DWORD
 ReadPipeAsync(HANDLE pipe, LPVOID buffer, DWORD size, DWORD count, LPHANDLE events)
 {
@@ -437,11 +444,11 @@ GetStartupData(HANDLE pipe, STARTUP_DATA *sud)
     WCHAR *data = NULL;
     DWORD bytes, read;
 
-    bytes = PeekNamedPipeAsync(pipe, 1, &exit_event);
+    bytes = PeekNamedPipeAsyncTimed(pipe, 1, &exit_event);
     if (bytes == 0)
     {
-        MsgToEventLog(M_SYSERR, L"PeekNamedPipeAsync failed");
-        ReturnLastError(pipe, L"PeekNamedPipeAsync");
+        MsgToEventLog(M_ERR, L"Timeout waiting for startup data");
+        ReturnError(pipe, ERROR_STARTUP_DATA, L"GetStartupData (timeout)", 1, &exit_event);
         goto err;
     }
 
@@ -3248,6 +3255,7 @@ RunOpenvpn(LPVOID p)
     size_t cmdline_size;
     undo_lists_t undo_lists;
     WCHAR errmsg[512] = L"";
+    BOOL flush_pipe = TRUE;
 
     SECURITY_ATTRIBUTES inheritable = { .nLength = sizeof(inheritable),
                                         .lpSecurityDescriptor = NULL,
@@ -3267,6 +3275,7 @@ RunOpenvpn(LPVOID p)
 
     if (!GetStartupData(pipe, &sud))
     {
+        flush_pipe = FALSE; /* client did not provide startup data */
         goto out;
     }
 
@@ -3562,7 +3571,10 @@ RunOpenvpn(LPVOID p)
     Undo(&undo_lists);
 
 out:
-    FlushFileBuffers(pipe);
+    if (flush_pipe)
+    {
+        FlushFileBuffers(pipe);
+    }
     DisconnectNamedPipe(pipe);
 
     free(ovpn_user);
@@ -3834,11 +3846,30 @@ ServiceStartInteractive(DWORD dwArgc, LPWSTR *lpszArgv)
 
     while (TRUE)
     {
-        if (ConnectNamedPipe(pipe, &overlapped) == FALSE && GetLastError() != ERROR_PIPE_CONNECTED
-            && GetLastError() != ERROR_IO_PENDING)
+        if (!ConnectNamedPipe(pipe, &overlapped))
         {
-            MsgToEventLog(M_SYSERR, L"Could not connect pipe");
-            break;
+            DWORD connect_error = GetLastError();
+            if (connect_error == ERROR_NO_DATA)
+            {
+                /*
+                 * Client connected and disconnected before we could process it.
+                 * Disconnect and retry instead of aborting the service.
+                 */
+                MsgToEventLog(M_ERR, L"ConnectNamedPipe returned ERROR_NO_DATA (client dropped)");
+                DisconnectNamedPipe(pipe);
+                ResetOverlapped(&overlapped);
+                continue;
+            }
+            else if (connect_error == ERROR_PIPE_CONNECTED)
+            {
+                /* No async I/O pending in this case; signal manually. */
+                SetEvent(overlapped.hEvent);
+            }
+            else if (connect_error != ERROR_IO_PENDING)
+            {
+                MsgToEventLog(M_SYSERR, L"Could not connect pipe");
+                break;
+            }
         }
 
         error = WaitForMultipleObjects(handle_count, handles, FALSE, INFINITE);
@@ -3846,6 +3877,17 @@ ServiceStartInteractive(DWORD dwArgc, LPWSTR *lpszArgv)
         {
             /* Client connected, spawn a worker thread for it */
             HANDLE next_pipe = CreateClientPipeInstance();
+
+            /* Avoid exceeding WaitForMultipleObjects MAXIMUM_WAIT_OBJECTS */
+            if (handle_count + 1 > MAXIMUM_WAIT_OBJECTS)
+            {
+                ReturnError(pipe, ERROR_CANT_WAIT, L"Too many concurrent clients", 1, &exit_event);
+                CloseHandleEx(&pipe);
+                pipe = next_pipe;
+                ResetOverlapped(&overlapped);
+                continue;
+            }
+
             HANDLE thread = CreateThread(NULL, 0, RunOpenvpn, pipe, CREATE_SUSPENDED, NULL);
             if (thread)
             {