[ovs-dev] [PATCH] Windows: Secure the namedpipe implementation

[Ben Pfaff]

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 <vsairam at 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!