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

Sairam Venugopal vsairam at vmware.com
Wed May 3 21:15:42 UTC 2017


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" <blp at 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 <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!


More information about the dev mailing list