CVE-2018-6560: Interpretation Conflict (CWE-436)
value: In dbus-proxy/flatpak-proxy.c in Flatpak before 0.8.9, and 0.9.x and 0.10.x before 0.10.3, crafted D-Bus messages to the host can be used to break out of the sandbox, because whitespace handling in the proxy is not identical to whitespace handling in the daemon.
CWE description: Product A handles inputs or steps differently than Product B, which causes A to perform incorrect actions based on its perception of B's state.
CVSS v3 base score: 8.8
Published: 2018-02-02T14:29Z
Repository: https://github.com/flatpak/flatpak
Language: C
Files changed: dbus-proxy/flatpak-proxy.c
Commit date: 2018-01-30 12:06:14 +0100
Commit message: Fix vulnerability in dbus proxy

During the authentication all client data is directly forwarded
to the dbus daemon as is, until we detect the BEGIN command after
which we start filtering the binary dbus protocol.

Unfortunately the detection of the BEGIN command in the proxy
did not exactly match the detection in the dbus daemon. A BEGIN
followed by a space or tab was considered ok in the daemon but
not by the proxy. This could be exploited to send arbitrary
dbus messages to the host, which can be used to break out of
the sandbox.

This was noticed by Gabriel Campana of The Google Security Team.

This fix makes the detection of the authentication phase end
match the dbus code. In addition we duplicate the authentication
line validation from dbus, which includes ensuring all data is
ASCII, and limiting the size of a line to 16k. In fact, we add
some extra stringent checks, disallowing ASCII control chars and
requiring that auth lines start with a capital letter.

Diff: diff --git a/dbus-proxy/flatpak-proxy.c b/dbus-proxy/flatpak-proxy.c
index ee884054..ff5fe455 100644
--- a/dbus-proxy/flatpak-proxy.c
+++ b/dbus-proxy/flatpak-proxy.c
@@ -171,14 +171,15 @@
  * For unknown methods, we return a synthetic error.
  */
 
 typedef struct FlatpakProxyClient FlatpakProxyClient;
 
-/* We start looking ignoring the first cr-lf
-   since there is no previous line initially */
-#define AUTH_END_INIT_OFFSET 2
-#define AUTH_END_STRING "\r\nBEGIN\r\n"
+#define FIND_AUTH_END_CONTINUE -1
+#define FIND_AUTH_END_ABORT -2
+
+#define AUTH_LINE_SENTINEL "\r\n"
+#define AUTH_BEGIN "BEGIN"
 
 typedef enum {
   EXPECTED_REPLY_NONE,
   EXPECTED_REPLY_NORMAL,
   EXPECTED_REPLY_HELLO,
@@ -256,11 +257,11 @@ struct FlatpakProxyClient
   GObject       parent;
 
   FlatpakProxy *proxy;
 
   gboolean      authenticated;
-  int           auth_end_offset;
+  GByteArray   *auth_buffer;
 
   ProxySide     client_side;
   ProxySide     bus_side;
 
   /* Filtering data: */
@@ -370,10 +371,11 @@ flatpak_proxy_client_finalize (GObject *object)
   FlatpakProxyClient *client = FLATPAK_PROXY_CLIENT (object);
 
   client->proxy->clients = g_list_remove (client->proxy->clients, client);
   g_clear_object (&client->proxy);
 
+  g_byte_array_free (client->auth_buffer, TRUE);
   g_hash_table_destroy (client->rewrite_reply);
   g_hash_table_destroy (client->get_owner_reply);
   g_hash_table_destroy (client->unique_id_policy);
 
   free_side (&client->client_side);
@@ -405,11 +407,11 @@ static void
 flatpak_proxy_client_init (FlatpakProxyClient *client)
 {
   init_side (client, &client->client_side);
   init_side (client, &client->bus_side);
 
-  client->auth_end_offset = AUTH_END_INIT_OFFSET;
+  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);
   client->unique_id_policy = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL);
 }
 
@@ -2313,55 +2315,96 @@ got_buffer_from_side (ProxySide *side, Buffer *buffer)
     got_buffer_from_client (client, side, buffer);
   else
     got_buffer_from_bus (client, side, buffer);
 }
 
+#define _DBUS_ISASCII(c) ((c) != '\0' && (((c) & ~0x7f) == 0))
+
+static gboolean
+auth_line_is_valid (guint8 *line, guint8 *line_end)
+{
+  guint8 *p;
+
+  for (p = line; p < line_end; p++)
+    {
+      if (!_DBUS_ISASCII(*p))
+        return FALSE;
+
+      /* Technically, the dbus spec allows all ASCII characters, but for robustness we also
+         fail if we see any control characters. Such low values will appear in  potential attacks,
+         but will never happen in real sasl (where all binary data is hex encoded). */
+      if (*p < ' ')
+        return FALSE;
+    }
+
+  /* For robustness we require the first char of the line to be an upper case letter.
+     This is not technically required by the dbus spec, but all commands are upper
+     case, and there is no provisioning for whitespace before the command, so in practice
+     this is true, and this means we're not confused by e.g. initial whitespace. */
+  if (line[0] < 'A' || line[0] > 'Z')
+    return FALSE;
+
+  return TRUE;
+}
+
+static gboolean
+auth_line_is_begin (guint8 *line)
+{
+  guint8 next_char;
+
+  if (!g_str_has_prefix ((char *)line, AUTH_BEGIN))
+    return FALSE;
+
+  /* dbus-daemon accepts either nothing, or a whitespace followed by anything as end of auth */
+  next_char = line[strlen (AUTH_BEGIN)];
+  return (next_char == 0 ||
+          next_char == ' ' ||
+          next_char == '\t');
+}
+
 static gssize
 find_auth_end (FlatpakProxyClient *client, Buffer *buffer)
 {
-  guchar *match;
-  int i;
+  goffset offset = 0;
+  gsize original_size = client->auth_buffer->len;
+
+  /* Add the new data to the remaining data from last iteration */
+  g_byte_array_append (client->auth_buffer, buffer->data, buffer->pos);
 
-  /* First try to match any leftover at the start */
-  if (client->auth_end_offset > 0)
+  while (TRUE)
     {
-      gsize left = strlen (AUTH_END_STRING) - client->auth_end_offset;
-      gsize to_match = MIN (left, buffer->pos);
-      /* Matched at least up to to_match */
-      if (memcmp (buffer->data, &AUTH_END_STRING[client->auth_end_offset], to_match) == 0)
+      guint8 *line_start = client->auth_buffer->data + offset;
+      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));
+      if (line_end) /* Found end of line */
         {
-          client->auth_end_offset += to_match;
+          offset = (line_end + strlen (AUTH_LINE_SENTINEL) - line_start);
 
-          /* Matched all */
-          if (client->auth_end_offset == strlen (AUTH_END_STRING))
-            return to_match;
+          if (!auth_line_is_valid (line_start, line_end))
+            return FIND_AUTH_END_ABORT;
 
-          /* Matched to end of buffer */
-          return -1;
-        }
+          *line_end = 0;
+          if (auth_line_is_begin (line_start))
+            return offset - original_size;
 
-      /* Did not actually match at start */
-      client->auth_end_offset = -1;
-    }
+          /* continue with next line */
+        }
+      else
+        {
+          /* No end-of-line in this buffer */
+          g_byte_array_remove_range (client->auth_buffer, 0, offset);
 
-  /* Look for whole match inside buffer */
-  match = memmem (buffer, buffer->pos,
-                  AUTH_END_STRING, strlen (AUTH_END_STRING));
-  if (match != NULL)
-    return match - buffer->data + strlen (AUTH_END_STRING);
+          /* Abort if more than 16k before newline, similar to what dbus-daemon does */
+          if (client->auth_buffer->len >= 16*1024)
+            return FIND_AUTH_END_ABORT;
 
-  /* Record longest prefix match at the end */
-  for (i = MIN (strlen (AUTH_END_STRING) - 1, buffer->pos); i > 0; i--)
-    {
-      if (memcmp (buffer->data + buffer->pos - i, AUTH_END_STRING, i) == 0)
-        {
-          client->auth_end_offset = i;
-          break;
+          return FIND_AUTH_END_CONTINUE;
         }
     }
-
-  return -1;
 }
 
 static gboolean
 side_in_cb (GSocket *socket, GIOCondition condition, gpointer user_data)
 {
@@ -2416,10 +2459,18 @@ side_in_cb (GSocket *socket, GIOCondition condition, gpointer user_data)
                       /* 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;
+                    }
                 }
 
               got_buffer_from_side (side, buffer);
 
               if (found_auth_end)

Vulnerable code: /* We start looking ignoring the first cr-lf
   since there is no previous line initially */
#define AUTH_END_INIT_OFFSET 2
#define AUTH_END_STRING "\r\nBEGIN\r\n"
  int           auth_end_offset;
  client->auth_end_offset = AUTH_END_INIT_OFFSET;
  guchar *match;
  int i;
  /* First try to match any leftover at the start */
  if (client->auth_end_offset > 0)
      gsize left = strlen (AUTH_END_STRING) - client->auth_end_offset;
      gsize to_match = MIN (left, buffer->pos);
      /* Matched at least up to to_match */
      if (memcmp (buffer->data, &AUTH_END_STRING[client->auth_end_offset], to_match) == 0)
          client->auth_end_offset += to_match;
          /* Matched all */
          if (client->auth_end_offset == strlen (AUTH_END_STRING))
            return to_match;
          /* Matched to end of buffer */
          return -1;
        }
      /* Did not actually match at start */
      client->auth_end_offset = -1;
    }
  /* Look for whole match inside buffer */
  match = memmem (buffer, buffer->pos,
                  AUTH_END_STRING, strlen (AUTH_END_STRING));
  if (match != NULL)
    return match - buffer->data + strlen (AUTH_END_STRING);
  /* Record longest prefix match at the end */
  for (i = MIN (strlen (AUTH_END_STRING) - 1, buffer->pos); i > 0; i--)
    {
      if (memcmp (buffer->data + buffer->pos - i, AUTH_END_STRING, i) == 0)
        {
          client->auth_end_offset = i;
          break;

  return -1;
Fixed code: #define FIND_AUTH_END_CONTINUE -1
#define FIND_AUTH_END_ABORT -2

#define AUTH_LINE_SENTINEL "\r\n"
#define AUTH_BEGIN "BEGIN"
  GByteArray   *auth_buffer;
  g_byte_array_free (client->auth_buffer, TRUE);
  client->auth_buffer = g_byte_array_new ();
#define _DBUS_ISASCII(c) ((c) != '\0' && (((c) & ~0x7f) == 0))

static gboolean
auth_line_is_valid (guint8 *line, guint8 *line_end)
{
  guint8 *p;

  for (p = line; p < line_end; p++)
    {
      if (!_DBUS_ISASCII(*p))
        return FALSE;

      /* Technically, the dbus spec allows all ASCII characters, but for robustness we also
         fail if we see any control characters. Such low values will appear in  potential attacks,
         but will never happen in real sasl (where all binary data is hex encoded). */
      if (*p < ' ')
        return FALSE;
    }

  /* For robustness we require the first char of the line to be an upper case letter.
     This is not technically required by the dbus spec, but all commands are upper
     case, and there is no provisioning for whitespace before the command, so in practice
     this is true, and this means we're not confused by e.g. initial whitespace. */
  if (line[0] < 'A' || line[0] > 'Z')
    return FALSE;

  return TRUE;
}

static gboolean
auth_line_is_begin (guint8 *line)
{
  guint8 next_char;

  if (!g_str_has_prefix ((char *)line, AUTH_BEGIN))
    return FALSE;

  /* dbus-daemon accepts either nothing, or a whitespace followed by anything as end of auth */
  next_char = line[strlen (AUTH_BEGIN)];
  return (next_char == 0 ||
          next_char == ' ' ||
          next_char == '\t');
}

  goffset offset = 0;
  gsize original_size = client->auth_buffer->len;

  /* Add the new data to the remaining data from last iteration */
  g_byte_array_append (client->auth_buffer, buffer->data, buffer->pos);
  while (TRUE)
      guint8 *line_start = client->auth_buffer->data + offset;
      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));
      if (line_end) /* Found end of line */
          offset = (line_end + strlen (AUTH_LINE_SENTINEL) - line_start);
          if (!auth_line_is_valid (line_start, line_end))
            return FIND_AUTH_END_ABORT;
          *line_end = 0;
          if (auth_line_is_begin (line_start))
            return offset - original_size;
          /* continue with next line */
        }
      else
        {
          /* No end-of-line in this buffer */
          g_byte_array_remove_range (client->auth_buffer, 0, offset);
          /* Abort if more than 16k before newline, similar to what dbus-daemon does */
          if (client->auth_buffer->len >= 16*1024)
            return FIND_AUTH_END_ABORT;
          return FIND_AUTH_END_CONTINUE;
                  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;
                    }