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

Alin Serdean aserdean at cloudbasesolutions.com
Fri Jan 24 14:52:43 UTC 2020


Thank you Ning and Anand.

Applied on master!

Alin.

> -----Original Message-----
> From: Anand Kumar <kumaranand at vmware.com>
> Sent: Friday, January 24, 2020 7:22 AM
> To: Ning Wu <nwu at vmware.com>; Alin Serdean
> <aserdean at cloudbasesolutions.com>; dev at openvswitch.org
> Cc: Lina Li <linali at vmware.com>; Roy Luo <luoroy at vmware.com>
> Subject: Re: [PATCH]lib/stream-windows.c: Grant Access Privilege of Named
> Pipe to Creator
> 
> Acked-by: Anand Kumar <kumaranand at vmware.com>
> 
> Thanks,
> Anand Kumar
> 
> On 1/22/20, 12:19 AM, "Ning Wu" <nwu at vmware.com> wrote:
> 
>     From e42950665acee9aab941b26ebdd067ca0de908a3 Mon Sep 17 00:00:00
> 2001
>     From: Ning Wu <nwu at 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 at 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 at cloudbasesolutions.com>
>     Sent: Wednesday, January 22, 2020 12:19
>     To: Ning Wu <nwu at vmware.com>; dev at openvswitch.org; Anand Kumar
> <kumaranand at vmware.com>
>     Cc: Lina Li <linali at vmware.com>; Roy Luo <luoroy at 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&data=02%7C01%7Cnwu%40vmware.com%7Cf87a8fa19a3d43832
> 5d008d79ef2400c%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C1%7C637
> 152635616314521&sdata=h73zweLknUd2m9ReaYEe94QNR4wj3q5TilBV8t
> Qmjiw%3D&reserved=0
> 
>     Thanks,
>     Alin.
> 
>     > -----Original Message-----
>     > From: Ning Wu <nwu at vmware.com>
>     > Sent: Monday, January 20, 2020 4:16 AM
>     > To: dev at openvswitch.org; Alin Serdean
>     > <aserdean at cloudbasesolutions.com>
>     > Cc: Lina Li <linali at vmware.com>; Roy Luo <luoroy at 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 at openvswitch.org; Alin Serdean
>     > <aserdean at cloudbasesolutions.com>
>     > Cc: Lina Li <linali at vmware.com>; Roy Luo <luoroy at 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 at vmware.com <mailto:nwu at 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
>     >
>     >
> 
> 



More information about the dev mailing list