Hi Ben, Thanks for taking the time to review this. I will address the coding style changes along with other potential comments in the next version.
Thanks, Sairam On 5/3/17, 1:05 PM, "Ben Pfaff" <b...@ovn.org> wrote: >On Wed, May 03, 2017 at 12:22:48PM -0700, Sairam Venugopal wrote: >> Update the security policies around the creation of the namedpipe. The >> current security updates restrict how the namedpipe can be accessed. >> >> - Disable Network access >> - Windows Services can access the namedpipe >> - Administrators can access the namedpipe >> >> Signed-off-by: Sairam Venugopal <vsai...@vmware.com> > >Hi Sairam, thanks for working on making the Windows code more secure. >It's easy to overlook this kind of thing. > >I don't know much about Windows in this area, so I can't review this, >but I have some style requests. > >For the code here, coding-style.rst says: > Try to avoid casts. Don't cast the return value of malloc(). >Also, xmalloc() never returns NULL, so you need not check for that: >> + acl = (PACL) xmalloc(aclSize); >> + if (acl == NULL) { >> + return INVALID_HANDLE_VALUE; >> + } > >Here, and various other places, coding-style.rst says: > Comments should be written as full sentences that start with a > capital letter and end with a period. Put two spaces between > sentences. >> + /* Add denied ACL */ >> + if (!AddAccessDeniedAce(acl, ACL_REVISION, >> + GENERIC_ALL, remoteAccessSid)) { >> + goto handle_error; >> + } > >Unnecessary cast: >> + psd = (PSECURITY_DESCRIPTOR) xmalloc(SECURITY_DESCRIPTOR_MIN_LENGTH); >> + if (!InitializeSecurityDescriptor(psd, SECURITY_DESCRIPTOR_REVISION)) { >> + goto handle_error; >> + } >> + >> + /* Set DACL */ >> + if (!SetSecurityDescriptorDacl(psd, TRUE, acl, FALSE)) { >> + goto handle_error; >> + } > >Regarding "sizeof", coding-style.rst says: > The "sizeof" operator is unique among C operators in that it accepts > two very different kinds of operands: an expression or a type. In > general, prefer to specify an expression, e.g. ``int *x = > xmalloc(sizeof *\ x);``. When the operand of sizeof is an > expression, there is no need to parenthesize that operand, and > please don't. >> + sa.nLength = sizeof(SECURITY_ATTRIBUTES); >> sa.bInheritHandle = TRUE; >> + sa.lpSecurityDescriptor = psd; >> + >> if (strlen(name) > 256) { >> - VLOG_ERR_RL(&rl, "Named pipe name too long."); >> - return INVALID_HANDLE_VALUE; >> + goto handle_error; >> + } >> + >> + npipe = CreateNamedPipe(name, PIPE_ACCESS_DUPLEX | FILE_FLAG_OVERLAPPED, >> + PIPE_TYPE_MESSAGE | PIPE_READMODE_BYTE | >> PIPE_WAIT, >> + 64, BUFSIZE, BUFSIZE, 0, &sa); >> + free(acl); >> + free(psd); >> + return npipe; >> +handle_error: > >Regarding free(), coding-style.rst says: > Functions that destroy an instance of a dynamically-allocated type > should accept and ignore a null pointer argument. Code that calls > such a function (including the C standard library function > ``free()``) should omit a null-pointer check. We find that this > usually makes code easier to read. >> + if (acl != NULL) { >> + free(acl); >> + } >> + if (psd != NULL) { >> + free(psd); >> } >> - return CreateNamedPipe(name, PIPE_ACCESS_DUPLEX | FILE_FLAG_OVERLAPPED, >> - PIPE_TYPE_MESSAGE | PIPE_READMODE_BYTE | >> PIPE_WAIT, >> - 64, BUFSIZE, BUFSIZE, 0, &sa); >> + return INVALID_HANDLE_VALUE; >> } > >Thanks again for making OVS more secure! _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev