From 764f071909df70622e79ee71323973c18c055c8c Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Mon, 14 Sep 2020 16:28:10 +0200 Subject: [PATCH 1/5] gdbusauth: empty DATA does not need a trailing space This is an interoperability fix. If the line is exactly "DATA\r\n", the reference implementation of D-Bus treats this as equivalent to "DATA \r\n", meaning the data block consists of zero hex-encoded bytes. In practice, D-Bus clients send empty data blocks as "DATA\r\n", and in fact sd-bus only accepts that, rejecting "DATA \r\n". [Originally part of a larger commit; commit message added by smcv] Signed-off-by: Giuseppe Scrivano Co-authored-by: Simon McVittie Signed-off-by: Simon McVittie --- gio/gdbusauth.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/gio/gdbusauth.c b/gio/gdbusauth.c index ede21c8514..d2ca41a201 100644 --- a/gio/gdbusauth.c +++ b/gio/gdbusauth.c @@ -783,13 +783,13 @@ _g_dbus_auth_run_client (GDBusAuth *auth, if (line == NULL) goto out; debug_print ("CLIENT: WaitingForData, read='%s'", line); - if (g_str_has_prefix (line, "DATA ")) + if (g_str_equal (line, "DATA") || g_str_has_prefix (line, "DATA ")) { gchar *encoded; gchar *decoded_data; gsize decoded_data_len = 0; - encoded = g_strdup (line + 5); + encoded = g_strdup (line + 4); g_free (line); g_strstrip (encoded); decoded_data = hexdecode (encoded, &decoded_data_len, error); @@ -1255,13 +1255,13 @@ _g_dbus_auth_run_server (GDBusAuth *auth, debug_print ("SERVER: WaitingForData, read '%s'", line); if (line == NULL) goto out; - if (g_str_has_prefix (line, "DATA ")) + if (g_str_equal (line, "DATA") || g_str_has_prefix (line, "DATA ")) { gchar *encoded; gchar *decoded_data; gsize decoded_data_len = 0; - encoded = g_strdup (line + 5); + encoded = g_strdup (line + 4); g_free (line); g_strstrip (encoded); decoded_data = hexdecode (encoded, &decoded_data_len, error); -- GitLab From a7d2e727eefcf883bb463ad559f5632e8e448757 Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Mon, 14 Sep 2020 16:28:10 +0200 Subject: [PATCH 2/5] GDBusServer: If no initial response for EXTERNAL, send a challenge Sending an "initial response" along with the AUTH command is meant to be an optional optimization, and clients are allowed to omit it. We must reply with our initial challenge, which in the case of EXTERNAL is an empty string: the client responds to that with the authorization identity. If we do not reply to the AUTH command, then the client will wait forever for our reply, while we wait forever for the reply that we expect the client to send, resulting in deadlock. D-Bus does not have a way to distinguish between an empty initial response and the absence of an initial response, so clients that want to use an empty authorization identity, such as systed's sd-bus, cannot use the initial-response optimization and will fail to connect to a GDBusServer that does not have this change. [Originally part of a larger commit; commit message added by smcv.] Signed-off-by: Simon McVittie --- gio/gdbusauthmechanismexternal.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/gio/gdbusauthmechanismexternal.c b/gio/gdbusauthmechanismexternal.c index 617fe1d0e5..ddd06cbd5e 100644 --- a/gio/gdbusauthmechanismexternal.c +++ b/gio/gdbusauthmechanismexternal.c @@ -40,6 +40,7 @@ struct _GDBusAuthMechanismExternalPrivate gboolean is_client; gboolean is_server; GDBusAuthMechanismState state; + gboolean empty_data_sent; }; static gint mechanism_get_priority (void); @@ -253,7 +254,9 @@ mechanism_server_initiate (GDBusAuthMechanism *mechanism, } else { - m->priv->state = G_DBUS_AUTH_MECHANISM_STATE_WAITING_FOR_DATA; + /* The initial-response optimization was not used, so we need to + * send an empty challenge to prompt the client to respond. */ + m->priv->state = G_DBUS_AUTH_MECHANISM_STATE_HAVE_DATA_TO_SEND; } } @@ -288,12 +291,22 @@ mechanism_server_data_send (GDBusAuthMechanism *mechanism, g_return_val_if_fail (G_IS_DBUS_AUTH_MECHANISM_EXTERNAL (mechanism), NULL); g_return_val_if_fail (m->priv->is_server && !m->priv->is_client, NULL); - g_return_val_if_fail (m->priv->state == G_DBUS_AUTH_MECHANISM_STATE_HAVE_DATA_TO_SEND, NULL); - /* can never end up here because we are never in the HAVE_DATA_TO_SEND state */ - g_assert_not_reached (); + if (out_data_len) + *out_data_len = 0; - return NULL; + if (m->priv->empty_data_sent) + { + /* We have already sent an empty data response. + Reject the connection. */ + m->priv->state = G_DBUS_AUTH_MECHANISM_STATE_REJECTED; + return NULL; + } + + m->priv->state = G_DBUS_AUTH_MECHANISM_STATE_WAITING_FOR_DATA; + m->priv->empty_data_sent = TRUE; + + return g_strdup (""); } static gchar * -- GitLab From b51e3ab09e39c590c65a7be6228ecfa48a6189f6 Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Mon, 14 Sep 2020 16:28:10 +0200 Subject: [PATCH 3/5] GDBusServer: Accept empty authorization identity for EXTERNAL mechanism RFC 4422 appendix A defines the empty authorization identity to mean the identity that the server associated with its authentication credentials. In this case, this means whatever uid is in the GCredentials object. In particular, this means that clients in a different Linux user namespace can authenticate against our server and will be authorized as the version of their uid that is visible in the server's namespace, even if the corresponding numeric uid returned by geteuid() in the client's namespace was different. systemd's sd-bus has relied on this since commit https://github.com/systemd/systemd/commit/1ed4723d38cd0d1423c8fe650f90fa86007ddf55. [Originally part of a larger commit; commit message added by smcv] Signed-off-by: Simon McVittie --- gio/gdbusauthmechanismexternal.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/gio/gdbusauthmechanismexternal.c b/gio/gdbusauthmechanismexternal.c index ddd06cbd5e..a465862d12 100644 --- a/gio/gdbusauthmechanismexternal.c +++ b/gio/gdbusauthmechanismexternal.c @@ -201,14 +201,24 @@ data_matches_credentials (const gchar *data, if (credentials == NULL) goto out; - if (data == NULL || data_len == 0) - goto out; - #if defined(G_OS_UNIX) { gint64 alleged_uid; gchar *endp; + /* If we were unable to find out the uid, then nothing + * can possibly match it. */ + if (g_credentials_get_unix_user (credentials, NULL) == (uid_t) -1) + goto out; + + /* An empty authorization identity means we want to be + * whatever identity the out-of-band credentials say we have + * (RFC 4422 appendix A.1). This effectively matches any uid. */ + if (data == NULL || data_len == 0) + { + match = TRUE; + goto out; + } /* on UNIX, this is the uid as a string in base 10 */ alleged_uid = g_ascii_strtoll (data, &endp, 10); if (*endp == '\0') -- GitLab From 3f532af65c98e4ba8426c53f26c9ee15d3692f9c Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Mon, 18 Jul 2022 17:14:44 +0100 Subject: [PATCH 4/5] gdbusauth: Represent empty data block as DATA\r\n, with no space This is an interoperability fix. The reference implementation of D-Bus treats "DATA\r\n" as equivalent to "DATA \r\n", but sd-bus does not, and only accepts the former. Signed-off-by: Simon McVittie --- gio/gdbusauth.c | 34 ++++++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/gio/gdbusauth.c b/gio/gdbusauth.c index d2ca41a201..89cbbf67c6 100644 --- a/gio/gdbusauth.c +++ b/gio/gdbusauth.c @@ -807,11 +807,21 @@ _g_dbus_auth_run_client (GDBusAuth *auth, { gchar *data; gsize data_len; - gchar *encoded_data; + data = _g_dbus_auth_mechanism_client_data_send (mech, &data_len); - encoded_data = _g_dbus_hexencode (data, data_len); - s = g_strdup_printf ("DATA %s\r\n", encoded_data); - g_free (encoded_data); + + if (data_len == 0) + { + s = g_strdup ("DATA\r\n"); + } + else + { + gchar *encoded_data = _g_dbus_hexencode (data, data_len); + + s = g_strdup_printf ("DATA %s\r\n", encoded_data); + g_free (encoded_data); + } + g_free (data); debug_print ("CLIENT: writing '%s'", s); if (!g_data_output_stream_put_string (dos, s, cancellable, error)) @@ -1209,13 +1219,21 @@ _g_dbus_auth_run_server (GDBusAuth *auth, gsize data_len; data = _g_dbus_auth_mechanism_server_data_send (mech, &data_len); + if (data != NULL) { - gchar *encoded_data; + if (data_len == 0) + { + s = g_strdup ("DATA\r\n"); + } + else + { + gchar *encoded_data = _g_dbus_hexencode (data, data_len); + + s = g_strdup_printf ("DATA %s\r\n", encoded_data); + g_free (encoded_data); + } - encoded_data = _g_dbus_hexencode (data, data_len); - s = g_strdup_printf ("DATA %s\r\n", encoded_data); - g_free (encoded_data); g_free (data); debug_print ("SERVER: writing '%s'", s); -- GitLab From 0c240398740d306f6395daef93fd6fdce26c526d Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Mon, 18 Jul 2022 17:22:34 +0100 Subject: [PATCH 5/5] tests: Add a test for GDBusServer with various simulated clients Instead of using a GDBusConnection, this does the handshake at a lower level using specific strings in the SASL handshake, to verify that we will interoperate with various clients including sd-bus, libdbus and older versions of GDBus. Signed-off-by: Simon McVittie --- gio/tests/gdbus-sasl.c | 334 +++++++++++++++++++++++++++++++++++++++++ gio/tests/meson.build | 1 + 2 files changed, 335 insertions(+) create mode 100644 gio/tests/gdbus-sasl.c diff --git a/gio/tests/gdbus-sasl.c b/gio/tests/gdbus-sasl.c new file mode 100644 index 0000000000..2a46525403 --- /dev/null +++ b/gio/tests/gdbus-sasl.c @@ -0,0 +1,334 @@ +/* + * Copyright 2019-2022 Collabora Ltd. + * + * SPDX-License-Identifier: LGPL-2.1-or-later + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library 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 + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General + * Public License along with this library; if not, see . + */ + +#include "config.h" + +#include +#include +#include +#include + +#include +#include +#include + +/* For G_CREDENTIALS_*_SUPPORTED */ +#include + +static const char * const explicit_external_initial_response_fail[] = +{ + "EXTERNAL with incorrect initial response", + "C:AUTH EXTERNAL ", + "S:REJECTED.*$", + NULL +}; + +static const char * const explicit_external_fail[] = +{ + "EXTERNAL without initial response, failing to authenticate", + "C:AUTH EXTERNAL", + "S:DATA$", + "C:DATA ", + "S:REJECTED.*$", + NULL +}; + +#if defined(G_CREDENTIALS_SOCKET_GET_CREDENTIALS_SUPPORTED) || defined(G_CREDENTIALS_UNIX_CREDENTIALS_MESSAGE_SUPPORTED) +static const char * const explicit_external_initial_response[] = +{ + "EXTERNAL with initial response", + /* This is what most older D-Bus libraries do. */ + "C:AUTH EXTERNAL ", /* I claim to be */ + "S:OK [0-9a-f]+$", + NULL +}; + +static const char * const explicit_external[] = +{ + "EXTERNAL without initial response", + /* In theory this is equally valid, although many D-Bus libraries + * probably don't support it correctly. */ + "C:AUTH EXTERNAL", /* Start EXTERNAL, no initial response */ + "S:DATA$", /* Who are you? */ + "C:DATA ", /* I claim to be */ + "S:OK [0-9a-f]+$", + NULL +}; + +static const char * const implicit_external[] = +{ + "EXTERNAL with empty authorization identity", + /* This is what sd-bus does. */ + "C:AUTH EXTERNAL", /* Start EXTERNAL, no initial response */ + "S:DATA$", /* Who are you? */ + "C:DATA", /* I'm whoever the kernel says I am */ + "S:OK [0-9a-f]+$", + NULL +}; + +static const char * const implicit_external_space[] = +{ + "EXTERNAL with empty authorization identity and whitespace", + /* GDBus used to represent empty data blocks like this, although it + * isn't interoperable to do so (in particular sd-bus would reject this). */ + "C:AUTH EXTERNAL", /* Start EXTERNAL, no initial response */ + "S:DATA$", /* Who are you? */ + "C:DATA ", /* I'm whoever the kernel says I am */ + "S:OK [0-9a-f]+$", + NULL +}; +#endif + +static const char * const * const handshakes[] = +{ + explicit_external_initial_response_fail, + explicit_external_fail, +#if defined(G_CREDENTIALS_SOCKET_GET_CREDENTIALS_SUPPORTED) || defined(G_CREDENTIALS_UNIX_CREDENTIALS_MESSAGE_SUPPORTED) + explicit_external_initial_response, + explicit_external, + implicit_external, + implicit_external_space, +#endif +}; + +static void +encode_uid (guint uid, + GString *dest) +{ + gchar *str = g_strdup_printf ("%u", uid); + gchar *p; + + g_string_assign (dest, ""); + + for (p = str; *p != '\0'; p++) + g_string_append_printf (dest, "%02x", (unsigned char) *p); + + g_free (str); +} + +typedef struct +{ + GCond cond; + GMutex mutex; + GDBusServerFlags server_flags; + GMainContext *ctx; + GMainLoop *loop; + gchar *guid; + gchar *listenable_address; + gboolean ready; +} ServerInfo; + +static gboolean +idle_in_server_thread_cb (gpointer user_data) +{ + ServerInfo *info = user_data; + + g_mutex_lock (&info->mutex); + info->ready = TRUE; + g_cond_broadcast (&info->cond); + g_mutex_unlock (&info->mutex); + return G_SOURCE_REMOVE; +} + +static gpointer +server_thread_cb (gpointer user_data) +{ + GDBusServer *server = NULL; + GError *error = NULL; + GSource *source; + ServerInfo *info = user_data; + + g_main_context_push_thread_default (info->ctx); + server = g_dbus_server_new_sync (info->listenable_address, + info->server_flags, + info->guid, + NULL, + NULL, + &error); + g_assert_no_error (error); + g_assert_nonnull (server); + g_dbus_server_start (server); + + /* Tell the main thread when the server is ready to accept connections */ + source = g_idle_source_new (); + g_source_set_callback (source, idle_in_server_thread_cb, info, NULL); + g_source_attach (source, info->ctx); + g_source_unref (source); + + g_main_loop_run (info->loop); + + g_main_context_pop_thread_default (info->ctx); + g_dbus_server_stop (server); + g_clear_object (&server); + return NULL; +} + +static void +test_sasl_server (void) +{ + GError *error = NULL; + GSocketAddress *addr = NULL; + GString *buf = g_string_new (""); + GString *encoded_uid = g_string_new (""); + GString *encoded_wrong_uid = g_string_new (""); + GThread *server_thread = NULL; + ServerInfo info = + { + .server_flags = G_DBUS_SERVER_FLAGS_RUN_IN_THREAD, + }; + gchar *escaped = NULL; + gchar *path = NULL; + gchar *tmpdir = NULL; + gsize i; + + tmpdir = g_dir_make_tmp ("gdbus-server-auth-XXXXXX", &error); + g_assert_no_error (error); + escaped = g_dbus_address_escape_value (tmpdir); + + path = g_build_filename (tmpdir, "socket", NULL); + g_cond_init (&info.cond); + g_mutex_init (&info.mutex); + info.ctx = g_main_context_new (); + info.guid = g_dbus_generate_guid (); + info.listenable_address = g_strdup_printf ("unix:path=%s/socket", escaped); + info.loop = g_main_loop_new (info.ctx, FALSE); + info.ready = FALSE; + server_thread = g_thread_new ("GDBusServer", server_thread_cb, &info); + + g_mutex_lock (&info.mutex); + + while (!info.ready) + g_cond_wait (&info.cond, &info.mutex); + + g_mutex_unlock (&info.mutex); + + addr = g_unix_socket_address_new (path); + + encode_uid (geteuid (), encoded_uid); + encode_uid (geteuid () == 0 ? 65534 : 0, encoded_wrong_uid); + + for (i = 0; i < G_N_ELEMENTS (handshakes); i++) + { + const char * const *handshake = handshakes[i]; + GSocketClient *client; + GSocketConnection *conn; + GUnixConnection *conn_unix; /* unowned */ + GInputStream *istream; /* unowned */ + GDataInputStream *istream_data; + GOutputStream *ostream; /* unowned */ + GError *error = NULL; + gsize j; + + g_test_message ("New handshake: %s", handshake[0]); + + client = g_socket_client_new (); + conn = g_socket_client_connect (client, G_SOCKET_CONNECTABLE (addr), + NULL, &error); + g_assert_no_error (error); + + g_assert_true (G_IS_UNIX_CONNECTION (conn)); + conn_unix = G_UNIX_CONNECTION (conn); + istream = g_io_stream_get_input_stream (G_IO_STREAM (conn)); + ostream = g_io_stream_get_output_stream (G_IO_STREAM (conn)); + istream_data = g_data_input_stream_new (istream); + g_data_input_stream_set_newline_type (istream_data, G_DATA_STREAM_NEWLINE_TYPE_CR_LF); + + g_unix_connection_send_credentials (conn_unix, NULL, &error); + g_assert_no_error (error); + + for (j = 1; handshake[j] != NULL; j++) + { + if (j % 2 == 1) + { + /* client to server */ + const char *line = handshake[j]; + + g_assert_cmpint (line[0], ==, 'C'); + g_assert_cmpint (line[1], ==, ':'); + g_string_assign (buf, line + 2); + g_string_replace (buf, "", encoded_uid->str, 0); + g_string_replace (buf, "", encoded_wrong_uid->str, 0); + g_test_message ("C:“%s”", buf->str); + g_string_append (buf, "\r\n"); + + g_output_stream_write_all (ostream, buf->str, buf->len, NULL, NULL, &error); + g_assert_no_error (error); + } + else + { + /* server to client */ + const char *pattern = handshake[j]; + char *line; + gsize len; + + g_assert_cmpint (pattern[0], ==, 'S'); + g_assert_cmpint (pattern[1], ==, ':'); + + g_test_message ("Expect: /^%s/", pattern + 2); + line = g_data_input_stream_read_line (istream_data, &len, NULL, &error); + g_assert_no_error (error); + g_test_message ("S:“%s”", line); + g_assert_cmpuint (len, ==, strlen (line)); + + if (!g_regex_match_simple (pattern + 2, line, + G_REGEX_ANCHORED, + G_REGEX_MATCH_ANCHORED)) + g_error ("Expected /^%s/, got “%s”", pattern + 2, line); + + g_free (line); + } + } + + g_object_unref (istream_data); + g_object_unref (conn); + g_object_unref (client); + } + + g_main_loop_quit (info.loop); + g_thread_join (server_thread); + + if (tmpdir != NULL) + g_assert_no_errno (g_rmdir (tmpdir)); + + g_clear_pointer (&info.ctx, g_main_context_unref); + g_clear_pointer (&info.loop, g_main_loop_unref); + g_clear_object (&addr); + g_string_free (buf, TRUE); + g_string_free (encoded_uid, TRUE); + g_string_free (encoded_wrong_uid, TRUE); + g_free (escaped); + g_free (info.guid); + g_free (info.listenable_address); + g_free (path); + g_free (tmpdir); + g_cond_clear (&info.cond); + g_mutex_clear (&info.mutex); +} + +int +main (int argc, + char *argv[]) +{ + setlocale (LC_ALL, ""); + g_test_init (&argc, &argv, G_TEST_OPTION_ISOLATE_DIRS, NULL); + + g_test_add_func ("/gdbus/sasl/server", test_sasl_server); + + return g_test_run(); +} diff --git a/gio/tests/meson.build b/gio/tests/meson.build index e993ab2284..282b911c72 100644 --- a/gio/tests/meson.build +++ b/gio/tests/meson.build @@ -196,6 +196,7 @@ if host_machine.system() != 'windows' gio_tests += { 'file' : {}, 'gdbus-peer-object-manager' : {}, + 'gdbus-sasl' : {}, 'live-g-file' : {}, 'resolver-parsing' : {'dependencies' : [network_libs]}, 'socket-address' : {}, -- GitLab