[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