From 4607d5855bea8802d1fe93f21f1e9c962320d775 Mon Sep 17 00:00:00 2001 From: Ryan Gonzalez Date: Fri, 17 Dec 2021 16:04:55 -0600 Subject: [PATCH 1/3] Add support for pipelined SASL handshakes, such as sdbus and zbus sd-bus sends the BEGIN command in at the same time as the AUTH command, assuming that the authentication will succeed. This ends up confusing xdg-dbus-proxy, as it assumes that BEGIN is not sent at the same time as any other messages. As a result, the server's authentication replies end up interpreted as standard D-Bus messages, failing parsing and resulting in the client being unable to connect. This changes the code to keep track of the number of authentication requests and the number ofresponses from the server to know when the authentication phase of the connection has actually completed. Until the authentication is completed, hold off all client messages received after BEGIN. We only send to the server the authentication commands up (and including) the BEGIN command, but everything past the line terminators is queued into the `extra_input_data` buffer and we stop reading from the client socket. Once the authentication is completed, the (partial) message we saved into `extra_input_data` is queued for the D-Bus server into the first outgoing buffer, and reading from the client socket resumes. Note that in order to cleanly process the partial sendind of data, another offset is introduced into the Buffer structure, which holds the bytes of buffers which have already been sent over the socket. Fixes #21 (finally!) This is based on initial work by: * Ryan Gonzalez * Alberto Mardegan --- flatpak-proxy.c | 182 +++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 148 insertions(+), 34 deletions(-) diff --git a/flatpak-proxy.c b/flatpak-proxy.c index 82c3f0d..0bbd160 100644 --- a/flatpak-proxy.c +++ b/flatpak-proxy.c @@ -198,6 +198,16 @@ typedef struct FlatpakProxyClient FlatpakProxyClient; // Use a relatively hight number since there are not a lot of fake requests we need to do #define MAX_CLIENT_SERIAL (G_MAXUINT32 - 65536) +typedef enum { + /* The client has not sent BEGIN yet */ + AUTH_WAITING_FOR_BEGIN, + /* The client sent BEGIN, but the server has not yet responded to the auth + messages that the client sent before */ + AUTH_WAITING_FOR_BACKLOG, + /* Authentication is fully complete */ + AUTH_COMPLETE, +} AuthState; + typedef enum { EXPECTED_REPLY_NONE, EXPECTED_REPLY_NORMAL, @@ -211,8 +221,14 @@ typedef enum { typedef struct { + /* During write and message parsing this is the size of the valid data in the buffer. + During reads this is the capacity of the buffer. */ gsize size; + /* Offset to the first writable position (the buffer is full when pos == + * size) */ gsize pos; + /* Offset to the first byte that hasn't been sent yet */ + gsize sent; int refcount; gboolean send_credentials; GList *control_messages; @@ -291,7 +307,9 @@ struct FlatpakProxyClient FlatpakProxy *proxy; - gboolean authenticated; + AuthState auth_state; + gsize auth_requests; + gsize auth_replies; GByteArray *auth_buffer; ProxySide client_side; @@ -773,6 +791,7 @@ buffer_new (gsize size, Buffer *old) if (old) { buffer->pos = old->pos; + buffer->sent = old->sent; /* Takes ownership of any old control messages */ buffer->control_messages = old->control_messages; old->control_messages = NULL; @@ -837,13 +856,18 @@ buffer_read (ProxySide *side, Buffer *buffer, GSocket *socket) { - gsize received; + FlatpakProxyClient *client = side->client; + gsize received = 0; GInputVector v; GError *error = NULL; GSocketControlMessage **messages; int num_messages, i; - if (side->extra_input_data) + if (client->auth_state == AUTH_WAITING_FOR_BACKLOG && + side == &client->client_side) + return FALSE; + + if (side->extra_input_data && client->auth_state == AUTH_COMPLETE) { gsize extra_size; const guchar *extra_bytes = g_bytes_get_data (side->extra_input_data, &extra_size); @@ -865,7 +889,7 @@ buffer_read (ProxySide *side, g_clear_pointer (&side->extra_input_data, g_bytes_unref); } } - else + else if (!side->extra_input_data) { gssize res; int flags = 0; @@ -941,7 +965,7 @@ buffer_write (ProxySide *side, return FALSE; } - buffer->pos = 1; + buffer->sent = 1; return TRUE; } @@ -950,8 +974,8 @@ buffer_write (ProxySide *side, for (l = buffer->control_messages, i = 0; l != NULL; l = l->next, i++) messages[i] = l->data; - v.buffer = &buffer->data[buffer->pos]; - v.size = buffer->size - buffer->pos; + v.buffer = &buffer->data[buffer->sent]; + v.size = buffer->pos - buffer->sent; res = g_socket_send_message (socket, NULL, &v, 1, messages, n_messages, @@ -978,16 +1002,15 @@ buffer_write (ProxySide *side, g_list_free_full (buffer->control_messages, g_object_unref); buffer->control_messages = NULL; - buffer->pos += res; + buffer->sent += res; return TRUE; } static gboolean -side_out_cb (GSocket *socket, GIOCondition condition, gpointer user_data) +send_outgoing_buffers (GSocket *socket, ProxySide *side) { - ProxySide *side = user_data; FlatpakProxyClient *client = side->client; - gboolean retval = G_SOURCE_CONTINUE; + gboolean all_done = FALSE; g_object_ref (client); @@ -997,7 +1020,7 @@ side_out_cb (GSocket *socket, GIOCondition condition, gpointer user_data) if (buffer_write (side, buffer, socket)) { - if (buffer->pos == buffer->size) + if (buffer->sent == buffer->size) { side->buffers = g_list_delete_link (side->buffers, side->buffers); buffer_unref (buffer); @@ -1013,8 +1036,7 @@ side_out_cb (GSocket *socket, GIOCondition condition, gpointer user_data) { ProxySide *other_side = get_other_side (side); - side->out_source = NULL; - retval = G_SOURCE_REMOVE; + all_done = TRUE; if (other_side->closed) side_closed (side); @@ -1022,7 +1044,24 @@ side_out_cb (GSocket *socket, GIOCondition condition, gpointer user_data) g_object_unref (client); - return retval; + return all_done; +} + +static gboolean +side_out_cb (GSocket *socket, GIOCondition condition, gpointer user_data) +{ + ProxySide *side = user_data; + + gboolean all_done = send_outgoing_buffers (socket, side); + if (all_done) + { + side->out_source = NULL; + return G_SOURCE_REMOVE; + } + else + { + return G_SOURCE_CONTINUE; + } } static void @@ -1061,7 +1100,6 @@ queue_outgoing_buffer (ProxySide *side, Buffer *buffer) g_source_unref (side->out_source); } - buffer->pos = 0; side->buffers = g_list_append (side->buffers, buffer); } @@ -1821,6 +1859,7 @@ message_to_buffer (GDBusMessage *message) blob = g_dbus_message_to_blob (message, &blob_size, G_DBUS_CAPABILITY_FLAGS_NONE, NULL); buffer = buffer_new (blob_size, NULL); memcpy (buffer->data, blob, blob_size); + buffer->pos = blob_size; g_free (blob); return buffer; @@ -2415,7 +2454,7 @@ got_buffer_from_client (FlatpakProxyClient *client, ProxySide *side, Buffer *buf { ExpectedReplyType expecting_reply = EXPECTED_REPLY_NONE; - if (client->authenticated && client->proxy->filter) + if (client->auth_state == AUTH_COMPLETE && client->proxy->filter) { g_autoptr(Header) header = NULL; g_autoptr(GError) error = NULL; @@ -2582,7 +2621,7 @@ got_buffer_from_client (FlatpakProxyClient *client, ProxySide *side, Buffer *buf static void got_buffer_from_bus (FlatpakProxyClient *client, ProxySide *side, Buffer *buffer) { - if (client->authenticated && client->proxy->filter) + if (client->auth_state == AUTH_COMPLETE && client->proxy->filter) { g_autoptr(Header) header = NULL; g_autoptr(GError) error = NULL; @@ -2827,11 +2866,19 @@ auth_line_is_begin (guint8 *line) next_char == '\t'; } +static guint8 * +find_auth_line_end (guint8 *line_start, gsize buffer_size) +{ + return memmem (line_start, buffer_size, + AUTH_LINE_SENTINEL, strlen (AUTH_LINE_SENTINEL)); +} + static gssize -find_auth_end (FlatpakProxyClient *client, Buffer *buffer) +find_auth_end (FlatpakProxyClient *client, Buffer *buffer, gsize *out_lines_skipped) { goffset offset = 0; gsize original_size = client->auth_buffer->len; + gsize lines_skipped = 0; /* Add the new data to the remaining data from last iteration */ g_byte_array_append (client->auth_buffer, buffer->data, buffer->pos); @@ -2842,8 +2889,7 @@ find_auth_end (FlatpakProxyClient *client, Buffer *buffer) gsize remaining_data = client->auth_buffer->len - offset; guint8 *line_end; - line_end = memmem (line_start, remaining_data, - AUTH_LINE_SENTINEL, strlen (AUTH_LINE_SENTINEL)); + line_end = find_auth_line_end (line_start, remaining_data); if (line_end) /* Found end of line */ { offset = (line_end + strlen (AUTH_LINE_SENTINEL) - client->auth_buffer->data); @@ -2853,13 +2899,18 @@ find_auth_end (FlatpakProxyClient *client, Buffer *buffer) *line_end = 0; if (auth_line_is_begin (line_start)) - return offset - original_size; + { + *out_lines_skipped = lines_skipped; + return offset - original_size; + } /* continue with next line */ + ++lines_skipped; } else { - /* No end-of-line in this buffer */ + /* No more end-of-line in this buffer */ + *out_lines_skipped = lines_skipped; g_byte_array_remove_range (client->auth_buffer, 0, offset); /* Abort if more than 16k before newline, similar to what dbus-daemon does */ @@ -2879,6 +2930,7 @@ side_in_cb (GSocket *socket, GIOCondition condition, gpointer user_data) GError *error = NULL; Buffer *buffer; gboolean retval = G_SOURCE_CONTINUE; + gboolean wake_client_reader = FALSE; g_object_ref (client); @@ -2886,7 +2938,7 @@ side_in_cb (GSocket *socket, GIOCondition condition, gpointer user_data) { if (!side->got_first_byte) buffer = buffer_new (1, NULL); - else if (!client->authenticated) + else if (client->auth_state != AUTH_COMPLETE) buffer = buffer_new (64, NULL); else buffer = side->current_read_buffer; @@ -2898,12 +2950,12 @@ side_in_cb (GSocket *socket, GIOCondition condition, gpointer user_data) break; } - if (!client->authenticated) + if (client->auth_state != AUTH_COMPLETE) { if (buffer->pos > 0) { - gboolean found_auth_end = FALSE; - gsize extra_data; + /* The state of the client after handling this buffer */ + AuthState new_auth_state = client->auth_state; buffer->size = buffer->pos; if (!side->got_first_byte) @@ -2912,15 +2964,24 @@ side_in_cb (GSocket *socket, GIOCondition condition, gpointer user_data) side->got_first_byte = TRUE; } /* Look for end of authentication mechanism */ - else if (side == &client->client_side) + else if (side == &client->client_side && client->auth_state == AUTH_WAITING_FOR_BEGIN) { - gssize auth_end = find_auth_end (client, buffer); + gsize lines_skipped = 0; + gssize auth_end = find_auth_end (client, buffer, &lines_skipped); + + client->auth_requests += lines_skipped; if (auth_end >= 0) { - found_auth_end = TRUE; - buffer->size = auth_end; - extra_data = buffer->pos - buffer->size; + gsize extra_data; + + if (client->auth_replies == client->auth_requests) + new_auth_state = AUTH_COMPLETE; + else + new_auth_state = AUTH_WAITING_FOR_BACKLOG; + + extra_data = buffer->pos - auth_end; + buffer->size = buffer->pos = auth_end; /* We may have gotten some extra data which is not part of the auth handshake, keep it for the next iteration. */ @@ -2936,11 +2997,59 @@ side_in_cb (GSocket *socket, GIOCondition condition, gpointer user_data) break; } } + else if (side == &client->bus_side) + { + gsize remaining = buffer->pos; + guint8 *line_start = buffer->data; + + while (remaining > 0) + { + guint8 *line_end = NULL; + + if (client->auth_replies == client->auth_requests) + { + buffer_unref (buffer); + if (client->proxy->log_messages) + g_print ("Unexpected auth reply line from bus, aborting\n"); + side_closed (side); + break; + } + + line_end = find_auth_line_end (line_start, remaining); + if (line_end == NULL) + line_end = line_start + remaining; + else + { + line_end += strlen (AUTH_LINE_SENTINEL); + client->auth_replies++; + } + + remaining -= line_end - line_start; + line_start = line_end; + + + if (client->auth_state == AUTH_WAITING_FOR_BACKLOG && + client->auth_replies == client->auth_requests) + { + new_auth_state = AUTH_COMPLETE; + /* We may have added extra data on the input, ensure we read it directly */ + wake_client_reader = TRUE; + + buffer->pos = buffer->size = line_start - buffer->data; + + /* We may have gotten some extra data which is not part of + the auth handshake, keep it for the next iteration. */ + if (remaining > 0) + side->extra_input_data = g_bytes_new (line_start, remaining); + + break; + } + } + } got_buffer_from_side (side, buffer); - if (found_auth_end) - client->authenticated = TRUE; + client->auth_state = new_auth_state; } else { @@ -2977,6 +3086,11 @@ side_in_cb (GSocket *socket, GIOCondition condition, gpointer user_data) side->in_source = NULL; retval = G_SOURCE_REMOVE; } + else if (wake_client_reader) + { + GSocket *client_socket = g_socket_connection_get_socket (client->client_side.connection); + side_in_cb (client_socket, G_IO_IN, &client->client_side); + } g_object_unref (client); From 65cde98752649e7faf4a43b29ff69b9863ea27be Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Fri, 16 Aug 2024 15:18:06 +0200 Subject: [PATCH 2/3] Now that we support pipelined auth, make default auth buffer larger This way we can avoid multiple reads in the pipelined case. --- flatpak-proxy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flatpak-proxy.c b/flatpak-proxy.c index 0bbd160..24fe1ea 100644 --- a/flatpak-proxy.c +++ b/flatpak-proxy.c @@ -2939,7 +2939,7 @@ side_in_cb (GSocket *socket, GIOCondition condition, gpointer user_data) if (!side->got_first_byte) buffer = buffer_new (1, NULL); else if (client->auth_state != AUTH_COMPLETE) - buffer = buffer_new (64, NULL); + buffer = buffer_new (256, NULL); else buffer = side->current_read_buffer; From 5073e0c3426628e98e3e08cd877ea698fdbb0253 Mon Sep 17 00:00:00 2001 From: Alexander Larsson Date: Tue, 20 Aug 2024 12:11:59 +0200 Subject: [PATCH 3/3] Minor code cleanup of side_in_cb() We indent one tab less by moving the check for buffer->pos == 0 to a separate check. --- flatpak-proxy.c | 164 ++++++++++++++++++++++++------------------------ 1 file changed, 81 insertions(+), 83 deletions(-) diff --git a/flatpak-proxy.c b/flatpak-proxy.c index 24fe1ea..7153135 100644 --- a/flatpak-proxy.c +++ b/flatpak-proxy.c @@ -2952,109 +2952,107 @@ side_in_cb (GSocket *socket, GIOCondition condition, gpointer user_data) if (client->auth_state != AUTH_COMPLETE) { - if (buffer->pos > 0) + if (buffer->pos == 0) { - /* The state of the client after handling this buffer */ - AuthState new_auth_state = client->auth_state; + buffer_unref (buffer); + continue; + } - buffer->size = buffer->pos; - if (!side->got_first_byte) - { - buffer->send_credentials = TRUE; - side->got_first_byte = TRUE; - } - /* Look for end of authentication mechanism */ - else if (side == &client->client_side && client->auth_state == AUTH_WAITING_FOR_BEGIN) + /* The state of the client after handling this buffer */ + AuthState new_auth_state = client->auth_state; + + buffer->size = buffer->pos; + if (!side->got_first_byte) + { + buffer->send_credentials = TRUE; + side->got_first_byte = TRUE; + } + /* Look for end of authentication mechanism */ + else if (side == &client->client_side && client->auth_state == AUTH_WAITING_FOR_BEGIN) + { + gsize lines_skipped = 0; + gssize auth_end = find_auth_end (client, buffer, &lines_skipped); + + client->auth_requests += lines_skipped; + + if (auth_end >= 0) { - gsize lines_skipped = 0; - gssize auth_end = find_auth_end (client, buffer, &lines_skipped); + gsize extra_data; - client->auth_requests += lines_skipped; + if (client->auth_replies == client->auth_requests) + new_auth_state = AUTH_COMPLETE; + else + new_auth_state = AUTH_WAITING_FOR_BACKLOG; - if (auth_end >= 0) - { - gsize extra_data; + extra_data = buffer->pos - auth_end; + buffer->size = buffer->pos = auth_end; - if (client->auth_replies == client->auth_requests) - new_auth_state = AUTH_COMPLETE; - else - new_auth_state = AUTH_WAITING_FOR_BACKLOG; + /* We may have gotten some extra data which is not part of + the auth handshake, keep it for the next iteration. */ + if (extra_data > 0) + side->extra_input_data = g_bytes_new (buffer->data + buffer->size, extra_data); + } + else if (auth_end == FIND_AUTH_END_ABORT) + { + buffer_unref (buffer); + if (client->proxy->log_messages) + g_print ("Invalid AUTH line, aborting\n"); + side_closed (side); + break; + } + } + else if (side == &client->bus_side) + { + gsize remaining = buffer->pos; + guint8 *line_start = buffer->data; - extra_data = buffer->pos - auth_end; - buffer->size = buffer->pos = auth_end; + while (remaining > 0) + { + guint8 *line_end = NULL; - /* We may have gotten some extra data which is not part of - the auth handshake, keep it for the next iteration. */ - if (extra_data > 0) - side->extra_input_data = g_bytes_new (buffer->data + buffer->size, extra_data); - } - else if (auth_end == FIND_AUTH_END_ABORT) + if (client->auth_replies == client->auth_requests) { buffer_unref (buffer); if (client->proxy->log_messages) - g_print ("Invalid AUTH line, aborting\n"); + g_print ("Unexpected auth reply line from bus, aborting\n"); side_closed (side); break; } - } - else if (side == &client->bus_side) - { - gsize remaining = buffer->pos; - guint8 *line_start = buffer->data; - while (remaining > 0) + line_end = find_auth_line_end (line_start, remaining); + if (line_end == NULL) + line_end = line_start + remaining; + else { - guint8 *line_end = NULL; - - if (client->auth_replies == client->auth_requests) - { - buffer_unref (buffer); - if (client->proxy->log_messages) - g_print ("Unexpected auth reply line from bus, aborting\n"); - side_closed (side); - break; - } - - line_end = find_auth_line_end (line_start, remaining); - if (line_end == NULL) - line_end = line_start + remaining; - else - { - line_end += strlen (AUTH_LINE_SENTINEL); - client->auth_replies++; - } - - remaining -= line_end - line_start; - line_start = line_end; - - - if (client->auth_state == AUTH_WAITING_FOR_BACKLOG && - client->auth_replies == client->auth_requests) - { - new_auth_state = AUTH_COMPLETE; - /* We may have added extra data on the input, ensure we read it directly */ - wake_client_reader = TRUE; - - buffer->pos = buffer->size = line_start - buffer->data; - - /* We may have gotten some extra data which is not part of - the auth handshake, keep it for the next iteration. */ - if (remaining > 0) - side->extra_input_data = g_bytes_new (line_start, remaining); - - break; - } + line_end += strlen (AUTH_LINE_SENTINEL); + client->auth_replies++; } - } - got_buffer_from_side (side, buffer); + remaining -= line_end - line_start; + line_start = line_end; - client->auth_state = new_auth_state; - } - else - { - buffer_unref (buffer); + if (client->auth_state == AUTH_WAITING_FOR_BACKLOG && + client->auth_replies == client->auth_requests) + { + new_auth_state = AUTH_COMPLETE; + /* We may have added extra data on the input, ensure we read it directly */ + wake_client_reader = TRUE; + + buffer->pos = buffer->size = line_start - buffer->data; + + /* We may have gotten some extra data which is not part of + the auth handshake, keep it for the next iteration. */ + if (remaining > 0) + side->extra_input_data = g_bytes_new (line_start, remaining); + + break; + } + } } + + got_buffer_from_side (side, buffer); + + client->auth_state = new_auth_state; } else if (buffer->pos == buffer->size) {