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
