Hello community, here is the log from the commit of package spice-vdagent for openSUSE:Factory checked in at 2020-11-05 21:54:57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ Comparing /work/SRC/openSUSE:Factory/spice-vdagent (Old) and /work/SRC/openSUSE:Factory/.spice-vdagent.new.11331 (New) ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Package is "spice-vdagent" Thu Nov 5 21:54:57 2020 rev:20 rq:846097 version:0.20.0 Changes: -------- --- /work/SRC/openSUSE:Factory/spice-vdagent/spice-vdagent.changes 2020-08-19 18:57:53.259881103 +0200 +++ /work/SRC/openSUSE:Factory/.spice-vdagent.new.11331/spice-vdagent.changes 2020-11-05 21:55:56.796045680 +0100 @@ -1,0 +2,25 @@ +Mon Nov 2 23:11:32 UTC 2020 - Bruce Rogers <[email protected]> + +- Fix multiple security issues as outlined in bsc#1173749 + bsc#1177780 bsc#1177781 bsc#1177782 bsc#1177783 + CVE-2020-25650 CVE-2020-25651 CVE-2020-25652 CVE-2020-25653 + systemd-login-Avoid-a-crash-on-container.patch + vdagentd-Use-bool-for-agent_owns_clipboard-and-clien.patch + vdagentd-Automatically-release-agent_data.patch + vdagent-connection-Pass-err-to-g_credentials_get_uni.patch + vdagentd-Better-check-for-vdagent_connection_get_pee.patch + vdagentd-Avoid-calling-chmod.patch + Avoids-unchecked-file-transfer-IDs-allocation-and-us.patch + Avoids-uncontrolled-active_xfers-allocations.patch + Avoids-unlimited-agent-connections.patch + Avoids-user-session-hijacking.patch + Better-check-for-sessions.patch + vdagentd-Limit-number-of-agents-per-session-to-1.patch + cleanup-active_xfers-when-the-client-disconnects.patch + vdagentd-do-not-allow-to-use-an-already-used-file-xf.patch + Add-a-test-for-session_info.patch +- Add a check section to run internal tests. Note that by default + the added session_info test is not run, as it doesn't work in + context of build service + +------------------------------------------------------------------- New: ---- Add-a-test-for-session_info.patch Avoids-unchecked-file-transfer-IDs-allocation-and-us.patch Avoids-uncontrolled-active_xfers-allocations.patch Avoids-unlimited-agent-connections.patch Avoids-user-session-hijacking.patch Better-check-for-sessions.patch cleanup-active_xfers-when-the-client-disconnects.patch systemd-login-Avoid-a-crash-on-container.patch vdagent-connection-Pass-err-to-g_credentials_get_uni.patch vdagentd-Automatically-release-agent_data.patch vdagentd-Avoid-calling-chmod.patch vdagentd-Better-check-for-vdagent_connection_get_pee.patch vdagentd-Limit-number-of-agents-per-session-to-1.patch vdagentd-Use-bool-for-agent_owns_clipboard-and-clien.patch vdagentd-do-not-allow-to-use-an-already-used-file-xf.patch ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ Other differences: ------------------ ++++++ spice-vdagent.spec ++++++ --- /var/tmp/diff_new_pack.TI6hyb/_old 2020-11-05 21:55:58.240042434 +0100 +++ /var/tmp/diff_new_pack.TI6hyb/_new 2020-11-05 21:55:58.244042424 +0100 @@ -17,6 +17,9 @@ # +#This test doesn't work right in build service, but does outside of it +%bcond_with session_info_test + Name: spice-vdagent Version: 0.20.0 Release: 0 @@ -29,6 +32,22 @@ Source2: %{name}.keyring Patch1: vdagentd-work-around-GLib-s-fork-issues.patch Patch2: vdagentd-init-static-uinput-before-fork.patch +Patch3: systemd-login-Avoid-a-crash-on-container.patch +Patch4: vdagentd-Use-bool-for-agent_owns_clipboard-and-clien.patch +Patch5: vdagentd-Automatically-release-agent_data.patch +Patch6: vdagent-connection-Pass-err-to-g_credentials_get_uni.patch +Patch7: vdagentd-Better-check-for-vdagent_connection_get_pee.patch +Patch8: vdagentd-Avoid-calling-chmod.patch +Patch9: Avoids-unchecked-file-transfer-IDs-allocation-and-us.patch +Patch10: Avoids-uncontrolled-active_xfers-allocations.patch +Patch11: Avoids-unlimited-agent-connections.patch +Patch12: Avoids-user-session-hijacking.patch +Patch13: Better-check-for-sessions.patch +Patch14: vdagentd-Limit-number-of-agents-per-session-to-1.patch +Patch15: cleanup-active_xfers-when-the-client-disconnects.patch +Patch16: vdagentd-do-not-allow-to-use-an-already-used-file-xf.patch +Patch17: Add-a-test-for-session_info.patch + BuildRequires: alsa-devel >= 1.0.22 BuildRequires: desktop-file-utils BuildRequires: libXfixes-devel @@ -65,13 +84,34 @@ %setup -q %patch1 -p1 %patch2 -p1 +%patch3 -p1 +%patch4 -p1 +%patch5 -p1 +%patch6 -p1 +%patch7 -p1 +%patch8 -p1 +%patch9 -p1 +%patch10 -p1 +%patch11 -p1 +%patch12 -p1 +%patch13 -p1 +%patch14 -p1 +%patch15 -p1 +%patch16 -p1 +%if %{with session_info_test} +%patch17 -p1 +%endif %build +autoreconf %configure \ --with-session-info=systemd \ --with-init-script=systemd make %{?_smp_mflags} V=2 +%check +make check V=2 + %install make install DESTDIR=%{buildroot} V=2 # create rc symlink ++++++ Add-a-test-for-session_info.patch ++++++ >From a77b09e5d77e5cf1d4fdd38d018ecf164cd01df9 Mon Sep 17 00:00:00 2001 From: Frediano Ziglio <[email protected]> Date: Tue, 20 Oct 2020 16:38:37 +0100 Subject: [PATCH 10/10] Add a test for session_info References: bsc#1173749 Test from Uri, integrated in test suite. Signed-off-by: Uri Lublin <[email protected]> Signed-off-by: Frediano Ziglio <[email protected]> --- Makefile.am | 30 ++++++++++++++++++++ tests/test-session-info.c | 58 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+) create mode 100644 tests/test-session-info.c diff --git a/Makefile.am b/Makefile.am index 575ba52..f4c65b4 100644 --- a/Makefile.am +++ b/Makefile.am @@ -109,13 +109,43 @@ src_spice_vdagentd_SOURCES = \ src/vdagentd/virtio-port.h \ $(NULL) +tests_test_session_info_CFLAGS = \ + $(DBUS_CFLAGS) \ + $(LIBSYSTEMD_DAEMON_CFLAGS) \ + $(LIBSYSTEMD_LOGIN_CFLAGS) \ + $(SPICE_CFLAGS) \ + $(GIO2_CFLAGS) \ + -I$(srcdir)/src \ + -I$(srcdir)/src/vdagentd \ + -DUDSCS_NO_SERVER \ + $(NULL) + +tests_test_session_info_LDADD = \ + $(DBUS_LIBS) \ + $(LIBSYSTEMD_DAEMON_LIBS) \ + $(LIBSYSTEMD_LOGIN_LIBS) \ + $(SPICE_LIBS) \ + $(GIO2_LIBS) \ + $(NULL) + +tests_test_session_info_SOURCES = \ + $(common_sources) \ + src/vdagentd/session-info.h \ + tests/test-session-info.c \ + $(NULL) + +check_PROGRAMS += tests/test-session-info + if HAVE_CONSOLE_KIT src_spice_vdagentd_SOURCES += src/vdagentd/console-kit.c +tests_test_session_info_SOURCES += src/vdagentd/console-kit.c else if HAVE_LIBSYSTEMD_LOGIN src_spice_vdagentd_SOURCES += src/vdagentd/systemd-login.c +tests_test_session_info_SOURCES += src/vdagentd/systemd-login.c else src_spice_vdagentd_SOURCES += src/vdagentd/dummy-session-info.c +tests_test_session_info_SOURCES += src/vdagentd/dummy-session-info.c endif endif diff --git a/tests/test-session-info.c b/tests/test-session-info.c new file mode 100644 index 0000000..dae3ec6 --- /dev/null +++ b/tests/test-session-info.c @@ -0,0 +1,58 @@ +/* test-session-info.c - test session info + + Copyright 2020 Red Hat, Inc. + + This program is free software: you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation, either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. +*/ +#include <config.h> + +#undef NDEBUG +#include <assert.h> +#include <stdio.h> +#include <stdlib.h> +#include <unistd.h> + +#include "session-info.h" + +int main(int argc, char *argv[]) +{ + int pid, uid, ck_uid; + + pid = (int)getpid(); + + struct session_info *session_info = session_info_create(1); + if (session_info == NULL) { + return 1; + } + + char *session = session_info_session_for_pid(session_info, pid); + if (session == NULL) { + session_info_destroy(session_info); + return 2; + } + ck_uid = session_info_uid_for_session(session_info, session); + + free(session); + session_info_destroy(session_info); + + uid = getuid(); + printf("MAIN: uid is %d, ck_uid is %d\n", uid, ck_uid); + + if (uid != ck_uid) { + fprintf(stderr, "MAIN: uid (%d) does not match console-kit uid %d\n", uid, ck_uid); + return 3; + } + + return 0; +} -- 2.28.0 ++++++ Avoids-unchecked-file-transfer-IDs-allocation-and-us.patch ++++++ >From e4a5f60ecbb0248159bc915613359d8f45b49134 Mon Sep 17 00:00:00 2001 From: Frediano Ziglio <[email protected]> Date: Sat, 19 Sep 2020 15:13:42 +0100 Subject: [PATCH 02/10] Avoids unchecked file transfer IDs allocation and usage References: bsc#1173749 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 <[email protected]> Acked-by: Uri Lublin <[email protected]> Signed-off-by: Bruce Rogers <[email protected]> --- src/vdagentd/vdagentd.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c index a2b74bb..f15989d 100644 --- a/src/vdagentd/vdagentd.c +++ b/src/vdagentd/vdagentd.c @@ -381,9 +381,11 @@ static void do_client_file_xfer(VirtioPort *vport, 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; @@ -408,6 +410,12 @@ static void do_client_file_xfer(VirtioPort *vport, 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) @@ -1012,6 +1020,15 @@ static void do_agent_file_xfer_status(UdscsConnection *conn, const gchar *log_msg = NULL; guint data_size = 0; + UdscsConnection *task_conn = g_hash_table_lookup(active_xfers, task_id); + if (task_conn == NULL || task_conn != conn) { + // 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: @@ -1026,10 +1043,9 @@ static void do_agent_file_xfer_status(UdscsConnection *conn, 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, conn); - 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(UdscsConnection *conn, -- 2.28.0 ++++++ Avoids-uncontrolled-active_xfers-allocations.patch ++++++ >From 53a5266e90ea09a5522f5ed867150a75c74052c3 Mon Sep 17 00:00:00 2001 From: Frediano Ziglio <[email protected]> Date: Fri, 2 Oct 2020 12:27:59 +0100 Subject: [PATCH 03/10] Avoids uncontrolled "active_xfers" allocations References: bsc#1173749 Limit the number of active file transfers possibly causing DoSes consuming memory in "active_xfers". This issue was reported by SUSE security team. Signed-off-by: Frediano Ziglio <[email protected]> Acked-by: Uri Lublin <[email protected]> Signed-off-by: Bruce Rogers <[email protected]> --- src/vdagentd/vdagentd.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c index f15989d..8462889 100644 --- a/src/vdagentd/vdagentd.c +++ b/src/vdagentd/vdagentd.c @@ -47,6 +47,14 @@ #define DEFAULT_UINPUT_DEVICE "/dev/uinput" +// Maximum number of transfers active at any time. +// Avoid DoS from client. +// As each transfer could likely end up taking a file descriptor +// it is good to have a limit less than the number of file descriptors +// in the process (by default 1024). The daemon do not open file +// descriptors for the transfers but the agents do. +#define MAX_ACTIVE_TRANSFERS 128 + struct agent_data { char *session; int width; @@ -380,6 +388,21 @@ static void do_client_file_xfer(VirtioPort *vport, "Cancelling client file-xfer request %u", s->id, VD_AGENT_FILE_XFER_STATUS_SESSION_LOCKED, NULL, 0); return; + } else if (g_hash_table_size(active_xfers) >= MAX_ACTIVE_TRANSFERS) { + VDAgentFileXferStatusError error = { + GUINT32_TO_LE(VD_AGENT_FILE_XFER_STATUS_ERROR_GLIB_IO), + GUINT32_TO_LE(G_IO_ERROR_TOO_MANY_OPEN_FILES), + }; + size_t detail_size = sizeof(error); + if (!VD_AGENT_HAS_CAPABILITY(capabilities, capabilities_size, + VD_AGENT_CAP_FILE_XFER_DETAILED_ERRORS)) { + detail_size = 0; + } + send_file_xfer_status(vport, + "Too many transfers ongoing. " + "Cancelling client file-xfer request %u", + s->id, VD_AGENT_FILE_XFER_STATUS_ERROR, (void*) &error, detail_size); + return; } msg_type = VDAGENTD_FILE_XFER_START; id = s->id; -- 2.28.0 ++++++ Avoids-unlimited-agent-connections.patch ++++++ >From e0ba1d640e2cac883ed1a0065aaea8764501b07c Mon Sep 17 00:00:00 2001 From: Frediano Ziglio <[email protected]> Date: Sun, 20 Sep 2020 08:05:37 +0100 Subject: [PATCH 04/10] Avoids unlimited agent connections References: bsc#1173749 Limit the number of agents that can be connected. Avoids reaching the maximum number of files in a process. Beside one file descriptor per agent the daemon open just some other fixed number of files. This issue was reported by SUSE security team. Signed-off-by: Frediano Ziglio <[email protected]> Signed-off-by: Bruce Rogers <[email protected]> --- src/udscs.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/udscs.c b/src/udscs.c index 7c99eed..3df67b3 100644 --- a/src/udscs.c +++ b/src/udscs.c @@ -30,6 +30,12 @@ #include "vdagentd-proto-strings.h" #include "vdagent-connection.h" +// Maximum number of connected agents. +// Avoid DoS from agents. +// As each connection end up taking a file descriptor is good to have a limit +// less than the number of file descriptors in the process (by default 1024). +#define MAX_CONNECTED_AGENTS 128 + struct _UdscsConnection { VDAgentConnection parent_instance; int debug; @@ -254,6 +260,12 @@ static gboolean udscs_server_accept_cb(GSocketService *service, struct udscs_server *server = user_data; UdscsConnection *new_conn; + /* prevents DoS having too many agents attached */ + if (g_list_length(server->connections) >= MAX_CONNECTED_AGENTS) { + syslog(LOG_ERR, "Too many agents connected"); + return TRUE; + } + new_conn = g_object_new(UDSCS_TYPE_CONNECTION, NULL); new_conn->debug = server->debug; new_conn->read_callback = server->read_callback; -- 2.28.0 ++++++ Avoids-user-session-hijacking.patch ++++++ >From 9d011f9787e93e3ceb5541ddcb242f5fba9ac2e6 Mon Sep 17 00:00:00 2001 From: Frediano Ziglio <[email protected]> Date: Sun, 20 Sep 2020 08:06:16 +0100 Subject: [PATCH 05/10] Avoids user session hijacking References: bsc#1173749 Avoids user hijacking sessions by reusing PID. In theory an attacker could: - open a connection to the daemon; - fork and exit the process but keep the file descriptor open (inheriting or duplicating it in forked process); - force OS to recycle the initial PID, by creating many short lived processes. Daemon would detect the old PID as having the new session. Check the user to avoid such replacements. This issue was reported by SUSE security team. Signed-off-by: Frediano Ziglio <[email protected]> Acked-by: Uri Lublin <[email protected]> Signed-off-by: Bruce Rogers <[email protected]> --- src/vdagent-connection.c | 13 +++++++------ src/vdagent-connection.h | 13 +++++++++---- src/vdagentd/vdagentd.c | 31 +++++++++++++++++++++++++++---- 3 files changed, 43 insertions(+), 14 deletions(-) diff --git a/src/vdagent-connection.c b/src/vdagent-connection.c index ff8b88d..8e47e79 100644 --- a/src/vdagent-connection.c +++ b/src/vdagent-connection.c @@ -142,24 +142,25 @@ void vdagent_connection_destroy(gpointer p) g_object_unref(self); } -gint vdagent_connection_get_peer_pid(VDAgentConnection *self, - GError **err) +PidUid vdagent_connection_get_peer_pid_uid(VDAgentConnection *self, + GError **err) { VDAgentConnectionPrivate *priv = vdagent_connection_get_instance_private(self); GSocket *sock; GCredentials *cred; - gint pid = -1; + PidUid pid_uid = { -1, -1 }; - g_return_val_if_fail(G_IS_SOCKET_CONNECTION(priv->io_stream), pid); + g_return_val_if_fail(G_IS_SOCKET_CONNECTION(priv->io_stream), pid_uid); sock = g_socket_connection_get_socket(G_SOCKET_CONNECTION(priv->io_stream)); cred = g_socket_get_credentials(sock, err); if (cred) { - pid = g_credentials_get_unix_pid(cred, err); + pid_uid.pid = g_credentials_get_unix_pid(cred, err); + pid_uid.uid = g_credentials_get_unix_user(cred, err); g_object_unref(cred); } - return pid; + return pid_uid; } /* Performs single write operation, diff --git a/src/vdagent-connection.h b/src/vdagent-connection.h index 9d5a212..c515a79 100644 --- a/src/vdagent-connection.h +++ b/src/vdagent-connection.h @@ -92,10 +92,15 @@ void vdagent_connection_write(VDAgentConnection *self, /* Synchronously write all queued messages to the output stream. */ void vdagent_connection_flush(VDAgentConnection *self); -/* Returns the PID of the foreign process connected to the socket - * or -1 with @err set. */ -gint vdagent_connection_get_peer_pid(VDAgentConnection *self, - GError **err); +typedef struct PidUid { + pid_t pid; + uid_t uid; +} PidUid; + +/* Returns the PID and UID of the foreign process connected to the socket + * or fill @err set. */ +PidUid vdagent_connection_get_peer_pid_uid(VDAgentConnection *self, + GError **err); G_END_DECLS diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c index 8462889..fc22338 100644 --- a/src/vdagentd/vdagentd.c +++ b/src/vdagentd/vdagentd.c @@ -952,16 +952,28 @@ static gboolean remove_active_xfers(gpointer key, gpointer value, gpointer conn) return 0; } +/* Check a given process has a given UID */ +static bool check_uid_of_pid(pid_t pid, uid_t uid) +{ + char fn[128]; + struct stat st; + + snprintf(fn, sizeof(fn), "/proc/%u/status", (unsigned) pid); + if (stat(fn, &st) != 0 || st.st_uid != uid) { + return false; + } + return true; +} + static void agent_connect(UdscsConnection *conn) { struct agent_data *agent_data; agent_data = g_new0(struct agent_data, 1); GError *err = NULL; - gint pid; if (session_info) { - pid = vdagent_connection_get_peer_pid(VDAGENT_CONNECTION(conn), &err); - if (err || pid <= 0) { + PidUid pid_uid = vdagent_connection_get_peer_pid_uid(VDAGENT_CONNECTION(conn), &err); + if (err || pid_uid.pid <= 0) { static const char msg[] = "Could not get peer PID, disconnecting new client"; if (err) { syslog(LOG_ERR, "%s: %s", msg, err->message); @@ -974,7 +986,18 @@ static void agent_connect(UdscsConnection *conn) return; } - agent_data->session = session_info_session_for_pid(session_info, pid); + agent_data->session = session_info_session_for_pid(session_info, pid_uid.pid); + + /* Check that the UID of the PID did not change, this should be done after + * computing the session to avoid race conditions. + * This can happen as vdagent_connection_get_peer_pid_uid get information + * from the time of creating the socket, but the process in the meantime + * have been replaced */ + if (!check_uid_of_pid(pid_uid.pid, pid_uid.uid)) { + agent_data_destroy(agent_data); + udscs_server_destroy_connection(server, conn); + return; + } } g_object_set_data_full(G_OBJECT(conn), "agent_data", agent_data, -- 2.28.0 ++++++ Better-check-for-sessions.patch ++++++ >From 438aaef5f308b6c0a8bd184db776d415ededcc73 Mon Sep 17 00:00:00 2001 From: Frediano Ziglio <[email protected]> Date: Mon, 21 Sep 2020 07:06:09 +0100 Subject: [PATCH 06/10] Better check for sessions References: bsc#1173749 Do not allow other users to hijack a session checking that the process is launched by the owner of the session. Signed-off-by: Frediano Ziglio <[email protected]> Acked-by: Uri Lublin <[email protected]> Signed-off-by: Bruce Rogers <[email protected]> --- src/vdagentd/console-kit.c | 67 +++++++++++++++++++++++++++++++ src/vdagentd/dummy-session-info.c | 5 +++ src/vdagentd/session-info.h | 3 ++ src/vdagentd/systemd-login.c | 9 +++++ src/vdagentd/vdagentd.c | 10 ++++- 5 files changed, 93 insertions(+), 1 deletion(-) diff --git a/src/vdagentd/console-kit.c b/src/vdagentd/console-kit.c index fcdb0b6..77429bc 100644 --- a/src/vdagentd/console-kit.c +++ b/src/vdagentd/console-kit.c @@ -568,3 +568,70 @@ exit: } return ret; } + +uid_t session_info_uid_for_session(struct session_info *info, const char *session) +{ + DBusError error; + DBusMessage *message = NULL; + DBusMessage *reply = NULL; + uint32_t uid; + uid_t ret = -1; + const char *err_msg; + + g_return_val_if_fail(info != NULL, ret); + g_return_val_if_fail(info->connection != NULL, ret); + g_return_val_if_fail(info->active_session != NULL, ret); + + dbus_error_init(&error); + + err_msg = "(console-kit) Unable to create dbus message for GetUnixUser"; + message = dbus_message_new_method_call(INTERFACE_CONSOLE_KIT, + session, + INTERFACE_CONSOLE_KIT_SESSION, + "GetUnixUser"); + if (message == NULL) { + goto exit; + } + + err_msg = "(console-kit) GetUnixUser failed"; + reply = dbus_connection_send_with_reply_and_block(info->connection, + message, + -1, + &error); + if (reply == NULL || dbus_error_is_set(&error)) { + goto exit; + } + + dbus_error_init(&error); + err_msg = "(console-kit) fail to get session-type from reply"; + if (!dbus_message_get_args(reply, + &error, + DBUS_TYPE_UINT32, &uid, + DBUS_TYPE_INVALID)) { + goto exit; + } + + if (info->verbose) { + syslog(LOG_DEBUG, "(console-kit) unix user is '%u'", (unsigned) uid); + } + + err_msg = NULL; + ret = uid; + +exit: + if (err_msg) { + if (dbus_error_is_set(&error)) { + syslog(LOG_ERR, "%s: %s", err_msg, error.message); + dbus_error_free(&error); + } else { + syslog(LOG_ERR, "%s", err_msg); + } + } + if (reply != NULL) { + dbus_message_unref(reply); + } + if (message != NULL) { + dbus_message_unref(message); + } + return ret; +} diff --git a/src/vdagentd/dummy-session-info.c b/src/vdagentd/dummy-session-info.c index 7fd1eea..137c01a 100644 --- a/src/vdagentd/dummy-session-info.c +++ b/src/vdagentd/dummy-session-info.c @@ -55,3 +55,8 @@ gboolean session_info_session_is_locked(G_GNUC_UNUSED struct session_info *si) { return FALSE; } + +uid_t session_info_uid_for_session(struct session_info *si, const char *session) +{ + return -1; +} diff --git a/src/vdagentd/session-info.h b/src/vdagentd/session-info.h index c8edb86..96aa8d3 100644 --- a/src/vdagentd/session-info.h +++ b/src/vdagentd/session-info.h @@ -40,4 +40,7 @@ char *session_info_session_for_pid(struct session_info *ck, uint32_t pid); gboolean session_info_session_is_locked(struct session_info *si); gboolean session_info_is_user(struct session_info *si); +/* get owner of a given session */ +uid_t session_info_uid_for_session(struct session_info *si, const char *session); + #endif diff --git a/src/vdagentd/systemd-login.c b/src/vdagentd/systemd-login.c index 2d2311c..42ccc5f 100644 --- a/src/vdagentd/systemd-login.c +++ b/src/vdagentd/systemd-login.c @@ -394,3 +394,12 @@ gboolean session_info_is_user(struct session_info *si) return ret; } + +uid_t session_info_uid_for_session(struct session_info *si, const char *session) +{ + uid_t ret = -1; + if (sd_session_get_uid(session, &ret) < 0) { + return -1; + } + return ret; +} diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c index fc22338..59aa523 100644 --- a/src/vdagentd/vdagentd.c +++ b/src/vdagentd/vdagentd.c @@ -988,12 +988,20 @@ static void agent_connect(UdscsConnection *conn) agent_data->session = session_info_session_for_pid(session_info, pid_uid.pid); + uid_t session_uid = session_info_uid_for_session(session_info, agent_data->session); + /* Check that the UID of the PID did not change, this should be done after * computing the session to avoid race conditions. * This can happen as vdagent_connection_get_peer_pid_uid get information * from the time of creating the socket, but the process in the meantime * have been replaced */ - if (!check_uid_of_pid(pid_uid.pid, pid_uid.uid)) { + if (!check_uid_of_pid(pid_uid.pid, pid_uid.uid) || + /* Check that the user launching the Agent is the same as session one + * or root user. + * This prevents session hijacks from other users. */ + (pid_uid.uid != 0 && pid_uid.uid != session_uid)) { + syslog(LOG_ERR, "UID mismatch: UID=%u PID=%u suid=%u", pid_uid.uid, + pid_uid.pid, session_uid); agent_data_destroy(agent_data); udscs_server_destroy_connection(server, conn); return; -- 2.28.0 ++++++ cleanup-active_xfers-when-the-client-disconnects.patch ++++++ >From 2e7760735696beab903d14bfe17202d60dd3b043 Mon Sep 17 00:00:00 2001 From: Uri Lublin <[email protected]> Date: Wed, 7 Oct 2020 19:34:57 +0300 Subject: [PATCH 08/10] cleanup active_xfers when the client disconnects References: bsc#1173749 Signed-off-by: Uri Lublin <[email protected]> Acked-by: Frediano Ziglio <[email protected]> Signed-off-by: Bruce Rogers <[email protected]> --- src/vdagentd/vdagentd.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c index 92885b5..8437779 100644 --- a/src/vdagentd/vdagentd.c +++ b/src/vdagentd/vdagentd.c @@ -168,6 +168,7 @@ static void send_capabilities(VirtioPort *vport, static void do_client_disconnect(void) { + g_hash_table_remove_all(active_xfers); if (client_connected) { udscs_server_write_all(server, VDAGENTD_CLIENT_DISCONNECTED, 0, 0, NULL, 0); -- 2.28.0 ++++++ systemd-login-Avoid-a-crash-on-container.patch ++++++ >From 5654f4d2f90f95efd1f0ca70b438a3ab81022d15 Mon Sep 17 00:00:00 2001 From: Frediano Ziglio <[email protected]> Date: Thu, 26 Mar 2020 11:31:50 +0000 Subject: [PATCH] systemd-login: Avoid a crash on container References: bsc#1173749 On containers dbus could be not running. In this case dbus.system_connection is NULL and calling dbus_connection_close on it will cause a crash. This happens also under Gitlab CI. Signed-off-by: Frediano Ziglio <[email protected]> Acked-by: Victor Toso <[email protected]> --- src/vdagentd/systemd-login.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/vdagentd/systemd-login.c b/src/vdagentd/systemd-login.c index 0b8f3c1..2d2311c 100644 --- a/src/vdagentd/systemd-login.c +++ b/src/vdagentd/systemd-login.c @@ -250,7 +250,9 @@ void session_info_destroy(struct session_info *si) return; si_dbus_match_remove(si); - dbus_connection_close(si->dbus.system_connection); + if (si->dbus.system_connection) { + dbus_connection_close(si->dbus.system_connection); + } sd_login_monitor_unref(si->mon); g_free(si->session); g_free(si); -- 2.29.0 ++++++ vdagent-connection-Pass-err-to-g_credentials_get_uni.patch ++++++ >From b894975bedb2b9e01385261183db19f7d0642292 Mon Sep 17 00:00:00 2001 From: Frediano Ziglio <[email protected]> Date: Tue, 22 Sep 2020 11:45:56 +0100 Subject: [PATCH] vdagent-connection: Pass "err" to g_credentials_get_unix_pid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Git-commit: b894975bedb2b9e01385261183db19f7d0642292 References: bsc#1173749 Allows to return detailed information if g_credentials_get_unix_pid fails. Signed-off-by: Frediano Ziglio <[email protected]> Acked-by: Julien Ropé <[email protected]> Signed-off-by: Bruce Rogers <[email protected]> --- src/vdagent-connection.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vdagent-connection.c b/src/vdagent-connection.c index ede784b..ff8b88d 100644 --- a/src/vdagent-connection.c +++ b/src/vdagent-connection.c @@ -155,7 +155,7 @@ gint vdagent_connection_get_peer_pid(VDAgentConnection *self, sock = g_socket_connection_get_socket(G_SOCKET_CONNECTION(priv->io_stream)); cred = g_socket_get_credentials(sock, err); if (cred) { - pid = g_credentials_get_unix_pid(cred, NULL); + pid = g_credentials_get_unix_pid(cred, err); g_object_unref(cred); } -- 2.29.0 ++++++ vdagentd-Automatically-release-agent_data.patch ++++++ >From b4479eb9b0a30cf99806a17bcbdc468828727e89 Mon Sep 17 00:00:00 2001 From: Frediano Ziglio <[email protected]> Date: Mon, 21 Sep 2020 07:00:39 +0100 Subject: [PATCH] vdagentd: Automatically release "agent_data" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Git-commit: b4479eb9b0a30cf99806a17bcbdc468828727e89 References: bsc#1173749 It's not guaranteed that agent_disconnect will release the connection so avoid to have a dandling pointer. Signed-off-by: Frediano Ziglio <[email protected]> Acked-by: Jakub Janků <[email protected]> Signed-off-by: Bruce Rogers <[email protected]> --- src/vdagentd/vdagentd.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c index 54e05c3..cd6340e 100644 --- a/src/vdagentd/vdagentd.c +++ b/src/vdagentd/vdagentd.c @@ -86,6 +86,13 @@ static uint32_t clipboard_serial[256]; static GMainLoop *loop; +static void agent_data_destroy(struct agent_data *agent_data) +{ + g_free(agent_data->session); + g_free(agent_data->screen_info); + g_free(agent_data); +} + static void vdagentd_quit(gint exit_code) { retval = exit_code; @@ -930,7 +937,7 @@ static void agent_connect(UdscsConnection *conn) syslog(LOG_ERR, "Could not get peer PID, disconnecting new client: %s", err->message); g_error_free(err); - g_free(agent_data); + agent_data_destroy(agent_data); udscs_server_destroy_connection(server, conn); return; } @@ -938,7 +945,8 @@ static void agent_connect(UdscsConnection *conn) agent_data->session = session_info_session_for_pid(session_info, pid); } - g_object_set_data(G_OBJECT(conn), "agent_data", agent_data); + g_object_set_data_full(G_OBJECT(conn), "agent_data", agent_data, + (GDestroyNotify) agent_data_destroy); udscs_write(conn, VDAGENTD_VERSION, 0, 0, (uint8_t *)VERSION, strlen(VERSION) + 1); update_active_session_connection(conn); @@ -951,13 +959,8 @@ static void agent_connect(UdscsConnection *conn) static void agent_disconnect(VDAgentConnection *conn, GError *err) { - struct agent_data *agent_data = g_object_get_data(G_OBJECT(conn), "agent_data"); - g_hash_table_foreach_remove(active_xfers, remove_active_xfers, conn); - g_clear_pointer(&agent_data->session, g_free); - g_free(agent_data->screen_info); - g_free(agent_data); if (err) { syslog(LOG_ERR, "%s", err->message); g_error_free(err); -- 2.29.0 ++++++ vdagentd-Avoid-calling-chmod.patch ++++++ >From f5f4506f6cb25bfd556f815565090a57296771ee Mon Sep 17 00:00:00 2001 From: Frediano Ziglio <[email protected]> Date: Thu, 24 Sep 2020 12:13:24 +0100 Subject: [PATCH 01/10] vdagentd: Avoid calling chmod References: bsc#1173749 Create the socket with the right permissions using umask. This also prevents possible symlink exploitation in case socket path is not secure. Signed-off-by: Frediano Ziglio <[email protected]> Acked-by: Uri Lublin <[email protected]> Signed-off-by: Bruce Rogers <[email protected]> --- src/vdagentd/vdagentd.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c index dca6980..a2b74bb 100644 --- a/src/vdagentd/vdagentd.c +++ b/src/vdagentd/vdagentd.c @@ -1208,7 +1208,9 @@ int main(int argc, char *argv[]) /* systemd socket activation not enabled, create our own */ #endif /* WITH_SYSTEMD_SOCKET_ACTIVATION */ { + mode_t mode = umask(0111); udscs_server_listen_to_address(server, vdagentd_socket, &err); + umask(mode); } if (err) { @@ -1219,16 +1221,6 @@ int main(int argc, char *argv[]) return 1; } - /* no need to set permissions on a socket that was provided by systemd */ - if (own_socket) { - if (chmod(vdagentd_socket, 0666)) { - syslog(LOG_CRIT, "Fatal could not change permissions on %s: %m", - vdagentd_socket); - udscs_destroy_server(server); - return 1; - } - } - #ifdef WITH_STATIC_UINPUT uinput = vdagentd_uinput_create(uinput_device, 1024, 768, NULL, 0, debug > 1, uinput_fake); -- 2.28.0 ++++++ vdagentd-Better-check-for-vdagent_connection_get_pee.patch ++++++ >From 7e924bcbf0bb6b300c6518499c05e87cea13ac51 Mon Sep 17 00:00:00 2001 From: Frediano Ziglio <[email protected]> Date: Mon, 21 Sep 2020 16:42:26 +0100 Subject: [PATCH] vdagentd: Better check for vdagent_connection_get_peer_pid results MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Git-commit: 7e924bcbf0bb6b300c6518499c05e87cea13ac51 References: bsc#1173749 The function can return -1 and leave "err" to NULL in some cases, do not check only for "err". Signed-off-by: Frediano Ziglio <[email protected]> Acked-by: Julien Ropé <[email protected]> Signed-off-by: Bruce Rogers <[email protected]> --- src/vdagentd/vdagentd.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c index cd6340e..560f2ce 100644 --- a/src/vdagentd/vdagentd.c +++ b/src/vdagentd/vdagentd.c @@ -933,10 +933,14 @@ static void agent_connect(UdscsConnection *conn) if (session_info) { pid = vdagent_connection_get_peer_pid(VDAGENT_CONNECTION(conn), &err); - if (err) { - syslog(LOG_ERR, "Could not get peer PID, disconnecting new client: %s", - err->message); - g_error_free(err); + if (err || pid <= 0) { + static const char msg[] = "Could not get peer PID, disconnecting new client"; + if (err) { + syslog(LOG_ERR, "%s: %s", msg, err->message); + g_error_free(err); + } else { + syslog(LOG_ERR, "%s", msg); + } agent_data_destroy(agent_data); udscs_server_destroy_connection(server, conn); return; -- 2.29.0 ++++++ vdagentd-Limit-number-of-agents-per-session-to-1.patch ++++++ >From cb15e7c8052cae75272bbd0d6a5cac37efa360f8 Mon Sep 17 00:00:00 2001 From: Frediano Ziglio <[email protected]> Date: Thu, 24 Sep 2020 12:13:44 +0100 Subject: [PATCH 07/10] vdagentd: Limit number of agents per session to 1 References: bsc#1173749 Signed-off-by: Frediano Ziglio <[email protected]> Acked-by: Uri Lublin <[email protected]> Signed-off-by: Bruce Rogers <[email protected]> --- src/vdagentd/vdagentd.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c index 59aa523..92885b5 100644 --- a/src/vdagentd/vdagentd.c +++ b/src/vdagentd/vdagentd.c @@ -952,6 +952,20 @@ static gboolean remove_active_xfers(gpointer key, gpointer value, gpointer conn) return 0; } +/* Check if this connection matches the passed session */ +static int connection_matches_session(UdscsConnection *conn, void *priv) +{ + const char *session = priv; + const struct agent_data *agent_data = g_object_get_data(G_OBJECT(conn), "agent_data"); + + if (!agent_data || !agent_data->session || + strcmp(agent_data->session, session) != 0) { + return 0; + } + + return 1; +} + /* Check a given process has a given UID */ static bool check_uid_of_pid(pid_t pid, uid_t uid) { @@ -1006,6 +1020,16 @@ static void agent_connect(UdscsConnection *conn) udscs_server_destroy_connection(server, conn); return; } + + // Check there are no other connection for this session + // Note that "conn" is not counted as "agent_data" is still not attached to it + if (udscs_server_for_all_clients(server, connection_matches_session, + agent_data->session) > 0) { + syslog(LOG_ERR, "An agent is already connected for this session"); + agent_data_destroy(agent_data); + udscs_server_destroy_connection(server, conn); + return; + } } g_object_set_data_full(G_OBJECT(conn), "agent_data", agent_data, -- 2.28.0 ++++++ vdagentd-Use-bool-for-agent_owns_clipboard-and-clien.patch ++++++ >From 7e37b05774fb5aed1978e28e3201fc8d28498df6 Mon Sep 17 00:00:00 2001 From: Frediano Ziglio <[email protected]> Date: Mon, 21 Sep 2020 06:53:45 +0100 Subject: [PATCH] vdagentd: Use bool for agent_owns_clipboard and client_connected MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Git-commit: 7e37b05774fb5aed1978e28e3201fc8d28498df6 References: bsc#1173749 More clear (instaed of 0/1) and save some bytes. Signed-off-by: Frediano Ziglio <[email protected]> Acked-by: Jakub Janků <[email protected]> Signed-off-by: Bruce Rogers <[email protected]> --- src/vdagentd/vdagentd.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c index 753c9bf..051de74 100644 --- a/src/vdagentd/vdagentd.c +++ b/src/vdagentd/vdagentd.c @@ -23,6 +23,7 @@ #include <stdio.h> #include <stdlib.h> +#include <stdbool.h> #include <string.h> #include <unistd.h> #include <fcntl.h> @@ -77,9 +78,9 @@ static int capabilities_size = 0; static const char *active_session = NULL; static unsigned int session_count = 0; static UdscsConnection *active_session_conn = NULL; -static int agent_owns_clipboard[256] = { 0, }; +static bool agent_owns_clipboard[256] = { false, }; static int retval = 0; -static int client_connected = 0; +static bool client_connected = false; static int max_clipboard = -1; static uint32_t clipboard_serial[256]; @@ -155,7 +156,7 @@ static void do_client_disconnect(void) if (client_connected) { udscs_server_write_all(server, VDAGENTD_CLIENT_DISCONNECTED, 0, 0, NULL, 0); - client_connected = 0; + client_connected = false; } } @@ -239,7 +240,7 @@ static void do_client_capabilities(VirtioPort *vport, do_client_disconnect(); if (debug) syslog(LOG_DEBUG, "New client connected"); - client_connected = 1; + client_connected = true; memset(clipboard_serial, 0, sizeof(clipboard_serial)); send_capabilities(vport, 0); } @@ -286,7 +287,7 @@ static void do_client_clipboard(VirtioPort *vport, } msg_type = VDAGENTD_CLIPBOARD_GRAB; - agent_owns_clipboard[selection] = 0; + agent_owns_clipboard[selection] = false; break; case VD_AGENT_CLIPBOARD_REQUEST: { VDAgentClipboardRequest *req = (VDAgentClipboardRequest *)data; @@ -624,7 +625,7 @@ static void virtio_port_read_complete( static void virtio_port_error_cb(VDAgentConnection *conn, GError *err) { - gboolean old_client_connected = client_connected; + bool old_client_connected = client_connected; syslog(LOG_CRIT, "AIIEEE lost spice client connection, reconnecting (err: %s)", err ? err->message : ""); g_clear_error(&err); @@ -717,7 +718,7 @@ static void do_agent_clipboard(UdscsConnection *conn, switch (header->type) { case VDAGENTD_CLIPBOARD_GRAB: msg_type = VD_AGENT_CLIPBOARD_GRAB; - agent_owns_clipboard[selection] = 1; + agent_owns_clipboard[selection] = true; break; case VDAGENTD_CLIPBOARD_REQUEST: msg_type = VD_AGENT_CLIPBOARD_REQUEST; @@ -737,7 +738,7 @@ static void do_agent_clipboard(UdscsConnection *conn, case VDAGENTD_CLIPBOARD_RELEASE: msg_type = VD_AGENT_CLIPBOARD_RELEASE; size = 0; - agent_owns_clipboard[selection] = 0; + agent_owns_clipboard[selection] = false; break; default: syslog(LOG_WARNING, "unexpected clipboard message type"); @@ -851,7 +852,7 @@ static void release_clipboards(void) vdagent_virtio_port_write(virtio_port, VDP_CLIENT_PORT, VD_AGENT_CLIPBOARD_RELEASE, 0, &sel, 1); } - agent_owns_clipboard[sel] = 0; + agent_owns_clipboard[sel] = false; } } -- 2.29.0 ++++++ vdagentd-do-not-allow-to-use-an-already-used-file-xf.patch ++++++ >From c120b431b7ccda0a0a1076e31f2f1367dbee656f Mon Sep 17 00:00:00 2001 From: Uri Lublin <[email protected]> Date: Sun, 11 Oct 2020 20:59:17 +0300 Subject: [PATCH 09/10] vdagentd: do not allow to use an already used file-xfer id References: bsc#1173749 Signed-off-by: Uri Lublin <[email protected]> Acked-by: Frediano Ziglio <[email protected]> Signed-off-by: Bruce Rogers <[email protected]> --- src/vdagentd/vdagentd.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/vdagentd/vdagentd.c b/src/vdagentd/vdagentd.c index 8437779..78378aa 100644 --- a/src/vdagentd/vdagentd.c +++ b/src/vdagentd/vdagentd.c @@ -404,6 +404,13 @@ static void do_client_file_xfer(VirtioPort *vport, "Cancelling client file-xfer request %u", s->id, VD_AGENT_FILE_XFER_STATUS_ERROR, (void*) &error, detail_size); return; + } else if (g_hash_table_lookup(active_xfers, GUINT_TO_POINTER(s->id)) != NULL) { + // id is already used -- client is confused + send_file_xfer_status(vport, + "File transfer ID is already used. " + "Cancelling client file-xfer request %u", + s->id, VD_AGENT_FILE_XFER_STATUS_ERROR, NULL, 0); + return; } msg_type = VDAGENTD_FILE_XFER_START; id = s->id; -- 2.28.0 ++++++ vdagentd-init-static-uinput-before-fork.patch ++++++ --- /var/tmp/diff_new_pack.TI6hyb/_old 2020-11-05 21:55:58.448041965 +0100 +++ /var/tmp/diff_new_pack.TI6hyb/_new 2020-11-05 21:55:58.452041957 +0100 @@ -6,6 +6,8 @@ Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit +Git-commit: 7b0435ef66af088c1a1be20b6bc6b0fcb76e4e1a + Otherwise the caller doesn't know that the init failed because we're returning 0 in the parent and 1 in child. ++++++ vdagentd-work-around-GLib-s-fork-issues.patch ++++++ --- /var/tmp/diff_new_pack.TI6hyb/_old 2020-11-05 21:55:58.464041930 +0100 +++ /var/tmp/diff_new_pack.TI6hyb/_new 2020-11-05 21:55:58.468041921 +0100 @@ -6,6 +6,8 @@ Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit +Git-commit: 9b8c0ebb9fb573e6ce3c5416371509f416503d0c + Creating threads is not compatible with forking as only the thread that calls fork() is inherited.
