Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split proxy/client serials based on lower bit #57

Merged
merged 3 commits into from
May 7, 2024
Merged

Split proxy/client serials based on lower bit #57

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4f081c7
Split proxy/client serials based on lower bit
sophie-h Apr 7, 2024
22d8333
Allow for higher number of client serials
sophie-h May 7, 2024
4e7f20e
Update comment about serials for fake messsages
sophie-h May 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Split proxy/client serials based on lower bit 
Separation between messages created by client and proxy was ensured by
requiring the client to use monotonically increasing serials and adding
an offset to distinguish the client message from proxy messages.

The requirement to only send messages with increasing serials cannot be
ensured by libraries godbus or zbus.

This commit instead reserves the high-bit=0 space for client messages
and the high-bit=1 for messages created by the proxy. This gets rid of
any serial translation mechanism and the requirement for increasing serials.
However, it limits the possible values of serials available to the client
to about half of the usual maximum value.

Closes #46
  • Loading branch information
@sophie-h
sophie-h committed Apr 27, 2024
commit 4f081c72de254ecdeedaa126dfcc21fa53557c25
74 changes: 27 additions & 47 deletions flatpak-proxy.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,12 @@
* reply_serials, i.e. that a reply can only be sent once and by the real
* recipient of an previously sent method call.
*
* We don't however trust the serials from the client. We verify that
* they are strictly increasing to make sure the code is not confused
* by serials being reused.
* Serial numbers with high-bit=1 are reserved for messages created by the
* proxy itself (fake messages). This limits the possible values of serials
* available to the client to half of the usual maximum value. Versions
* older than 0.1.6 required monotonically increasing serials instead. This
* mechanism was dropped since it caused regular issues with multiple D-Bus
* clients.
*
* In order to track the ownership of the allowed names we hijack the
* connection after the initial Hello message, sending AddMatch,
sophie-h marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -192,6 +195,8 @@ typedef struct FlatpakProxyClient FlatpakProxyClient;
#define AUTH_LINE_SENTINEL "\r\n"
#define AUTH_BEGIN "BEGIN"

#define MAX_CLIENT_SERIAL (G_MAXUINT32 / 2)

typedef enum {
EXPECTED_REPLY_NONE,
EXPECTED_REPLY_NORMAL,
Expand Down Expand Up @@ -292,9 +297,8 @@ struct FlatpakProxyClient
ProxySide bus_side;

/* Filtering data: */
guint32 serial_offset;
guint32 hello_serial;
guint32 last_serial;
guint32 last_fake_serial;
GHashTable *rewrite_reply;
GHashTable *get_owner_reply;

Expand Down Expand Up @@ -442,6 +446,7 @@ flatpak_proxy_client_init (FlatpakProxyClient *client)
init_side (client, &client->client_side);
init_side (client, &client->bus_side);

client->last_fake_serial = MAX_CLIENT_SERIAL;
client->auth_buffer = g_byte_array_new ();
client->rewrite_reply = g_hash_table_new_full (g_direct_hash, g_direct_equal, NULL, g_object_unref);
client->get_owner_reply = g_hash_table_new_full (g_direct_hash, g_direct_equal, NULL, g_free);
Expand Down Expand Up @@ -1068,15 +1073,6 @@ read_uint32 (Header *header, guint8 *ptr)
return GUINT32_FROM_LE (*(guint32 *) ptr);
}

static void
write_uint32 (Header *header, guint8 *ptr, guint32 val)
{
if (header->big_endian)
*(guint32 *) ptr = GUINT32_TO_BE (val);
else
*(guint32 *) ptr = GUINT32_TO_LE (val);
}

static inline guint32
align_by_8 (guint32 offset)
{
Expand Down Expand Up @@ -1185,12 +1181,11 @@ header_debug_str (GString *s, Header *header)
}

static Header *
parse_header (Buffer *buffer, guint32 serial_offset, guint32 reply_serial_offset, guint32 hello_serial, GError **error)
parse_header (Buffer *buffer, GError **error)
{
guint32 array_len, header_len;
guint32 offset, end_offset;
guint8 header_type;
guint32 reply_serial_pos = 0;
const char *signature;
g_autoptr(GError) str_error = NULL;
g_autoptr(GString) header_str = NULL;
Expand Down Expand Up @@ -1417,7 +1412,6 @@ parse_header (Buffer *buffer, guint32 serial_offset, guint32 reply_serial_offset
}

header->has_reply_serial = TRUE;
reply_serial_pos = offset;
header->reply_serial = read_uint32 (header, &buffer->data[offset]);
offset += 4;
break;
Expand Down Expand Up @@ -1593,17 +1587,6 @@ parse_header (Buffer *buffer, guint32 serial_offset, guint32 reply_serial_offset
return NULL;
}

if (serial_offset > 0)
{
header->serial += serial_offset;
write_uint32 (header, &buffer->data[8], header->serial);
}

if (reply_serial_offset > 0 &&
header->has_reply_serial &&
header->reply_serial > hello_serial + reply_serial_offset)
sophie-h marked this conversation as resolved.
Show resolved Hide resolved
write_uint32 (header, &buffer->data[reply_serial_pos], header->reply_serial - reply_serial_offset);

return g_steal_pointer (&header);
}

Expand Down Expand Up @@ -1850,7 +1833,7 @@ get_error_for_header (FlatpakProxyClient *client, Header *header, const char *er
reply = g_dbus_message_new ();
g_dbus_message_set_message_type (reply, G_DBUS_MESSAGE_TYPE_ERROR);
g_dbus_message_set_flags (reply, G_DBUS_MESSAGE_FLAGS_NO_REPLY_EXPECTED);
g_dbus_message_set_reply_serial (reply, header->serial - client->serial_offset);
g_dbus_message_set_reply_serial (reply, header->serial);
g_dbus_message_set_error_name (reply, error);
g_dbus_message_set_body (reply, g_variant_new ("(s)", error));

Expand All @@ -1865,7 +1848,7 @@ get_bool_reply_for_header (FlatpakProxyClient *client, Header *header, gboolean
reply = g_dbus_message_new ();
g_dbus_message_set_message_type (reply, G_DBUS_MESSAGE_TYPE_METHOD_RETURN);
g_dbus_message_set_flags (reply, G_DBUS_MESSAGE_FLAGS_NO_REPLY_EXPECTED);
g_dbus_message_set_reply_serial (reply, header->serial - client->serial_offset);
g_dbus_message_set_reply_serial (reply, header->serial);
g_dbus_message_set_body (reply, g_variant_new ("(b)", val));

return reply;
Expand Down Expand Up @@ -2293,14 +2276,14 @@ queue_fake_message (FlatpakProxyClient *client, GDBusMessage *message, ExpectedR
{
Buffer *buffer;

client->last_serial++;
client->serial_offset++;
g_dbus_message_set_serial (message, client->last_serial);
client->last_fake_serial++;
g_assert (client->last_fake_serial > MAX_CLIENT_SERIAL);
g_dbus_message_set_serial (message, client->last_fake_serial);
buffer = message_to_buffer (message);
g_object_unref (message);

queue_outgoing_buffer (&client->bus_side, buffer);
queue_expected_reply (&client->client_side, client->last_serial, reply_type);
queue_expected_reply (&client->client_side, client->last_fake_serial, reply_type);
}
sophie-h marked this conversation as resolved.
Show resolved Hide resolved

/* After the first Hello message we need to synthesize a bunch of messages to synchronize the
Expand Down Expand Up @@ -2346,18 +2329,18 @@ queue_initial_name_ops (FlatpakProxyClient *client)
queue_fake_message (client, message, EXPECTED_REPLY_FILTER);

if (client->proxy->log_messages)
g_print ("C%d: -> org.freedesktop.DBus fake %sAddMatch for %s\n", client->last_serial, name_needs_subtree ? "wildcarded " : "", name);
g_print ("C%d: -> org.freedesktop.DBus fake %sAddMatch for %s\n", client->last_fake_serial, name_needs_subtree ? "wildcarded " : "", name);

if (!name_needs_subtree)
{
/* Get the current owner of the name (if any) so we can apply policy to it */
message = g_dbus_message_new_method_call ("org.freedesktop.DBus", "/", "org.freedesktop.DBus", "GetNameOwner");
g_dbus_message_set_body (message, g_variant_new ("(s)", name));
queue_fake_message (client, message, EXPECTED_REPLY_FAKE_GET_NAME_OWNER);
g_hash_table_replace (client->get_owner_reply, GINT_TO_POINTER (client->last_serial), g_strdup (name));
g_hash_table_replace (client->get_owner_reply, GINT_TO_POINTER (client->last_fake_serial), g_strdup (name));

if (client->proxy->log_messages)
g_print ("C%d: -> org.freedesktop.DBus fake GetNameOwner for %s\n", client->last_serial, name);
g_print ("C%d: -> org.freedesktop.DBus fake GetNameOwner for %s\n", client->last_fake_serial, name);
}
else
has_wildcards = TRUE; /* Send ListNames below */
Expand All @@ -2375,7 +2358,7 @@ queue_initial_name_ops (FlatpakProxyClient *client)
queue_fake_message (client, message, EXPECTED_REPLY_FAKE_LIST_NAMES);

if (client->proxy->log_messages)
g_print ("C%d: -> org.freedesktop.DBus fake ListNames\n", client->last_serial);
g_print ("C%d: -> org.freedesktop.DBus fake ListNames\n", client->last_fake_serial);

/* Stop reading from the client, to avoid incoming messages fighting with the ListNames roundtrip.
We will start it again once we have handled the ListNames reply */
Expand Down Expand Up @@ -2417,10 +2400,10 @@ queue_wildcard_initial_name_ops (FlatpakProxyClient *client, Header *header, Buf
GDBusMessage *message = g_dbus_message_new_method_call ("org.freedesktop.DBus", "/", "org.freedesktop.DBus", "GetNameOwner");
g_dbus_message_set_body (message, g_variant_new ("(s)", name));
queue_fake_message (client, message, EXPECTED_REPLY_FAKE_GET_NAME_OWNER);
g_hash_table_replace (client->get_owner_reply, GINT_TO_POINTER (client->last_serial), g_strdup (name));
g_hash_table_replace (client->get_owner_reply, GINT_TO_POINTER (client->last_fake_serial), g_strdup (name));

if (client->proxy->log_messages)
g_print ("C%d: -> org.freedesktop.DBus fake GetNameOwner for %s\n", client->last_serial, name);
g_print ("C%d: -> org.freedesktop.DBus fake GetNameOwner for %s\n", client->last_fake_serial, name);
}
}
}
Expand All @@ -2439,7 +2422,7 @@ got_buffer_from_client (FlatpakProxyClient *client, ProxySide *side, Buffer *buf

/* Filter and rewrite outgoing messages as needed */

header = parse_header (buffer, client->serial_offset, 0, 0, &error);
header = parse_header (buffer, &error);
if (header == NULL)
{
g_warning ("Invalid message header format from client: %s",
Expand All @@ -2452,16 +2435,13 @@ got_buffer_from_client (FlatpakProxyClient *client, ProxySide *side, Buffer *buf
if (!update_socket_messages (side, buffer, header))
return;

/* Make sure the client is not playing games with the serials, as that
could confuse us. */
if (header->serial <= client->last_serial)
if (header->serial > MAX_CLIENT_SERIAL)
{
g_warning ("Invalid client serial");
g_warning ("Invalid client serial: Exceeds maximum value of %u", MAX_CLIENT_SERIAL);
side_closed (side);
buffer_unref (buffer);
return;
}
client->last_serial = header->serial;
sophie-h marked this conversation as resolved.
Show resolved Hide resolved

if (client->proxy->log_messages)
print_outgoing_header (header);
Expand Down Expand Up @@ -2611,7 +2591,7 @@ got_buffer_from_bus (FlatpakProxyClient *client, ProxySide *side, Buffer *buffer

/* Filter and rewrite incoming messages as needed */

header = parse_header (buffer, 0, client->serial_offset, client->hello_serial, &error);
header = parse_header (buffer, &error);
if (header == NULL)
{
g_warning ("Invalid message header format from bus: %s",
Expand Down