Thank you Ning and Anand. Applied on master!
Alin. > -----Original Message----- > From: Anand Kumar <kumaran...@vmware.com> > Sent: Friday, January 24, 2020 7:22 AM > To: Ning Wu <n...@vmware.com>; Alin Serdean > <aserd...@cloudbasesolutions.com>; d...@openvswitch.org > Cc: Lina Li <lin...@vmware.com>; Roy Luo <luo...@vmware.com> > Subject: Re: [PATCH]lib/stream-windows.c: Grant Access Privilege of Named > Pipe to Creator > > Acked-by: Anand Kumar <kumaran...@vmware.com> > > Thanks, > Anand Kumar > > On 1/22/20, 12:19 AM, "Ning Wu" <n...@vmware.com> wrote: > > From e42950665acee9aab941b26ebdd067ca0de908a3 Mon Sep 17 00:00:00 > 2001 > From: Ning Wu <n...@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 <n...@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 <aserd...@cloudbasesolutions.com> > Sent: Wednesday, January 22, 2020 12:19 > To: Ning Wu <n...@vmware.com>; d...@openvswitch.org; Anand Kumar > <kumaran...@vmware.com> > Cc: Lina Li <lin...@vmware.com>; Roy Luo <luo...@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 <n...@vmware.com> > > Sent: Monday, January 20, 2020 4:16 AM > > To: d...@openvswitch.org; Alin Serdean > > <aserd...@cloudbasesolutions.com> > > Cc: Lina Li <lin...@vmware.com>; Roy Luo <luo...@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: d...@openvswitch.org; Alin Serdean > > <aserd...@cloudbasesolutions.com> > > Cc: Lina Li <lin...@vmware.com>; Roy Luo <luo...@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 <n...@vmware.com <mailto:n...@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 > > > > > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev