From edcdb5552d48deb2de937965d62e0fbe5a180376 Mon Sep 17 00:00:00 2001 From: Tobias Stoeckmann Date: Fri, 7 May 2021 20:12:57 +0200 Subject: [PATCH 1/3] common: handle invalid IPC messages The size of IPC data is stored in an unsigned 32 bit data type within the IPC message header. In order to terminate the received data with a nul byte, one additional byte is allocated. It is not checked if the transmitted size is 2^32 - 1. Adding one more byte would overflow and lead to 0 byte allocation. On 64 bit systems, the recv call with 2^32 - 1 does not fail instantly but reads data from the server into unallocated memory. Prevent override of unallocated memory by aborting communication. Proof of Concept Python server (use 64 bit address sanitized client): ``` import os import socket os.remove('/tmp/sway-poc.socket') server = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) server.bind('/tmp/sway-poc.socket') server.listen(1) print('waiting for client') (client, address) = server.accept() client.send(b'\x69\x33\x2D\x69\x70\x63\xFF\xFF\xFF\xFF\x00\x00\x00\x00\xFF\xFF\xFF\xFF\xFF') input('sent reply, press enter') client.close() ``` --- common/ipc-client.c | 20 ++++++++++++++++---- include/ipc-client.h | 4 ++-- sway/main.c | 2 +- swaybar/ipc.c | 8 ++++---- swaymsg/main.c | 2 +- 5 files changed, 24 insertions(+), 12 deletions(-) diff --git a/common/ipc-client.c b/common/ipc-client.c index d30212d25..78dec010f 100644 --- a/common/ipc-client.c +++ b/common/ipc-client.c @@ -1,4 +1,5 @@ #define _POSIX_C_SOURCE 200809L +#include #include #include #include @@ -94,9 +95,15 @@ struct ipc_response *ipc_recv_response(int socketfd) { goto error_1; } - memcpy(&response->size, data + sizeof(ipc_magic), sizeof(uint32_t)); + uint32_t size; + memcpy(&size, data + sizeof(ipc_magic), sizeof(uint32_t)); + response->size = size; memcpy(&response->type, data + sizeof(ipc_magic) + sizeof(uint32_t), sizeof(uint32_t)); + if (response->size >= SSIZE_MAX) { + sway_abort("Unable to receive overly long IPC response"); + } + char *payload = malloc(response->size + 1); if (!payload) { goto error_2; @@ -126,11 +133,16 @@ void free_ipc_response(struct ipc_response *response) { free(response); } -char *ipc_single_command(int socketfd, uint32_t type, const char *payload, uint32_t *len) { +char *ipc_single_command(int socketfd, uint32_t type, const char *payload, size_t *len) { char data[IPC_HEADER_SIZE]; + + if (*len > UINT32_MAX) { + sway_abort("Unable to send overly long IPC payload"); + } + uint32_t size = *len; memcpy(data, ipc_magic, sizeof(ipc_magic)); - memcpy(data + sizeof(ipc_magic), len, sizeof(*len)); - memcpy(data + sizeof(ipc_magic) + sizeof(*len), &type, sizeof(type)); + memcpy(data + sizeof(ipc_magic), &size, sizeof(size)); + memcpy(data + sizeof(ipc_magic) + sizeof(size), &type, sizeof(type)); if (write(socketfd, data, IPC_HEADER_SIZE) == -1) { sway_abort("Unable to send IPC header"); diff --git a/include/ipc-client.h b/include/ipc-client.h index d3895023f..a8919571c 100644 --- a/include/ipc-client.h +++ b/include/ipc-client.h @@ -12,7 +12,7 @@ * encoded payload string. */ struct ipc_response { - uint32_t size; + size_t size; uint32_t type; char *payload; }; @@ -29,7 +29,7 @@ int ipc_open_socket(const char *socket_path); * Issues a single IPC command and returns the buffer. len will be updated with * the length of the buffer returned from sway. */ -char *ipc_single_command(int socketfd, uint32_t type, const char *payload, uint32_t *len); +char *ipc_single_command(int socketfd, uint32_t type, const char *payload, size_t *len); /** * Receives a single IPC response and returns an ipc_response. */ diff --git a/sway/main.c b/sway/main.c index 0611e80bf..e367ed362 100644 --- a/sway/main.c +++ b/sway/main.c @@ -87,7 +87,7 @@ void detect_proprietary(int allow_unsupported_gpu) { void run_as_ipc_client(char *command, char *socket_path) { int socketfd = ipc_open_socket(socket_path); - uint32_t len = strlen(command); + size_t len = strlen(command); char *resp = ipc_single_command(socketfd, IPC_COMMAND, command, &len); printf("%s\n", resp); free(resp); diff --git a/swaybar/ipc.c b/swaybar/ipc.c index a64aa1abf..3800a1dd5 100644 --- a/swaybar/ipc.c +++ b/swaybar/ipc.c @@ -19,7 +19,7 @@ #include "util.h" void ipc_send_workspace_command(struct swaybar *bar, const char *ws) { - uint32_t size = strlen("workspace \"\"") + strlen(ws); + size_t size = strlen("workspace \"\"") + strlen(ws); for (size_t i = 0; i < strlen(ws); ++i) { if (ws[i] == '"' || ws[i] == '\\') { ++size; @@ -341,7 +341,7 @@ bool ipc_get_workspaces(struct swaybar *bar) { free_workspaces(&output->workspaces); output->focused = false; } - uint32_t len = 0; + size_t len = 0; char *res = ipc_single_command(bar->ipc_socketfd, IPC_GET_WORKSPACES, NULL, &len); json_object *results = json_tokener_parse(res); @@ -409,13 +409,13 @@ bool ipc_get_workspaces(struct swaybar *bar) { void ipc_execute_binding(struct swaybar *bar, struct swaybar_binding *bind) { sway_log(SWAY_DEBUG, "Executing binding for button %u (release=%d): `%s`", bind->button, bind->release, bind->command); - uint32_t len = strlen(bind->command); + size_t len = strlen(bind->command); free(ipc_single_command(bar->ipc_socketfd, IPC_COMMAND, bind->command, &len)); } bool ipc_initialize(struct swaybar *bar) { - uint32_t len = strlen(bar->id); + size_t len = strlen(bar->id); char *res = ipc_single_command(bar->ipc_socketfd, IPC_GET_BAR_CONFIG, bar->id, &len); if (!ipc_parse_config(bar->config, res)) { diff --git a/swaymsg/main.c b/swaymsg/main.c index 574d3b759..993e08f06 100644 --- a/swaymsg/main.c +++ b/swaymsg/main.c @@ -476,7 +476,7 @@ int main(int argc, char **argv) { int socketfd = ipc_open_socket(socket_path); struct timeval timeout = {.tv_sec = 3, .tv_usec = 0}; ipc_set_recv_timeout(socketfd, timeout); - uint32_t len = strlen(command); + size_t len = strlen(command); char *resp = ipc_single_command(socketfd, type, command, &len); // pretty print the json From 06ab0d166a69b69e13a1c0ae6f30706ed8fa98ae Mon Sep 17 00:00:00 2001 From: Tobias Stoeckmann Date: Fri, 7 May 2021 20:20:31 +0200 Subject: [PATCH 2/3] sway: prevent endless loop with broken clients The server loops endlessly in following scenarios: - client sends less bytes than IPC header requires - client sends less bytes than defined by payload size - client sends more payload data than buffered by operating system This happens because the server relies on the buffering in sockets by the operating system. The server only retrieves bytes from buffer when enough bytes are available. To prevent this, store data in heap. Also check supplied payload length before working with that value. Proof of Concept client in Python (you will notice that sway process consumes a lot of CPU while the client is running): ``` import os import socket swaysock=os.environ['SWAYSOCK'] client = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) client.connect(swaysock) client.send(b'\x69\x33\x2D\x69\x70\x63\x00\x00\x00\x00\xFF\xFF\xFF\xFF\x00') input('Press enter to quit.') ``` --- include/ipc.h | 3 + sway/ipc-server.c | 151 +++++++++++++++++++++++++--------------------- 2 files changed, 85 insertions(+), 69 deletions(-) diff --git a/include/ipc.h b/include/ipc.h index ff0117502..285cc7ec7 100644 --- a/include/ipc.h +++ b/include/ipc.h @@ -3,6 +3,9 @@ #define event_mask(ev) (1 << (ev & 0x7F)) +// maximum size of payload is 4 MB +#define IPC_MAX_SIZE 4e6 + enum ipc_command_type { // i3 command types - see i3's I3_REPLY_TYPE constants IPC_COMMAND = 0, diff --git a/sway/ipc-server.c b/sway/ipc-server.c index aad9a7b5b..9cd223b70 100644 --- a/sway/ipc-server.c +++ b/sway/ipc-server.c @@ -48,6 +48,9 @@ struct ipc_client { struct sway_server *server; int fd; enum ipc_command_type subscribed_events; + size_t read_buffer_len; + size_t read_buffer_size; + char *read_buffer; size_t write_buffer_len; size_t write_buffer_size; char *write_buffer; @@ -187,11 +190,23 @@ int ipc_handle_connection(int fd, uint32_t mask, void *data) { client_fd, WL_EVENT_READABLE, ipc_client_handle_readable, client); client->writable_event_source = NULL; + client->read_buffer_size = 128; + client->read_buffer_len = 0; + client->read_buffer = malloc(client->read_buffer_size); + if (!client->read_buffer) { + sway_log(SWAY_ERROR, "Unable to allocate ipc client read buffer"); + free(client); + close(client_fd); + return 0; + } + client->write_buffer_size = 128; client->write_buffer_len = 0; client->write_buffer = malloc(client->write_buffer_size); if (!client->write_buffer) { sway_log(SWAY_ERROR, "Unable to allocate ipc client write buffer"); + free(client->read_buffer); + free(client); close(client_fd); return 0; } @@ -224,49 +239,67 @@ int ipc_client_handle_readable(int client_fd, uint32_t mask, void *data) { ipc_client_disconnect(client); return 0; } + if ((size_t)read_available + 1 > SIZE_MAX - client->read_buffer_len) { + sway_log_errno(SWAY_INFO, "Receiving too much data from IPC client"); + ipc_client_disconnect(client); + return 0; + } - // Wait for the rest of the command payload in case the header has already been read - if (client->pending_length > 0) { - if ((uint32_t)read_available >= client->pending_length) { - // Reset pending values. - uint32_t pending_length = client->pending_length; - enum ipc_command_type pending_type = client->pending_type; - client->pending_length = 0; - ipc_client_handle_command(client, pending_length, pending_type); + if (client->read_buffer_len + read_available >= client->read_buffer_size) { + size_t new_size = client->read_buffer_len + read_available + 1; + char *new_buffer = realloc(client->read_buffer, new_size); + if (new_buffer == NULL) { + sway_log_errno(SWAY_INFO, "Unable to increase read buffer for IPC client"); + ipc_client_disconnect(client); + return 0; } - return 0; + client->read_buffer = new_buffer; + client->read_buffer_size = new_size; } - if (read_available < (int) IPC_HEADER_SIZE) { - return 0; - } - - uint8_t buf[IPC_HEADER_SIZE]; - // Should be fully available, because read_available >= IPC_HEADER_SIZE - ssize_t received = recv(client_fd, buf, IPC_HEADER_SIZE, 0); + ssize_t received = recv(client_fd, client->read_buffer + client->read_buffer_len, read_available, 0); if (received == -1) { sway_log_errno(SWAY_INFO, "Unable to receive header from IPC client"); ipc_client_disconnect(client); return 0; } + client->read_buffer_len += received; - if (memcmp(buf, ipc_magic, sizeof(ipc_magic)) != 0) { - sway_log(SWAY_DEBUG, "IPC header check failed"); - ipc_client_disconnect(client); - return 0; + if (client->pending_length == 0) { + if (client->read_buffer_len < (size_t) IPC_HEADER_SIZE) { + return 0; + } + + if (memcmp(client->read_buffer, ipc_magic, sizeof(ipc_magic)) != 0) { + sway_log(SWAY_DEBUG, "IPC header check failed"); + ipc_client_disconnect(client); + return 0; + } + + memcpy(&client->pending_length, + client->read_buffer + sizeof(ipc_magic), sizeof(uint32_t)); + memcpy(&client->pending_type, + client->read_buffer + sizeof(ipc_magic) + sizeof(uint32_t), sizeof(uint32_t)); + if (client->pending_length > IPC_MAX_SIZE) { + sway_log_errno(SWAY_INFO, "Receiving too much payload from IPC client"); + ipc_client_disconnect(client); + return 0; + } } - memcpy(&client->pending_length, buf + sizeof(ipc_magic), sizeof(uint32_t)); - memcpy(&client->pending_type, buf + sizeof(ipc_magic) + sizeof(uint32_t), sizeof(uint32_t)); - - if (read_available - received >= (long)client->pending_length) { - // Reset pending values. + if (client->read_buffer_len >= IPC_HEADER_SIZE + client->pending_length) { uint32_t pending_length = client->pending_length; enum ipc_command_type pending_type = client->pending_type; - client->pending_length = 0; + char c = client->read_buffer[IPC_HEADER_SIZE + client->pending_length]; + client->read_buffer[IPC_HEADER_SIZE + client->pending_length] = '\0'; ipc_client_handle_command(client, pending_length, pending_type); + // Reset values. + client->read_buffer[IPC_HEADER_SIZE + client->pending_length] = c; + memmove(client->read_buffer, client->read_buffer + IPC_HEADER_SIZE + client->pending_length, + client->read_buffer_len - IPC_HEADER_SIZE - client->pending_length); + client->read_buffer_len -= IPC_HEADER_SIZE + client->pending_length; + client->pending_length = 0; } - return 0; } @@ -572,6 +605,7 @@ void ipc_client_disconnect(struct ipc_client *client) { i++; } list_del(ipc_client_list, i); + free(client->read_buffer); free(client->write_buffer); close(client->fd); free(client); @@ -610,24 +644,7 @@ void ipc_client_handle_command(struct ipc_client *client, uint32_t payload_lengt return; } - char *buf = malloc(payload_length + 1); - if (!buf) { - sway_log_errno(SWAY_INFO, "Unable to allocate IPC payload"); - ipc_client_disconnect(client); - return; - } - if (payload_length > 0) { - // Payload should be fully available - ssize_t received = recv(client->fd, buf, payload_length, 0); - if (received == -1) - { - sway_log_errno(SWAY_INFO, "Unable to receive payload from IPC client"); - ipc_client_disconnect(client); - free(buf); - return; - } - } - buf[payload_length] = '\0'; + char *buf = client->read_buffer + IPC_HEADER_SIZE; switch (payload_type) { case IPC_COMMAND: @@ -654,14 +671,14 @@ void ipc_client_handle_command(struct ipc_client *client, uint32_t payload_lengt list_del(res_list, 0); } list_free(res_list); - goto exit_cleanup; + break; } case IPC_SEND_TICK: { ipc_event_tick(buf); ipc_send_reply(client, payload_type, "{\"success\": true}", 17); - goto exit_cleanup; + break; } case IPC_GET_OUTPUTS: @@ -696,7 +713,7 @@ void ipc_client_handle_command(struct ipc_client *client, uint32_t payload_lengt ipc_send_reply(client, payload_type, json_string, (uint32_t)strlen(json_string)); json_object_put(outputs); // free - goto exit_cleanup; + break; } case IPC_GET_WORKSPACES: @@ -707,7 +724,7 @@ void ipc_client_handle_command(struct ipc_client *client, uint32_t payload_lengt ipc_send_reply(client, payload_type, json_string, (uint32_t)strlen(json_string)); json_object_put(workspaces); // free - goto exit_cleanup; + break; } case IPC_SUBSCRIBE: @@ -718,7 +735,7 @@ void ipc_client_handle_command(struct ipc_client *client, uint32_t payload_lengt const char msg[] = "{\"success\": false}"; ipc_send_reply(client, payload_type, msg, strlen(msg)); sway_log(SWAY_INFO, "Failed to parse subscribe request"); - goto exit_cleanup; + break; } bool is_tick = false; @@ -749,7 +766,7 @@ void ipc_client_handle_command(struct ipc_client *client, uint32_t payload_lengt ipc_send_reply(client, payload_type, msg, strlen(msg)); json_object_put(request); sway_log(SWAY_INFO, "Unsupported event type in subscribe request"); - goto exit_cleanup; + return; } } @@ -761,7 +778,7 @@ void ipc_client_handle_command(struct ipc_client *client, uint32_t payload_lengt ipc_send_reply(client, IPC_EVENT_TICK, tickmsg, strlen(tickmsg)); } - goto exit_cleanup; + break; } case IPC_GET_INPUTS: @@ -775,7 +792,7 @@ void ipc_client_handle_command(struct ipc_client *client, uint32_t payload_lengt ipc_send_reply(client, payload_type, json_string, (uint32_t)strlen(json_string)); json_object_put(inputs); // free - goto exit_cleanup; + break; } case IPC_GET_SEATS: @@ -789,7 +806,7 @@ void ipc_client_handle_command(struct ipc_client *client, uint32_t payload_lengt ipc_send_reply(client, payload_type, json_string, (uint32_t)strlen(json_string)); json_object_put(seats); // free - goto exit_cleanup; + break; } case IPC_GET_TREE: @@ -799,7 +816,7 @@ void ipc_client_handle_command(struct ipc_client *client, uint32_t payload_lengt ipc_send_reply(client, payload_type, json_string, (uint32_t)strlen(json_string)); json_object_put(tree); - goto exit_cleanup; + break; } case IPC_GET_MARKS: @@ -810,7 +827,7 @@ void ipc_client_handle_command(struct ipc_client *client, uint32_t payload_lengt ipc_send_reply(client, payload_type, json_string, (uint32_t)strlen(json_string)); json_object_put(marks); - goto exit_cleanup; + break; } case IPC_GET_VERSION: @@ -820,7 +837,7 @@ void ipc_client_handle_command(struct ipc_client *client, uint32_t payload_lengt ipc_send_reply(client, payload_type, json_string, (uint32_t)strlen(json_string)); json_object_put(version); // free - goto exit_cleanup; + break; } case IPC_GET_BAR_CONFIG: @@ -850,7 +867,7 @@ void ipc_client_handle_command(struct ipc_client *client, uint32_t payload_lengt const char *error = "{ \"success\": false, \"error\": \"No bar with that ID\" }"; ipc_send_reply(client, payload_type, error, (uint32_t)strlen(error)); - goto exit_cleanup; + break; } json_object *json = ipc_json_describe_bar_config(bar); const char *json_string = json_object_to_json_string(json); @@ -858,7 +875,7 @@ void ipc_client_handle_command(struct ipc_client *client, uint32_t payload_lengt (uint32_t)strlen(json_string)); json_object_put(json); // free } - goto exit_cleanup; + break; } case IPC_GET_BINDING_MODES: @@ -872,7 +889,7 @@ void ipc_client_handle_command(struct ipc_client *client, uint32_t payload_lengt ipc_send_reply(client, payload_type, json_string, (uint32_t)strlen(json_string)); json_object_put(modes); // free - goto exit_cleanup; + break; } case IPC_GET_BINDING_STATE: @@ -882,7 +899,7 @@ void ipc_client_handle_command(struct ipc_client *client, uint32_t payload_lengt ipc_send_reply(client, payload_type, json_string, (uint32_t)strlen(json_string)); json_object_put(current_mode); // free - goto exit_cleanup; + break; } case IPC_GET_CONFIG: @@ -893,7 +910,7 @@ void ipc_client_handle_command(struct ipc_client *client, uint32_t payload_lengt ipc_send_reply(client, payload_type, json_string, (uint32_t)strlen(json_string)); json_object_put(json); // free - goto exit_cleanup; + break; } case IPC_SYNC: @@ -901,17 +918,13 @@ void ipc_client_handle_command(struct ipc_client *client, uint32_t payload_lengt // It was decided sway will not support this, just return success:false const char msg[] = "{\"success\": false}"; ipc_send_reply(client, payload_type, msg, strlen(msg)); - goto exit_cleanup; + break; } default: sway_log(SWAY_INFO, "Unknown IPC command type %x", payload_type); - goto exit_cleanup; + break; } - -exit_cleanup: - free(buf); - return; } bool ipc_send_reply(struct ipc_client *client, enum ipc_command_type payload_type, @@ -929,7 +942,7 @@ bool ipc_send_reply(struct ipc_client *client, enum ipc_command_type payload_typ client->write_buffer_size *= 2; } - if (client->write_buffer_size > 4e6) { // 4 MB + if (client->write_buffer_size > IPC_MAX_SIZE) { sway_log(SWAY_ERROR, "Client write buffer too big (%zu), disconnecting client", client->write_buffer_size); ipc_client_disconnect(client); From 3e69928f1332a631345709c72d9161f2d2ea8fbb Mon Sep 17 00:00:00 2001 From: Tobias Stoeckmann Date: Fri, 7 May 2021 21:27:33 +0200 Subject: [PATCH 3/3] common: ipc_single_command can return NULL Especially printf's %s with NULL value is undefined behavior. --- common/ipc-client.c | 12 +++++++++--- sway/main.c | 2 +- swaybar/ipc.c | 5 ++++- swaymsg/main.c | 8 ++++++++ 4 files changed, 22 insertions(+), 5 deletions(-) diff --git a/common/ipc-client.c b/common/ipc-client.c index 78dec010f..088347ebc 100644 --- a/common/ipc-client.c +++ b/common/ipc-client.c @@ -153,9 +153,15 @@ char *ipc_single_command(int socketfd, uint32_t type, const char *payload, size_ } struct ipc_response *resp = ipc_recv_response(socketfd); - char *response = resp->payload; - *len = resp->size; - free(resp); + char *response; + if (resp == NULL) { + response = NULL; + *len = 0; + } else { + response = resp->payload; + *len = resp->size; + free(resp); + } return response; } diff --git a/sway/main.c b/sway/main.c index e367ed362..95b522c51 100644 --- a/sway/main.c +++ b/sway/main.c @@ -89,7 +89,7 @@ void run_as_ipc_client(char *command, char *socket_path) { int socketfd = ipc_open_socket(socket_path); size_t len = strlen(command); char *resp = ipc_single_command(socketfd, IPC_COMMAND, command, &len); - printf("%s\n", resp); + printf("%s\n", resp == NULL ? "(null)" : resp); free(resp); close(socketfd); } diff --git a/swaybar/ipc.c b/swaybar/ipc.c index 3800a1dd5..d5cf6a313 100644 --- a/swaybar/ipc.c +++ b/swaybar/ipc.c @@ -344,6 +344,9 @@ bool ipc_get_workspaces(struct swaybar *bar) { size_t len = 0; char *res = ipc_single_command(bar->ipc_socketfd, IPC_GET_WORKSPACES, NULL, &len); + if (res == NULL) { + return false; + } json_object *results = json_tokener_parse(res); if (!results) { free(res); @@ -418,7 +421,7 @@ bool ipc_initialize(struct swaybar *bar) { size_t len = strlen(bar->id); char *res = ipc_single_command(bar->ipc_socketfd, IPC_GET_BAR_CONFIG, bar->id, &len); - if (!ipc_parse_config(bar->config, res)) { + if (res == NULL || !ipc_parse_config(bar->config, res)) { free(res); return false; } diff --git a/swaymsg/main.c b/swaymsg/main.c index 993e08f06..95f5695fa 100644 --- a/swaymsg/main.c +++ b/swaymsg/main.c @@ -478,6 +478,14 @@ int main(int argc, char **argv) { ipc_set_recv_timeout(socketfd, timeout); size_t len = strlen(command); char *resp = ipc_single_command(socketfd, type, command, &len); + if (resp == NULL) { + if (!quiet) { + sway_log(SWAY_ERROR, "Failed to receive reply"); + } + close(socketfd); + free(socket_path); + return 1; + } // pretty print the json json_object *obj = json_tokener_parse(resp);