diff mbox series

[ovs-dev] lib/stream-windows.c: Grant Access Privilege of Named Pipe to Creator

Message ID BN6PR05MB33641B83DEDA70B205C5D33DC30C0@BN6PR05MB3364.namprd05.prod.outlook.com
State Accepted
Delegated to: Alin Gabriel Serdean
Headers show
Series [ovs-dev] lib/stream-windows.c: Grant Access Privilege of Named Pipe to Creator | expand

Commit Message

姜立东 via dev Jan. 22, 2020, 8:18 a.m. UTC
From e42950665acee9aab941b26ebdd067ca0de908a3 Mon Sep 17 00:00:00 2001
From: Ning Wu <nwu@vmware.com>
Date: Tue, 21 Jan 2020 23:46:58 -0800
Subject: [PATCH]lib/stream-windows.c: Grant Access Privilege of Named Pipe to Creator

Current implementation of ovs on windows only allows LocalSystem and
Administrators to access the named pipe created with API of ovs.
Thus any service that needs to invoke the API to create named pipe
has to run as System account to interactive with ovs. It causes the
system more vulnerable if one of those services was break into.
The patch adds the creator owner account to allowed ACLs.

Signed-off-by: Ning Wu <nwu@vmware.com>
---
 Documentation/ref/ovsdb.7.rst |  3 ++-
 lib/stream-windows.c          | 33 ++++++++++++++++++++++++++++++++-
 2 files changed, 34 insertions(+), 2 deletions(-)

Comments

姜立东 via dev Jan. 23, 2020, 7 a.m. UTC | #1
Hi Alin and Anand,

I've rebased the patch and also added some description on the documentation. 
Please help to review.
Thanks.

-----Original Message-----
From: Ning Wu 
Sent: Wednesday, January 22, 2020 16:19
To: Alin Serdean <aserdean@cloudbasesolutions.com>; dev@openvswitch.org; Anand Kumar <kumaranand@vmware.com>
Cc: Lina Li <linali@vmware.com>; Roy Luo <luoroy@vmware.com>
Subject: [PATCH]lib/stream-windows.c: Grant Access Privilege of Named Pipe to Creator

From e42950665acee9aab941b26ebdd067ca0de908a3 Mon Sep 17 00:00:00 2001
From: Ning Wu <nwu@vmware.com>
Date: Tue, 21 Jan 2020 23:46:58 -0800
Subject: [PATCH]lib/stream-windows.c: Grant Access Privilege of Named Pipe to Creator

Current implementation of ovs on windows only allows LocalSystem and Administrators to access the named pipe created with API of ovs.
Thus any service that needs to invoke the API to create named pipe has to run as System account to interactive with ovs. It causes the system more vulnerable if one of those services was break into.
The patch adds the creator owner account to allowed ACLs.

Signed-off-by: Ning Wu <nwu@vmware.com>
---
 Documentation/ref/ovsdb.7.rst |  3 ++-
 lib/stream-windows.c          | 33 ++++++++++++++++++++++++++++++++-
 2 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/Documentation/ref/ovsdb.7.rst b/Documentation/ref/ovsdb.7.rst index b1f3f5d..da4dbed 100644
--- a/Documentation/ref/ovsdb.7.rst
+++ b/Documentation/ref/ovsdb.7.rst
@@ -422,7 +422,8 @@ punix:<file>
     named <file>.
 
     On Windows, listens on a local named pipe, creating a named pipe
-    <file> to mimic the behavior of a Unix domain socket.
+    <file> to mimic the behavior of a Unix domain socket. The ACLs of the named
+    pipe include LocalSystem, Administrators, and Creator Owner.
 
 All IP-based connection methods accept IPv4 and IPv6 addresses.  To specify an
 IPv6 address, wrap it in square brackets, e.g.  ``ssl:[::1]:6640``.  Passive diff --git a/lib/stream-windows.c b/lib/stream-windows.c index 34bc610..5c4c55e 100644
--- a/lib/stream-windows.c
+++ b/lib/stream-windows.c
@@ -41,7 +41,7 @@ static void maybe_unlink_and_free(char *path);  #define LOCAL_PREFIX "\\\\.\\pipe\\"
 
 /* Size of the allowed PSIDs for securing Named Pipe. */ -#define ALLOWED_PSIDS_SIZE 2
+#define ALLOWED_PSIDS_SIZE 3
 
 /* This function has the purpose to remove all the slashes received in s. */  static char * @@ -412,6 +412,9 @@ create_pnpipe(char *name)
     PACL acl = NULL;
     PSECURITY_DESCRIPTOR psd = NULL;
     HANDLE npipe;
+    HANDLE hToken = NULL;
+    DWORD dwBufSize = 0;
+    PTOKEN_USER pTokenUsr = NULL;
 
     /* Disable access over network. */
     if (!AllocateAndInitializeSid(&sia, 1, SECURITY_NETWORK_RID, @@ -438,6 +441,32 @@ create_pnpipe(char *name)
         goto handle_error;
     }
 
+    /* Open the access token of calling process */
+    if (!OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &hToken)) {
+        VLOG_ERR_RL(&rl, "Error opening access token of calling process.");
+        goto handle_error;
+    }
+
+    /* get the buffer size buffer needed for SID */
+    GetTokenInformation(hToken, TokenUser, NULL, 0, &dwBufSize);
+
+    pTokenUsr = xmalloc(dwBufSize);
+    memset(pTokenUsr, 0, dwBufSize);
+
+    /* Retrieve the token information in a TOKEN_USER structure. */
+    if (!GetTokenInformation(hToken, TokenUser, pTokenUsr, dwBufSize,
+        &dwBufSize)) {
+        VLOG_ERR_RL(&rl, "Error retrieving token information.");
+        goto handle_error;
+    }
+    CloseHandle(hToken);
+
+    if (!IsValidSid(pTokenUsr->User.Sid)) {
+        VLOG_ERR_RL(&rl, "Invalid SID.");
+        goto handle_error;
+    }
+    allowedPsid[2] = pTokenUsr->User.Sid;
+
     for (int i = 0; i < ALLOWED_PSIDS_SIZE; i++) {
         aclSize += sizeof(ACCESS_ALLOWED_ACE) +
                    GetLengthSid(allowedPsid[i]) - @@ -490,11 +519,13 @@ create_pnpipe(char *name)
     npipe = CreateNamedPipe(name, PIPE_ACCESS_DUPLEX | FILE_FLAG_OVERLAPPED,
                             PIPE_TYPE_MESSAGE | PIPE_READMODE_BYTE | PIPE_WAIT,
                             64, BUFSIZE, BUFSIZE, 0, &sa);
+    free(pTokenUsr);
     free(acl);
     free(psd);
     return npipe;
 
 handle_error:
+    free(pTokenUsr);
     free(acl);
     free(psd);
     return INVALID_HANDLE_VALUE;
--
2.6.2

-----Original Message-----
From: Alin Serdean <aserdean@cloudbasesolutions.com>
Sent: Wednesday, January 22, 2020 12:19
To: Ning Wu <nwu@vmware.com>; dev@openvswitch.org; Anand Kumar <kumaranand@vmware.com>
Cc: Lina Li <linali@vmware.com>; Roy Luo <luoroy@vmware.com>
Subject: RE: [PATCH] Grant Access Privilege of Named Pipe to Creator

Hi,

Sorry I missed the email.

The direction sounds ok with me. It will surely help with unit tests, since right now they require elevated permissions.

Adding also Anand in the loop.
Anand do you like the idea?

Please also add a few lines to the documentation so users are aware of the change.

The patch as is, fails to apply. Rebase on master.

Also please change the title so it is in compliance with:
https://nam04.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdocs.openvswitch.org%2Fen%2Flatest%2Finternals%2Fcontributing%2Fsubmitting-patches%2F%23email-subject&amp;data=02%7C01%7Cnwu%40vmware.com%7Cf87a8fa19a3d438325d008d79ef2400c%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C1%7C637152635616314521&amp;sdata=h73zweLknUd2m9ReaYEe94QNR4wj3q5TilBV8tQmjiw%3D&amp;reserved=0 

Thanks,
Alin.

> -----Original Message-----
> From: Ning Wu <nwu@vmware.com>
> Sent: Monday, January 20, 2020 4:16 AM
> To: dev@openvswitch.org; Alin Serdean 
> <aserdean@cloudbasesolutions.com>
> Cc: Lina Li <linali@vmware.com>; Roy Luo <luoroy@vmware.com>
> Subject: RE: [PATCH] Grant Access Privilege of Named Pipe to Creator
> 
> Hi Alin,
> 
> 
> 
> Could you please help to give some comment?
> 
> Thanks.
> 
> 
> 
> From: Ning Wu
> Sent: Friday, January 17, 2020 15:15
> To: dev@openvswitch.org; Alin Serdean 
> <aserdean@cloudbasesolutions.com>
> Cc: Lina Li <linali@vmware.com>; Roy Luo <luoroy@vmware.com>
> Subject: [PATCH] Grant Access Privilege of Named Pipe to Creator
> 
> 
> 
> Current implementation of ovs on windows only allows LocalSystem and
> 
> Administrators to access the named pipe created with API of ovs.
> 
> Thus any service that needs to invoke the API to create named pipe
> 
> has to run as System account to interactive with ovs. It causes the
> 
> system more vulnerable if one of those services was break into.
> 
> The patch adds the creator owner account to allowed ACLs.
> 
> 
> 
> Signed-off-by: Ning Wu <nwu@vmware.com <mailto:nwu@vmware.com> >
> 
> ---
> 
> lib/stream-windows.c | 33 ++++++++++++++++++++++++++++++++-
> 
> 1 file changed, 32 insertions(+), 1 deletion(-)
> 
> 
> 
> diff --git a/lib/stream-windows.c b/lib/stream-windows.c
> 
> index 34bc610..0cad927 100644
> 
> --- a/lib/stream-windows.c
> 
> +++ b/lib/stream-windows.c
> 
> @@ -41,7 +41,7 @@ static void maybe_unlink_and_free(char *path);
> 
> #define LOCAL_PREFIX "\\\\.\\pipe\\ <file://.//pipe/> "
> 
> 
> 
> /* Size of the allowed PSIDs for securing Named Pipe. */
> 
> -#define ALLOWED_PSIDS_SIZE 2
> 
> +#define ALLOWED_PSIDS_SIZE 3
> 
> 
> 
> /* This function has the purpose to remove all the slashes received in 
> s. */
> 
> static char *
> 
> @@ -412,6 +412,9 @@ create_pnpipe(char *name)
> 
>      PACL acl = NULL;
> 
>      PSECURITY_DESCRIPTOR psd = NULL;
> 
>      HANDLE npipe;
> 
> +    HANDLE hToken = NULL;
> 
> +    DWORD dwBufSize = 0;
> 
> +    PTOKEN_USER pTokenUsr = NULL;
> 
> 
> 
>      /* Disable access over network. */
> 
>      if (!AllocateAndInitializeSid(&sia, 1, SECURITY_NETWORK_RID,
> 
> @@ -438,6 +441,32 @@ create_pnpipe(char *name)
> 
>          goto handle_error;
> 
>      }
> 
> 
> 
> +    /* Open the access token of calling process */
> 
> +    if (!OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &hToken)) 
> + {
> 
> +       VLOG_ERR_RL(&rl, "Error opening access token of calling 
> + process.");
> 
> +        goto handle_error;
> 
> +    }
> 
> +
> 
> +    /* get the buffer size buffer needed for SID */
> 
> +    GetTokenInformation(hToken, TokenUser, NULL, 0, &dwBufSize);
> 
> +
> 
> +    pTokenUsr = xmalloc(dwBufSize);
> 
> +    memset(pTokenUsr, 0, dwBufSize);
> 
> +
> 
> +    /* Retrieve the token information in a TOKEN_USER structure. */
> 
> +    if (!GetTokenInformation(hToken, TokenUser, pTokenUsr, dwBufSize,
> 
> +        &dwBufSize)) {
> 
> +        VLOG_ERR_RL(&rl, "Error retrieving token information.");
> 
> +        goto handle_error;
> 
> +    }
> 
> +    CloseHandle(hToken);
> 
> +
> 
> +    if (!IsValidSid(pTokenUsr->User.Sid)) {
> 
> +        VLOG_ERR_RL(&rl, "Invalid SID.");
> 
> +        goto handle_error;
> 
> +    }
> 
> +    allowedPsid[2] = pTokenUsr->User.Sid;
> 
> +
> 
>      for (int i = 0; i < ALLOWED_PSIDS_SIZE; i++) {
> 
>          aclSize += sizeof(ACCESS_ALLOWED_ACE) +
> 
>                     GetLengthSid(allowedPsid[i]) -
> 
> @@ -490,11 +519,13 @@ create_pnpipe(char *name)
> 
>      npipe = CreateNamedPipe(name, PIPE_ACCESS_DUPLEX | 
> FILE_FLAG_OVERLAPPED,
> 
>                              PIPE_TYPE_MESSAGE | PIPE_READMODE_BYTE | 
> PIPE_WAIT,
> 
>                              64, BUFSIZE, BUFSIZE, 0, &sa);
> 
> +    free(pTokenUsr);
> 
>      free(acl);
> 
>      free(psd);
> 
>      return npipe;
> 
> 
> 
> handle_error:
> 
> +    free(pTokenUsr);
> 
>      free(acl);
> 
>      free(psd);
> 
>      return INVALID_HANDLE_VALUE;
> 
> --
> 
> 2.6.2
> 
>
Alin Serdean Jan. 23, 2020, 2:29 p.m. UTC | #2
> -----Original Message-----
> From: Ning Wu <nwu@vmware.com>
> Sent: Wednesday, January 22, 2020 10:19 AM
> To: Alin Serdean <aserdean@cloudbasesolutions.com>; dev@openvswitch.org;
> Anand Kumar <kumaranand@vmware.com>
> Cc: Lina Li <linali@vmware.com>; Roy Luo <luoroy@vmware.com>
> Subject: [PATCH]lib/stream-windows.c: Grant Access Privilege of Named Pipe to
> Creator
> 
[Alin Serdean] In the future include [PATCH v##], where ## is the number
of the revision.

Acked-by: Alin Gabriel Serdean <aserdean@ovn.org>
姜立东 via dev Jan. 24, 2020, 5:22 a.m. UTC | #3
Acked-by: Anand Kumar <kumaranand@vmware.com> 

Thanks,
Anand Kumar

On 1/22/20, 12:19 AM, "Ning Wu" <nwu@vmware.com> wrote:

    From e42950665acee9aab941b26ebdd067ca0de908a3 Mon Sep 17 00:00:00 2001
    From: Ning Wu <nwu@vmware.com>
    Date: Tue, 21 Jan 2020 23:46:58 -0800
    Subject: [PATCH]lib/stream-windows.c: Grant Access Privilege of Named Pipe to Creator
    
    Current implementation of ovs on windows only allows LocalSystem and
    Administrators to access the named pipe created with API of ovs.
    Thus any service that needs to invoke the API to create named pipe
    has to run as System account to interactive with ovs. It causes the
    system more vulnerable if one of those services was break into.
    The patch adds the creator owner account to allowed ACLs.
    
    Signed-off-by: Ning Wu <nwu@vmware.com>
    ---
     Documentation/ref/ovsdb.7.rst |  3 ++-
     lib/stream-windows.c          | 33 ++++++++++++++++++++++++++++++++-
     2 files changed, 34 insertions(+), 2 deletions(-)
    
    diff --git a/Documentation/ref/ovsdb.7.rst b/Documentation/ref/ovsdb.7.rst
    index b1f3f5d..da4dbed 100644
    --- a/Documentation/ref/ovsdb.7.rst
    +++ b/Documentation/ref/ovsdb.7.rst
    @@ -422,7 +422,8 @@ punix:<file>
         named <file>.
     
         On Windows, listens on a local named pipe, creating a named pipe
    -    <file> to mimic the behavior of a Unix domain socket.
    +    <file> to mimic the behavior of a Unix domain socket. The ACLs of the named
    +    pipe include LocalSystem, Administrators, and Creator Owner.
     
     All IP-based connection methods accept IPv4 and IPv6 addresses.  To specify an
     IPv6 address, wrap it in square brackets, e.g.  ``ssl:[::1]:6640``.  Passive
    diff --git a/lib/stream-windows.c b/lib/stream-windows.c
    index 34bc610..5c4c55e 100644
    --- a/lib/stream-windows.c
    +++ b/lib/stream-windows.c
    @@ -41,7 +41,7 @@ static void maybe_unlink_and_free(char *path);
     #define LOCAL_PREFIX "\\\\.\\pipe\\"
     
     /* Size of the allowed PSIDs for securing Named Pipe. */
    -#define ALLOWED_PSIDS_SIZE 2
    +#define ALLOWED_PSIDS_SIZE 3
     
     /* This function has the purpose to remove all the slashes received in s. */
     static char *
    @@ -412,6 +412,9 @@ create_pnpipe(char *name)
         PACL acl = NULL;
         PSECURITY_DESCRIPTOR psd = NULL;
         HANDLE npipe;
    +    HANDLE hToken = NULL;
    +    DWORD dwBufSize = 0;
    +    PTOKEN_USER pTokenUsr = NULL;
     
         /* Disable access over network. */
         if (!AllocateAndInitializeSid(&sia, 1, SECURITY_NETWORK_RID,
    @@ -438,6 +441,32 @@ create_pnpipe(char *name)
             goto handle_error;
         }
     
    +    /* Open the access token of calling process */
    +    if (!OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &hToken)) {
    +        VLOG_ERR_RL(&rl, "Error opening access token of calling process.");
    +        goto handle_error;
    +    }
    +
    +    /* get the buffer size buffer needed for SID */
    +    GetTokenInformation(hToken, TokenUser, NULL, 0, &dwBufSize);
    +
    +    pTokenUsr = xmalloc(dwBufSize);
    +    memset(pTokenUsr, 0, dwBufSize);
    +
    +    /* Retrieve the token information in a TOKEN_USER structure. */
    +    if (!GetTokenInformation(hToken, TokenUser, pTokenUsr, dwBufSize,
    +        &dwBufSize)) {
    +        VLOG_ERR_RL(&rl, "Error retrieving token information.");
    +        goto handle_error;
    +    }
    +    CloseHandle(hToken);
    +
    +    if (!IsValidSid(pTokenUsr->User.Sid)) {
    +        VLOG_ERR_RL(&rl, "Invalid SID.");
    +        goto handle_error;
    +    }
    +    allowedPsid[2] = pTokenUsr->User.Sid;
    +
         for (int i = 0; i < ALLOWED_PSIDS_SIZE; i++) {
             aclSize += sizeof(ACCESS_ALLOWED_ACE) +
                        GetLengthSid(allowedPsid[i]) -
    @@ -490,11 +519,13 @@ create_pnpipe(char *name)
         npipe = CreateNamedPipe(name, PIPE_ACCESS_DUPLEX | FILE_FLAG_OVERLAPPED,
                                 PIPE_TYPE_MESSAGE | PIPE_READMODE_BYTE | PIPE_WAIT,
                                 64, BUFSIZE, BUFSIZE, 0, &sa);
    +    free(pTokenUsr);
         free(acl);
         free(psd);
         return npipe;
     
     handle_error:
    +    free(pTokenUsr);
         free(acl);
         free(psd);
         return INVALID_HANDLE_VALUE;
    -- 
    2.6.2
    
    -----Original Message-----
    From: Alin Serdean <aserdean@cloudbasesolutions.com> 
    Sent: Wednesday, January 22, 2020 12:19
    To: Ning Wu <nwu@vmware.com>; dev@openvswitch.org; Anand Kumar <kumaranand@vmware.com>
    Cc: Lina Li <linali@vmware.com>; Roy Luo <luoroy@vmware.com>
    Subject: RE: [PATCH] Grant Access Privilege of Named Pipe to Creator
    
    Hi,
    
    Sorry I missed the email.
    
    The direction sounds ok with me. It will surely help with unit tests, since right now they require elevated permissions.
    
    Adding also Anand in the loop.
    Anand do you like the idea?
    
    Please also add a few lines to the documentation so users are aware of the change.
    
    The patch as is, fails to apply. Rebase on master.
    
    Also please change the title so it is in compliance with:
    https://nam04.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdocs.openvswitch.org%2Fen%2Flatest%2Finternals%2Fcontributing%2Fsubmitting-patches%2F%23email-subject&amp;data=02%7C01%7Cnwu%40vmware.com%7Cf87a8fa19a3d438325d008d79ef2400c%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C1%7C637152635616314521&amp;sdata=h73zweLknUd2m9ReaYEe94QNR4wj3q5TilBV8tQmjiw%3D&amp;reserved=0 
    
    Thanks,
    Alin.
    
    > -----Original Message-----
    > From: Ning Wu <nwu@vmware.com>
    > Sent: Monday, January 20, 2020 4:16 AM
    > To: dev@openvswitch.org; Alin Serdean 
    > <aserdean@cloudbasesolutions.com>
    > Cc: Lina Li <linali@vmware.com>; Roy Luo <luoroy@vmware.com>
    > Subject: RE: [PATCH] Grant Access Privilege of Named Pipe to Creator
    > 
    > Hi Alin,
    > 
    > 
    > 
    > Could you please help to give some comment?
    > 
    > Thanks.
    > 
    > 
    > 
    > From: Ning Wu
    > Sent: Friday, January 17, 2020 15:15
    > To: dev@openvswitch.org; Alin Serdean 
    > <aserdean@cloudbasesolutions.com>
    > Cc: Lina Li <linali@vmware.com>; Roy Luo <luoroy@vmware.com>
    > Subject: [PATCH] Grant Access Privilege of Named Pipe to Creator
    > 
    > 
    > 
    > Current implementation of ovs on windows only allows LocalSystem and
    > 
    > Administrators to access the named pipe created with API of ovs.
    > 
    > Thus any service that needs to invoke the API to create named pipe
    > 
    > has to run as System account to interactive with ovs. It causes the
    > 
    > system more vulnerable if one of those services was break into.
    > 
    > The patch adds the creator owner account to allowed ACLs.
    > 
    > 
    > 
    > Signed-off-by: Ning Wu <nwu@vmware.com <mailto:nwu@vmware.com> >
    > 
    > ---
    > 
    > lib/stream-windows.c | 33 ++++++++++++++++++++++++++++++++-
    > 
    > 1 file changed, 32 insertions(+), 1 deletion(-)
    > 
    > 
    > 
    > diff --git a/lib/stream-windows.c b/lib/stream-windows.c
    > 
    > index 34bc610..0cad927 100644
    > 
    > --- a/lib/stream-windows.c
    > 
    > +++ b/lib/stream-windows.c
    > 
    > @@ -41,7 +41,7 @@ static void maybe_unlink_and_free(char *path);
    > 
    > #define LOCAL_PREFIX "\\\\.\\pipe\\ <file://.//pipe/> "
    > 
    > 
    > 
    > /* Size of the allowed PSIDs for securing Named Pipe. */
    > 
    > -#define ALLOWED_PSIDS_SIZE 2
    > 
    > +#define ALLOWED_PSIDS_SIZE 3
    > 
    > 
    > 
    > /* This function has the purpose to remove all the slashes received in 
    > s. */
    > 
    > static char *
    > 
    > @@ -412,6 +412,9 @@ create_pnpipe(char *name)
    > 
    >      PACL acl = NULL;
    > 
    >      PSECURITY_DESCRIPTOR psd = NULL;
    > 
    >      HANDLE npipe;
    > 
    > +    HANDLE hToken = NULL;
    > 
    > +    DWORD dwBufSize = 0;
    > 
    > +    PTOKEN_USER pTokenUsr = NULL;
    > 
    > 
    > 
    >      /* Disable access over network. */
    > 
    >      if (!AllocateAndInitializeSid(&sia, 1, SECURITY_NETWORK_RID,
    > 
    > @@ -438,6 +441,32 @@ create_pnpipe(char *name)
    > 
    >          goto handle_error;
    > 
    >      }
    > 
    > 
    > 
    > +    /* Open the access token of calling process */
    > 
    > +    if (!OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &hToken)) 
    > + {
    > 
    > +       VLOG_ERR_RL(&rl, "Error opening access token of calling 
    > + process.");
    > 
    > +        goto handle_error;
    > 
    > +    }
    > 
    > +
    > 
    > +    /* get the buffer size buffer needed for SID */
    > 
    > +    GetTokenInformation(hToken, TokenUser, NULL, 0, &dwBufSize);
    > 
    > +
    > 
    > +    pTokenUsr = xmalloc(dwBufSize);
    > 
    > +    memset(pTokenUsr, 0, dwBufSize);
    > 
    > +
    > 
    > +    /* Retrieve the token information in a TOKEN_USER structure. */
    > 
    > +    if (!GetTokenInformation(hToken, TokenUser, pTokenUsr, dwBufSize,
    > 
    > +        &dwBufSize)) {
    > 
    > +        VLOG_ERR_RL(&rl, "Error retrieving token information.");
    > 
    > +        goto handle_error;
    > 
    > +    }
    > 
    > +    CloseHandle(hToken);
    > 
    > +
    > 
    > +    if (!IsValidSid(pTokenUsr->User.Sid)) {
    > 
    > +        VLOG_ERR_RL(&rl, "Invalid SID.");
    > 
    > +        goto handle_error;
    > 
    > +    }
    > 
    > +    allowedPsid[2] = pTokenUsr->User.Sid;
    > 
    > +
    > 
    >      for (int i = 0; i < ALLOWED_PSIDS_SIZE; i++) {
    > 
    >          aclSize += sizeof(ACCESS_ALLOWED_ACE) +
    > 
    >                     GetLengthSid(allowedPsid[i]) -
    > 
    > @@ -490,11 +519,13 @@ create_pnpipe(char *name)
    > 
    >      npipe = CreateNamedPipe(name, PIPE_ACCESS_DUPLEX | 
    > FILE_FLAG_OVERLAPPED,
    > 
    >                              PIPE_TYPE_MESSAGE | PIPE_READMODE_BYTE | 
    > PIPE_WAIT,
    > 
    >                              64, BUFSIZE, BUFSIZE, 0, &sa);
    > 
    > +    free(pTokenUsr);
    > 
    >      free(acl);
    > 
    >      free(psd);
    > 
    >      return npipe;
    > 
    > 
    > 
    > handle_error:
    > 
    > +    free(pTokenUsr);
    > 
    >      free(acl);
    > 
    >      free(psd);
    > 
    >      return INVALID_HANDLE_VALUE;
    > 
    > --
    > 
    > 2.6.2
    > 
    >
Alin Serdean Jan. 24, 2020, 2:52 p.m. UTC | #4
Thank you Ning and Anand.

Applied on master!

Alin.

> -----Original Message-----
> From: Anand Kumar <kumaranand@vmware.com>
> Sent: Friday, January 24, 2020 7:22 AM
> To: Ning Wu <nwu@vmware.com>; Alin Serdean
> <aserdean@cloudbasesolutions.com>; dev@openvswitch.org
> Cc: Lina Li <linali@vmware.com>; Roy Luo <luoroy@vmware.com>
> Subject: Re: [PATCH]lib/stream-windows.c: Grant Access Privilege of Named
> Pipe to Creator
> 
> Acked-by: Anand Kumar <kumaranand@vmware.com>
> 
> Thanks,
> Anand Kumar
> 
> On 1/22/20, 12:19 AM, "Ning Wu" <nwu@vmware.com> wrote:
> 
>     From e42950665acee9aab941b26ebdd067ca0de908a3 Mon Sep 17 00:00:00
> 2001
>     From: Ning Wu <nwu@vmware.com>
>     Date: Tue, 21 Jan 2020 23:46:58 -0800
>     Subject: [PATCH]lib/stream-windows.c: Grant Access Privilege of Named Pipe
> to Creator
> 
>     Current implementation of ovs on windows only allows LocalSystem and
>     Administrators to access the named pipe created with API of ovs.
>     Thus any service that needs to invoke the API to create named pipe
>     has to run as System account to interactive with ovs. It causes the
>     system more vulnerable if one of those services was break into.
>     The patch adds the creator owner account to allowed ACLs.
> 
>     Signed-off-by: Ning Wu <nwu@vmware.com>
>     ---
>      Documentation/ref/ovsdb.7.rst |  3 ++-
>      lib/stream-windows.c          | 33 ++++++++++++++++++++++++++++++++-
>      2 files changed, 34 insertions(+), 2 deletions(-)
> 
>     diff --git a/Documentation/ref/ovsdb.7.rst b/Documentation/ref/ovsdb.7.rst
>     index b1f3f5d..da4dbed 100644
>     --- a/Documentation/ref/ovsdb.7.rst
>     +++ b/Documentation/ref/ovsdb.7.rst
>     @@ -422,7 +422,8 @@ punix:<file>
>          named <file>.
> 
>          On Windows, listens on a local named pipe, creating a named pipe
>     -    <file> to mimic the behavior of a Unix domain socket.
>     +    <file> to mimic the behavior of a Unix domain socket. The ACLs of the
> named
>     +    pipe include LocalSystem, Administrators, and Creator Owner.
> 
>      All IP-based connection methods accept IPv4 and IPv6 addresses.  To specify
> an
>      IPv6 address, wrap it in square brackets, e.g.  ``ssl:[::1]:6640``.  Passive
>     diff --git a/lib/stream-windows.c b/lib/stream-windows.c
>     index 34bc610..5c4c55e 100644
>     --- a/lib/stream-windows.c
>     +++ b/lib/stream-windows.c
>     @@ -41,7 +41,7 @@ static void maybe_unlink_and_free(char *path);
>      #define LOCAL_PREFIX "\\\\.\\pipe\\"
> 
>      /* Size of the allowed PSIDs for securing Named Pipe. */
>     -#define ALLOWED_PSIDS_SIZE 2
>     +#define ALLOWED_PSIDS_SIZE 3
> 
>      /* This function has the purpose to remove all the slashes received in s. */
>      static char *
>     @@ -412,6 +412,9 @@ create_pnpipe(char *name)
>          PACL acl = NULL;
>          PSECURITY_DESCRIPTOR psd = NULL;
>          HANDLE npipe;
>     +    HANDLE hToken = NULL;
>     +    DWORD dwBufSize = 0;
>     +    PTOKEN_USER pTokenUsr = NULL;
> 
>          /* Disable access over network. */
>          if (!AllocateAndInitializeSid(&sia, 1, SECURITY_NETWORK_RID,
>     @@ -438,6 +441,32 @@ create_pnpipe(char *name)
>              goto handle_error;
>          }
> 
>     +    /* Open the access token of calling process */
>     +    if (!OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &hToken)) {
>     +        VLOG_ERR_RL(&rl, "Error opening access token of calling process.");
>     +        goto handle_error;
>     +    }
>     +
>     +    /* get the buffer size buffer needed for SID */
>     +    GetTokenInformation(hToken, TokenUser, NULL, 0, &dwBufSize);
>     +
>     +    pTokenUsr = xmalloc(dwBufSize);
>     +    memset(pTokenUsr, 0, dwBufSize);
>     +
>     +    /* Retrieve the token information in a TOKEN_USER structure. */
>     +    if (!GetTokenInformation(hToken, TokenUser, pTokenUsr, dwBufSize,
>     +        &dwBufSize)) {
>     +        VLOG_ERR_RL(&rl, "Error retrieving token information.");
>     +        goto handle_error;
>     +    }
>     +    CloseHandle(hToken);
>     +
>     +    if (!IsValidSid(pTokenUsr->User.Sid)) {
>     +        VLOG_ERR_RL(&rl, "Invalid SID.");
>     +        goto handle_error;
>     +    }
>     +    allowedPsid[2] = pTokenUsr->User.Sid;
>     +
>          for (int i = 0; i < ALLOWED_PSIDS_SIZE; i++) {
>              aclSize += sizeof(ACCESS_ALLOWED_ACE) +
>                         GetLengthSid(allowedPsid[i]) -
>     @@ -490,11 +519,13 @@ create_pnpipe(char *name)
>          npipe = CreateNamedPipe(name, PIPE_ACCESS_DUPLEX |
> FILE_FLAG_OVERLAPPED,
>                                  PIPE_TYPE_MESSAGE | PIPE_READMODE_BYTE | PIPE_WAIT,
>                                  64, BUFSIZE, BUFSIZE, 0, &sa);
>     +    free(pTokenUsr);
>          free(acl);
>          free(psd);
>          return npipe;
> 
>      handle_error:
>     +    free(pTokenUsr);
>          free(acl);
>          free(psd);
>          return INVALID_HANDLE_VALUE;
>     --
>     2.6.2
> 
>     -----Original Message-----
>     From: Alin Serdean <aserdean@cloudbasesolutions.com>
>     Sent: Wednesday, January 22, 2020 12:19
>     To: Ning Wu <nwu@vmware.com>; dev@openvswitch.org; Anand Kumar
> <kumaranand@vmware.com>
>     Cc: Lina Li <linali@vmware.com>; Roy Luo <luoroy@vmware.com>
>     Subject: RE: [PATCH] Grant Access Privilege of Named Pipe to Creator
> 
>     Hi,
> 
>     Sorry I missed the email.
> 
>     The direction sounds ok with me. It will surely help with unit tests, since right
> now they require elevated permissions.
> 
>     Adding also Anand in the loop.
>     Anand do you like the idea?
> 
>     Please also add a few lines to the documentation so users are aware of the
> change.
> 
>     The patch as is, fails to apply. Rebase on master.
> 
>     Also please change the title so it is in compliance with:
> 
> https://nam04.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdocs.ope
> nvswitch.org%2Fen%2Flatest%2Finternals%2Fcontributing%2Fsubmitting-
> patches%2F%23email-
> subject&amp;data=02%7C01%7Cnwu%40vmware.com%7Cf87a8fa19a3d43832
> 5d008d79ef2400c%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C1%7C637
> 152635616314521&amp;sdata=h73zweLknUd2m9ReaYEe94QNR4wj3q5TilBV8t
> Qmjiw%3D&amp;reserved=0
> 
>     Thanks,
>     Alin.
> 
>     > -----Original Message-----
>     > From: Ning Wu <nwu@vmware.com>
>     > Sent: Monday, January 20, 2020 4:16 AM
>     > To: dev@openvswitch.org; Alin Serdean
>     > <aserdean@cloudbasesolutions.com>
>     > Cc: Lina Li <linali@vmware.com>; Roy Luo <luoroy@vmware.com>
>     > Subject: RE: [PATCH] Grant Access Privilege of Named Pipe to Creator
>     >
>     > Hi Alin,
>     >
>     >
>     >
>     > Could you please help to give some comment?
>     >
>     > Thanks.
>     >
>     >
>     >
>     > From: Ning Wu
>     > Sent: Friday, January 17, 2020 15:15
>     > To: dev@openvswitch.org; Alin Serdean
>     > <aserdean@cloudbasesolutions.com>
>     > Cc: Lina Li <linali@vmware.com>; Roy Luo <luoroy@vmware.com>
>     > Subject: [PATCH] Grant Access Privilege of Named Pipe to Creator
>     >
>     >
>     >
>     > Current implementation of ovs on windows only allows LocalSystem and
>     >
>     > Administrators to access the named pipe created with API of ovs.
>     >
>     > Thus any service that needs to invoke the API to create named pipe
>     >
>     > has to run as System account to interactive with ovs. It causes the
>     >
>     > system more vulnerable if one of those services was break into.
>     >
>     > The patch adds the creator owner account to allowed ACLs.
>     >
>     >
>     >
>     > Signed-off-by: Ning Wu <nwu@vmware.com <mailto:nwu@vmware.com> >
>     >
>     > ---
>     >
>     > lib/stream-windows.c | 33 ++++++++++++++++++++++++++++++++-
>     >
>     > 1 file changed, 32 insertions(+), 1 deletion(-)
>     >
>     >
>     >
>     > diff --git a/lib/stream-windows.c b/lib/stream-windows.c
>     >
>     > index 34bc610..0cad927 100644
>     >
>     > --- a/lib/stream-windows.c
>     >
>     > +++ b/lib/stream-windows.c
>     >
>     > @@ -41,7 +41,7 @@ static void maybe_unlink_and_free(char *path);
>     >
>     > #define LOCAL_PREFIX "\\\\.\\pipe\\ <file://.//pipe/> "
>     >
>     >
>     >
>     > /* Size of the allowed PSIDs for securing Named Pipe. */
>     >
>     > -#define ALLOWED_PSIDS_SIZE 2
>     >
>     > +#define ALLOWED_PSIDS_SIZE 3
>     >
>     >
>     >
>     > /* This function has the purpose to remove all the slashes received in
>     > s. */
>     >
>     > static char *
>     >
>     > @@ -412,6 +412,9 @@ create_pnpipe(char *name)
>     >
>     >      PACL acl = NULL;
>     >
>     >      PSECURITY_DESCRIPTOR psd = NULL;
>     >
>     >      HANDLE npipe;
>     >
>     > +    HANDLE hToken = NULL;
>     >
>     > +    DWORD dwBufSize = 0;
>     >
>     > +    PTOKEN_USER pTokenUsr = NULL;
>     >
>     >
>     >
>     >      /* Disable access over network. */
>     >
>     >      if (!AllocateAndInitializeSid(&sia, 1, SECURITY_NETWORK_RID,
>     >
>     > @@ -438,6 +441,32 @@ create_pnpipe(char *name)
>     >
>     >          goto handle_error;
>     >
>     >      }
>     >
>     >
>     >
>     > +    /* Open the access token of calling process */
>     >
>     > +    if (!OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &hToken))
>     > + {
>     >
>     > +       VLOG_ERR_RL(&rl, "Error opening access token of calling
>     > + process.");
>     >
>     > +        goto handle_error;
>     >
>     > +    }
>     >
>     > +
>     >
>     > +    /* get the buffer size buffer needed for SID */
>     >
>     > +    GetTokenInformation(hToken, TokenUser, NULL, 0, &dwBufSize);
>     >
>     > +
>     >
>     > +    pTokenUsr = xmalloc(dwBufSize);
>     >
>     > +    memset(pTokenUsr, 0, dwBufSize);
>     >
>     > +
>     >
>     > +    /* Retrieve the token information in a TOKEN_USER structure. */
>     >
>     > +    if (!GetTokenInformation(hToken, TokenUser, pTokenUsr, dwBufSize,
>     >
>     > +        &dwBufSize)) {
>     >
>     > +        VLOG_ERR_RL(&rl, "Error retrieving token information.");
>     >
>     > +        goto handle_error;
>     >
>     > +    }
>     >
>     > +    CloseHandle(hToken);
>     >
>     > +
>     >
>     > +    if (!IsValidSid(pTokenUsr->User.Sid)) {
>     >
>     > +        VLOG_ERR_RL(&rl, "Invalid SID.");
>     >
>     > +        goto handle_error;
>     >
>     > +    }
>     >
>     > +    allowedPsid[2] = pTokenUsr->User.Sid;
>     >
>     > +
>     >
>     >      for (int i = 0; i < ALLOWED_PSIDS_SIZE; i++) {
>     >
>     >          aclSize += sizeof(ACCESS_ALLOWED_ACE) +
>     >
>     >                     GetLengthSid(allowedPsid[i]) -
>     >
>     > @@ -490,11 +519,13 @@ create_pnpipe(char *name)
>     >
>     >      npipe = CreateNamedPipe(name, PIPE_ACCESS_DUPLEX |
>     > FILE_FLAG_OVERLAPPED,
>     >
>     >                              PIPE_TYPE_MESSAGE | PIPE_READMODE_BYTE |
>     > PIPE_WAIT,
>     >
>     >                              64, BUFSIZE, BUFSIZE, 0, &sa);
>     >
>     > +    free(pTokenUsr);
>     >
>     >      free(acl);
>     >
>     >      free(psd);
>     >
>     >      return npipe;
>     >
>     >
>     >
>     > handle_error:
>     >
>     > +    free(pTokenUsr);
>     >
>     >      free(acl);
>     >
>     >      free(psd);
>     >
>     >      return INVALID_HANDLE_VALUE;
>     >
>     > --
>     >
>     > 2.6.2
>     >
>     >
> 
>
diff mbox series

Patch

diff --git a/Documentation/ref/ovsdb.7.rst b/Documentation/ref/ovsdb.7.rst
index b1f3f5d..da4dbed 100644
--- a/Documentation/ref/ovsdb.7.rst
+++ b/Documentation/ref/ovsdb.7.rst
@@ -422,7 +422,8 @@  punix:<file>
     named <file>.
 
     On Windows, listens on a local named pipe, creating a named pipe
-    <file> to mimic the behavior of a Unix domain socket.
+    <file> to mimic the behavior of a Unix domain socket. The ACLs of the named
+    pipe include LocalSystem, Administrators, and Creator Owner.
 
 All IP-based connection methods accept IPv4 and IPv6 addresses.  To specify an
 IPv6 address, wrap it in square brackets, e.g.  ``ssl:[::1]:6640``.  Passive
diff --git a/lib/stream-windows.c b/lib/stream-windows.c
index 34bc610..5c4c55e 100644
--- a/lib/stream-windows.c
+++ b/lib/stream-windows.c
@@ -41,7 +41,7 @@  static void maybe_unlink_and_free(char *path);
 #define LOCAL_PREFIX "\\\\.\\pipe\\"
 
 /* Size of the allowed PSIDs for securing Named Pipe. */
-#define ALLOWED_PSIDS_SIZE 2
+#define ALLOWED_PSIDS_SIZE 3
 
 /* This function has the purpose to remove all the slashes received in s. */
 static char *
@@ -412,6 +412,9 @@  create_pnpipe(char *name)
     PACL acl = NULL;
     PSECURITY_DESCRIPTOR psd = NULL;
     HANDLE npipe;
+    HANDLE hToken = NULL;
+    DWORD dwBufSize = 0;
+    PTOKEN_USER pTokenUsr = NULL;
 
     /* Disable access over network. */
     if (!AllocateAndInitializeSid(&sia, 1, SECURITY_NETWORK_RID,
@@ -438,6 +441,32 @@  create_pnpipe(char *name)
         goto handle_error;
     }
 
+    /* Open the access token of calling process */
+    if (!OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &hToken)) {
+        VLOG_ERR_RL(&rl, "Error opening access token of calling process.");
+        goto handle_error;
+    }
+
+    /* get the buffer size buffer needed for SID */
+    GetTokenInformation(hToken, TokenUser, NULL, 0, &dwBufSize);
+
+    pTokenUsr = xmalloc(dwBufSize);
+    memset(pTokenUsr, 0, dwBufSize);
+
+    /* Retrieve the token information in a TOKEN_USER structure. */
+    if (!GetTokenInformation(hToken, TokenUser, pTokenUsr, dwBufSize,
+        &dwBufSize)) {
+        VLOG_ERR_RL(&rl, "Error retrieving token information.");
+        goto handle_error;
+    }
+    CloseHandle(hToken);
+
+    if (!IsValidSid(pTokenUsr->User.Sid)) {
+        VLOG_ERR_RL(&rl, "Invalid SID.");
+        goto handle_error;
+    }
+    allowedPsid[2] = pTokenUsr->User.Sid;
+
     for (int i = 0; i < ALLOWED_PSIDS_SIZE; i++) {
         aclSize += sizeof(ACCESS_ALLOWED_ACE) +
                    GetLengthSid(allowedPsid[i]) -
@@ -490,11 +519,13 @@  create_pnpipe(char *name)
     npipe = CreateNamedPipe(name, PIPE_ACCESS_DUPLEX | FILE_FLAG_OVERLAPPED,
                             PIPE_TYPE_MESSAGE | PIPE_READMODE_BYTE | PIPE_WAIT,
                             64, BUFSIZE, BUFSIZE, 0, &sa);
+    free(pTokenUsr);
     free(acl);
     free(psd);
     return npipe;
 
 handle_error:
+    free(pTokenUsr);
     free(acl);
     free(psd);
     return INVALID_HANDLE_VALUE;