Backport of: From e4a5f60ecbb0248159bc915613359d8f45b49134 Mon Sep 17 00:00:00 2001 From: Frediano Ziglio <freddy77@gmail.com> Date: Sat, 19 Sep 2020 15:13:42 +0100 Subject: [PATCH 02/10] Avoids unchecked file transfer IDs allocation and usage Avoid agents allocating file transfers. The "active_xfers" entries are now inserted when client start sending files. Also different agents cannot mess with other agent transfers as a transfer is bound to a single agent. This issue was reported by SUSE security team. Signed-off-by: Frediano Ziglio <fziglio@redhat.com> Acked-by: Uri Lublin <uril@redhat.com> --- src/vdagentd/vdagentd.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) --- a/src/vdagentd/vdagentd.c +++ b/src/vdagentd/vdagentd.c @@ -345,9 +345,11 @@ static void do_client_file_xfer(struct v s->id, VD_AGENT_FILE_XFER_STATUS_SESSION_LOCKED, NULL, 0); return; } - udscs_write(active_session_conn, VDAGENTD_FILE_XFER_START, 0, 0, - data, message_header->size); - return; + msg_type = VDAGENTD_FILE_XFER_START; + id = s->id; + // associate the id with the active connection + g_hash_table_insert(active_xfers, GUINT_TO_POINTER(id), active_session_conn); + break; } case VD_AGENT_FILE_XFER_STATUS: { VDAgentFileXferStatusMessage *s = (VDAgentFileXferStatusMessage *)data; @@ -372,6 +374,12 @@ static void do_client_file_xfer(struct v return; } udscs_write(conn, msg_type, 0, 0, data, message_header->size); + + // client told that transfer is ended, agents too stop the transfer + // and release resources + if (message_header->type == VD_AGENT_FILE_XFER_STATUS) { + g_hash_table_remove(active_xfers, GUINT_TO_POINTER(id)); + } } static void forward_data_to_session_agent(uint32_t type, uint8_t *data, size_t size) @@ -923,6 +931,15 @@ static void do_agent_file_xfer_status(st const gchar *log_msg = NULL; guint data_size = 0; + struct udscs_connection *task_conn = g_hash_table_lookup(active_xfers, task_id); + if (task_conn == NULL || task_conn != *connp) { + // Protect against misbehaving agent. + // Ignore the message, but do not disconnect the agent, to protect against + // a misbehaving client that tries to disconnect a good agent + // e.g. by sending a new task and immediately cancelling it. + return; + } + /* header->arg1 = file xfer task id, header->arg2 = file xfer status */ switch (header->arg2) { case VD_AGENT_FILE_XFER_STATUS_NOT_ENOUGH_SPACE: @@ -937,10 +954,9 @@ static void do_agent_file_xfer_status(st send_file_xfer_status(virtio_port, log_msg, header->arg1, header->arg2, data, data_size); - if (header->arg2 == VD_AGENT_FILE_XFER_STATUS_CAN_SEND_DATA) - g_hash_table_insert(active_xfers, task_id, *connp); - else + if (header->arg2 != VD_AGENT_FILE_XFER_STATUS_CAN_SEND_DATA) { g_hash_table_remove(active_xfers, task_id); + } } static void agent_read_complete(struct udscs_connection **connp,