aleitner commented on code in PR #454: URL: https://github.com/apache/guacamole-server/pull/454#discussion_r1282360352
########## src/libguac/handle-helpers.c: ########## @@ -0,0 +1,103 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + + +#include "guacamole/error.h" +#include "guacamole/handle-helpers.h" + +#include <errhandlingapi.h> +#include <windef.h> +#include <handleapi.h> +#include <winbase.h> + +#include <stdio.h> + +int guac_read_from_handle( + HANDLE handle, void* buffer, DWORD count, DWORD* num_bytes_read) { + + /* + * Overlapped structure and associated event for waiting on async call. + * The event isn't used by this function, but is required in order to + * reliably wait on an async operation anyway - see + * https://learn.microsoft.com/en-us/windows/win32/api/ioapiset/nf-ioapiset-getoverlappedresult#remarks. + */ + OVERLAPPED overlapped = { 0 }; + overlapped.hEvent = CreateEvent(NULL, FALSE, FALSE, NULL); Review Comment: Should probably add a check for if CreateEvent fails. If it fails, it returns NULL and an error message can be retrieved by calling GetLastError. ########## src/libguac/handle-helpers.c: ########## @@ -0,0 +1,103 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + + +#include "guacamole/error.h" +#include "guacamole/handle-helpers.h" + +#include <errhandlingapi.h> +#include <windef.h> +#include <handleapi.h> +#include <winbase.h> + +#include <stdio.h> + +int guac_read_from_handle( + HANDLE handle, void* buffer, DWORD count, DWORD* num_bytes_read) { + + /* + * Overlapped structure and associated event for waiting on async call. + * The event isn't used by this function, but is required in order to + * reliably wait on an async operation anyway - see + * https://learn.microsoft.com/en-us/windows/win32/api/ioapiset/nf-ioapiset-getoverlappedresult#remarks. + */ + OVERLAPPED overlapped = { 0 }; + overlapped.hEvent = CreateEvent(NULL, FALSE, FALSE, NULL); + + /* Attempt to start the async read operation */ + if (!ReadFile(handle, buffer, count, NULL, &overlapped)) { + + DWORD error = GetLastError(); + + /* + * If an error other than the expected ERROR_IO_PENDING happens, + * return it as the error code immediately. + */ + if (error != ERROR_IO_PENDING) { + return error; + } + + } + + /* + * Wait on the result of the read. If any error occurs when waiting, + * return the error. + */ + if (!GetOverlappedResult(handle, &overlapped, num_bytes_read, TRUE)) + return GetLastError(); + + /* No errors occured, so the read was successful */ + return 0; + +} + +int guac_write_to_handle( + HANDLE handle, const void* buffer, DWORD count, DWORD* num_bytes_written) { + + /* + * Overlapped structure and associated event for waiting on async call. + */ + OVERLAPPED overlapped = { 0 }; + overlapped.hEvent = CreateEvent(NULL, FALSE, FALSE, NULL); Review Comment: Should probably add a check for if CreateEvent fails. If it fails, it returns NULL and an error message can be retrieved by calling GetLastError. ########## src/guacd/connection.c: ########## @@ -181,6 +272,129 @@ void* guacd_connection_io_thread(void* data) { */ static int guacd_add_user(guacd_proc* proc, guac_parser* parser, guac_socket* socket) { +#ifdef CYGWIN_BUILD + + SECURITY_ATTRIBUTES attributes = { 0 }; + attributes.nLength = sizeof(SECURITY_ATTRIBUTES); + + /* + * Attempt to create a Windows security descriptor that grants access only + * to the owner of this process. + */ + if (!ConvertStringSecurityDescriptorToSecurityDescriptor( + + /* + * An SDDL string that uses DACL to grant the General Access (GA) + * permission, only to the owner (OW). For more, see + * https://learn.microsoft.com/en-us/windows/win32/secauthz/security-descriptor-string-format. + */ + "D:P(A;;GA;;;OW)", + SDDL_REVISION_1, + + /* The populated security descriptor output */ + &(attributes.lpSecurityDescriptor), + + /* There's no need to capture the descriptor size */ + NULL + + )) { + guacd_log(GUAC_LOG_ERROR, "Unable to initialize named pipe security descriptor."); + return 1; + } + + char pipe_name[GUAC_PIPE_NAME_LENGTH]; + + /* Required pipe name prefix */ + memcpy(pipe_name, PIPE_NAME_PREFIX, strlen(PIPE_NAME_PREFIX)); + + /* UUID to ensure the pipe name is unique */ + char* uuid = guac_generate_id('G'); + if (uuid == NULL) { + guacd_log(GUAC_LOG_ERROR, "Unable to generate UUID for pipe name."); + return 1; + } + + memcpy(pipe_name + strlen(PIPE_NAME_PREFIX), uuid, GUAC_UUID_LEN); + + /* Null terminator */ + pipe_name[GUAC_PIPE_NAME_LENGTH - 1] = '\0'; + + /* + * Set up a named pipe for communication with the user. For more, see + * https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-createnamedpipea + */ + HANDLE pipe_handle = CreateNamedPipe( Review Comment: `pipe_name` and `attributes` may need validation. I was reading that incorrect or manipulated values might lead to some potential issues. If `pipe_name` is invalid or manipulated, it could lead to pipe squatting attack. Also nonvalidated `attributes` can lead to incorrect permissions being set on the named pipe, resulting in privilege escalation. ########## src/guacd/move-pipe.c: ########## @@ -0,0 +1,108 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "config.h" +#include "log.h" +#include "move-pipe.h" + +#include <errno.h> +#include <fcntl.h> +#include <stdio.h> +#include <string.h> +#include <sys/types.h> +#include <sys/socket.h> +#include <sys/wait.h> +#include <unistd.h> + +#include <guacamole/client-types.h> + +/* Windows headers */ +#include <errhandlingapi.h> +#include <fcntl.h> +#include <handleapi.h> +#include <io.h> +#include <windows.h> + +int guacd_send_pipe(int sock, char* pipe_name) { + + /* Assign data buffer */ + struct iovec io_vector[1]; + io_vector[0].iov_base = pipe_name; + io_vector[0].iov_len = GUAC_PIPE_NAME_LENGTH; + + struct msghdr message = {0}; + message.msg_iov = io_vector; + message.msg_iovlen = 1; + + /* Send pipe name */ + return (sendmsg(sock, &message, 0) == GUAC_PIPE_NAME_LENGTH); Review Comment: Should we check for and report errors from sendmsg? ########## src/libguac/wait-handle.c: ########## @@ -0,0 +1,67 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "config.h" + +#include <stdio.h> + +#include <errhandlingapi.h> +#include <windef.h> +#include <handleapi.h> +#include <winbase.h> + +int guac_wait_for_handle(HANDLE handle, int usec_timeout) { + + HANDLE event = CreateEvent(NULL, FALSE, FALSE, NULL); Review Comment: Should probably add a check for if CreateEvent fails. If it fails, it returns NULL and an error message can be retrieved by calling `GetLastError`. ########## src/guacd/move-pipe.c: ########## @@ -0,0 +1,108 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +#include "config.h" +#include "log.h" +#include "move-pipe.h" + +#include <errno.h> +#include <fcntl.h> +#include <stdio.h> +#include <string.h> +#include <sys/types.h> +#include <sys/socket.h> +#include <sys/wait.h> +#include <unistd.h> + +#include <guacamole/client-types.h> + +/* Windows headers */ +#include <errhandlingapi.h> +#include <fcntl.h> +#include <handleapi.h> +#include <io.h> +#include <windows.h> + +int guacd_send_pipe(int sock, char* pipe_name) { + + /* Assign data buffer */ + struct iovec io_vector[1]; + io_vector[0].iov_base = pipe_name; + io_vector[0].iov_len = GUAC_PIPE_NAME_LENGTH; + + struct msghdr message = {0}; + message.msg_iov = io_vector; + message.msg_iovlen = 1; + + /* Send pipe name */ + return (sendmsg(sock, &message, 0) == GUAC_PIPE_NAME_LENGTH); + +} + +HANDLE guacd_recv_pipe(int sock) { + + /* Assign data buffer */ + char pipe_name[GUAC_PIPE_NAME_LENGTH]; + struct iovec io_vector[1]; + io_vector[0].iov_base = pipe_name; + io_vector[0].iov_len = GUAC_PIPE_NAME_LENGTH; + + struct msghdr message = {0}; + message.msg_iov = io_vector; + message.msg_iovlen = 1; + + /* Receive file descriptor */ + if (recvmsg(sock, &message, 0) == GUAC_PIPE_NAME_LENGTH) { Review Comment: Should we check for and report errors from recvmsg? ########## src/guacd/proc.c: ########## @@ -84,7 +102,11 @@ static void* guacd_user_thread(void* data) { guac_client* client = proc->client; /* Get guac_socket for user's file descriptor */ +#ifdef CYGWIN_BUILD + guac_socket* socket = guac_socket_open_handle(params->handle); +#else guac_socket* socket = guac_socket_open(params->fd); +#endif if (socket == NULL) Review Comment: Could this prevent params from being freed? ########## src/libguac/guacamole/handle-helpers.h: ########## @@ -0,0 +1,78 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + Review Comment: Should we add `#define _GUAC_HANDLER_HELPERS_H` ? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
