diff mbox series

[Openvpn-devel] interactive.c: Improve access control for gui<->service pipe

Message ID 20240619134451.222-1-lev@openvpn.net
State Accepted
Headers show
Series [Openvpn-devel] interactive.c: Improve access control for gui<->service pipe | expand

Commit Message

Lev Stipakov June 19, 2024, 1:44 p.m. UTC 
At the moment everyone but anonymous are permitted
to create a pipe with the same name as interactive service creates,
which makes it possible for malicious process with SeImpersonatePrivilege
impersonate as local user.

This hardens the security of the pipe, making it possible only for
processes running as SYSTEM (such as interactive service) create the
pipe with the same name.

While on it, replace EXPLICIT_ACCESS structures with SDDL string.

CVE: 2024-4877

Change-Id: I35e783b79a332d247606e05a39e41b4d35d39b5d
Reported by: Zeze with TeamT5 <zeze7w@gmail.com>
Signed-off-by: Lev Stipakov <lev@openvpn.net>
---
 v2:
  - ensure that sd is freed even if pipe creation failed
  - added Reported-By

 src/openvpnserv/interactive.c | 81 +++++++++++++----------------------
 1 file changed, 29 insertions(+), 52 deletions(-)

Comments

Selva Nair June 19, 2024, 1:51 p.m. UTC | #1 
On Wed, Jun 19, 2024 at 9:47 AM Lev Stipakov <lstipakov@gmail.com> wrote:

> At the moment everyone but anonymous are permitted
> to create a pipe with the same name as interactive service creates,
> which makes it possible for malicious process with SeImpersonatePrivilege
> impersonate as local user.
>
> This hardens the security of the pipe, making it possible only for
> processes running as SYSTEM (such as interactive service) create the
> pipe with the same name.
>
> While on it, replace EXPLICIT_ACCESS structures with SDDL string.
>
> CVE: 2024-4877
>
> Change-Id: I35e783b79a332d247606e05a39e41b4d35d39b5d
> Reported by: Zeze with TeamT5 <zeze7w@gmail.com>
> Signed-off-by: Lev Stipakov <lev@openvpn.net>
> ---
>  v2:
>   - ensure that sd is freed even if pipe creation failed
>   - added Reported-By
>

 Acked-by: Selva Nair <selva.nair@gmail.com>
Selva Nair June 19, 2024, 2:05 p.m. UTC | #2 
Forgot to add:

This applies only to 2.6 -- for master we'll need a rebased version.

On Wed, Jun 19, 2024 at 9:51 AM Selva Nair <selva.nair@gmail.com> wrote:

>
>
> On Wed, Jun 19, 2024 at 9:47 AM Lev Stipakov <lstipakov@gmail.com> wrote:
>
>> At the moment everyone but anonymous are permitted
>> to create a pipe with the same name as interactive service creates,
>> which makes it possible for malicious process with SeImpersonatePrivilege
>> impersonate as local user.
>>
>> This hardens the security of the pipe, making it possible only for
>> processes running as SYSTEM (such as interactive service) create the
>> pipe with the same name.
>>
>> While on it, replace EXPLICIT_ACCESS structures with SDDL string.
>>
>> CVE: 2024-4877
>>
>> Change-Id: I35e783b79a332d247606e05a39e41b4d35d39b5d
>> Reported by: Zeze with TeamT5 <zeze7w@gmail.com>
>> Signed-off-by: Lev Stipakov <lev@openvpn.net>
>> ---
>>  v2:
>>   - ensure that sd is freed even if pipe creation failed
>>   - added Reported-By
>>
>
>  Acked-by: Selva Nair <selva.nair@gmail.com>
>
Gert Doering June 19, 2024, 2:40 p.m. UTC | #3 
This is another "developed in secrecy on the security@ mailing list"
patch, because it has security implications.

It affects windows builds, where it is possible to have two different
processes provide a pipe with the same name (ewwww!), and a connecting
client will might not end up at the interactive service but at "some
random process".  This is not a major issue in itself, but the GUI sends
a "user credentials token" (so openvpn.exe can be run as "normal user"
later on) and this can be abused by a malicious process to get access
to the user running openvpn-gui.exe - now, it's a somewhat theoretical
attack (malicious software having sufficient privileges to do use
a user token, but not having either "that user access" or "system privs"
to start with) - but it's worth fixing.

So, just stay calm, don't panic, and upgrade to 2.6.11 ;-)

I have not tested this beyond "does it compile?" on a local
ubuntu/mingw build and on GHA.  Lev, Selva and Heiko did all the
grunt work on coming up with a solution and testing the patch.

Your patch has been applied to the release/2.6 branch.  A rebase to
master is in the works (this conflicted with the snprintf() cleanup
patch, which is "only in master" and was merged right after *this*
was developed and tested).

Backport to release/2.5 is not fully straightforward either - there have
been a number of fixes to interactive.c, and not all of them have been
backported.  OTOH, we do not intend to provide 2.5.x windows binaries 
ever again (and said so at 2.5.10 release), so now is the time to
upgrade your windows clients to 2.6.x

commit 51301eb6c233c284270e3f4ed0c7f5781f2b5c62 (release/2.6)
Author: Lev Stipakov
Date:   Wed Jun 19 16:44:23 2024 +0300

     interactive.c: Improve access control for gui<->service pipe

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


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index d32223ce..6da8ee5d 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -2137,73 +2137,50 @@  ServiceCtrlInteractive(DWORD ctrl_code, DWORD event, LPVOID data, LPVOID ctx)
 static HANDLE
 CreateClientPipeInstance(VOID)
 {
-    TCHAR pipe_name[256]; /* The entire pipe name string can be up to 256 characters long according to MSDN. */
-    HANDLE pipe = NULL;
-    PACL old_dacl, new_dacl;
-    PSECURITY_DESCRIPTOR sd;
-    static EXPLICIT_ACCESS ea[2];
-    static BOOL initialized = FALSE;
-    DWORD flags = PIPE_ACCESS_DUPLEX | WRITE_DAC | FILE_FLAG_OVERLAPPED;
+    /*
+     * allow all access for local system
+     * deny FILE_CREATE_PIPE_INSTANCE for everyone
+     * allow read/write for authenticated users
+     * deny all access to anonymous
+     */
+    const TCHAR *sddlString = TEXT("D:(A;OICI;GA;;;S-1-5-18)(D;OICI;0x4;;;S-1-1-0)(A;OICI;GRGW;;;S-1-5-11)(D;;GA;;;S-1-5-7)");
 
-    if (!initialized)
+    PSECURITY_DESCRIPTOR sd = NULL;
+    if (!ConvertStringSecurityDescriptorToSecurityDescriptor(sddlString, SDDL_REVISION_1, &sd, NULL))
     {
-        PSID everyone, anonymous;
-
-        ConvertStringSidToSid(TEXT("S-1-1-0"), &everyone);
-        ConvertStringSidToSid(TEXT("S-1-5-7"), &anonymous);
+        MsgToEventLog(M_SYSERR, TEXT("ConvertStringSecurityDescriptorToSecurityDescriptor failed."));
+        return INVALID_HANDLE_VALUE;
+    }
 
-        ea[0].grfAccessPermissions = FILE_GENERIC_WRITE;
-        ea[0].grfAccessMode = GRANT_ACCESS;
-        ea[0].grfInheritance = NO_INHERITANCE;
-        ea[0].Trustee.pMultipleTrustee = NULL;
-        ea[0].Trustee.MultipleTrusteeOperation = NO_MULTIPLE_TRUSTEE;
-        ea[0].Trustee.TrusteeForm = TRUSTEE_IS_SID;
-        ea[0].Trustee.TrusteeType = TRUSTEE_IS_UNKNOWN;
-        ea[0].Trustee.ptstrName = (LPTSTR) everyone;
+    /* Set up SECURITY_ATTRIBUTES */
+    SECURITY_ATTRIBUTES sa = {0};
+    sa.nLength = sizeof(SECURITY_ATTRIBUTES);
+    sa.lpSecurityDescriptor = sd;
+    sa.bInheritHandle = FALSE;
 
-        ea[1].grfAccessPermissions = 0;
-        ea[1].grfAccessMode = REVOKE_ACCESS;
-        ea[1].grfInheritance = NO_INHERITANCE;
-        ea[1].Trustee.pMultipleTrustee = NULL;
-        ea[1].Trustee.MultipleTrusteeOperation = NO_MULTIPLE_TRUSTEE;
-        ea[1].Trustee.TrusteeForm = TRUSTEE_IS_SID;
-        ea[1].Trustee.TrusteeType = TRUSTEE_IS_UNKNOWN;
-        ea[1].Trustee.ptstrName = (LPTSTR) anonymous;
+    DWORD flags = PIPE_ACCESS_DUPLEX | WRITE_DAC | FILE_FLAG_OVERLAPPED;
 
+    static BOOL first = TRUE;
+    if (first)
+    {
         flags |= FILE_FLAG_FIRST_PIPE_INSTANCE;
-        initialized = TRUE;
+        first = FALSE;
     }
 
+    TCHAR pipe_name[256]; /* The entire pipe name string can be up to 256 characters long according to MSDN. */
     openvpn_swprintf(pipe_name, _countof(pipe_name), TEXT("\\\\.\\pipe\\" PACKAGE "%ls\\service"), service_instance);
-    pipe = CreateNamedPipe(pipe_name, flags,
-                           PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_REJECT_REMOTE_CLIENTS,
-                           PIPE_UNLIMITED_INSTANCES, 1024, 1024, 0, NULL);
+    HANDLE pipe = CreateNamedPipe(pipe_name, flags,
+                                  PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_REJECT_REMOTE_CLIENTS,
+                                  PIPE_UNLIMITED_INSTANCES, 1024, 1024, 0, &sa);
+
+    LocalFree(sd);
+
     if (pipe == INVALID_HANDLE_VALUE)
     {
         MsgToEventLog(M_SYSERR, TEXT("Could not create named pipe"));
         return INVALID_HANDLE_VALUE;
     }
 
-    if (GetSecurityInfo(pipe, SE_KERNEL_OBJECT, DACL_SECURITY_INFORMATION,
-                        NULL, NULL, &old_dacl, NULL, &sd) != ERROR_SUCCESS)
-    {
-        MsgToEventLog(M_SYSERR, TEXT("Could not get pipe security info"));
-        return CloseHandleEx(&pipe);
-    }
-
-    if (SetEntriesInAcl(2, ea, old_dacl, &new_dacl) != ERROR_SUCCESS)
-    {
-        MsgToEventLog(M_SYSERR, TEXT("Could not set entries in new acl"));
-        return CloseHandleEx(&pipe);
-    }
-
-    if (SetSecurityInfo(pipe, SE_KERNEL_OBJECT, DACL_SECURITY_INFORMATION,
-                        NULL, NULL, new_dacl, NULL) != ERROR_SUCCESS)
-    {
-        MsgToEventLog(M_SYSERR, TEXT("Could not set pipe security info"));
-        return CloseHandleEx(&pipe);
-    }
-
     return pipe;
 }