I have addressed this and the review comments in v2. Thanks, Sairam
On 5/4/17, 9:33 PM, "Ben Pfaff" <[email protected]> wrote: >Usually if we're taking the size of a variable, it makes sense to really >take the size of that variable, e.g. > sa.nLength = sizeof sa; > >On Fri, May 05, 2017 at 12:40:02AM +0000, Sairam Venugopal wrote: >> Hi Ben, >> >> Regarding the usage of sizeof - I think the current implementation is in >> line >> with the coding style. SECURITY_ATTRIBUTES is a struct and I noticed that we >> currently use sizeof with parenthesis while determining the size of >> structs/types. >> >> Please correct me if am wrong. >> >> Thanks, >> Sairam >> >> >> >> On 5/3/17, 1:05 PM, "Ben Pfaff" <[email protected]> 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 <[email protected]> >> > >> >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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
