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
