core: Always sort incoming xattrs

When recomputing selinux attrs during commit, we weren't sorting,
which could cause various issues like fsck failures.

This is a big hammer; change things so we always canonicalize
(i.e. sort) the incoming xattrs when creating a file header
and directory metadata.

I think almost all places in the code were already keeping
things sorted, but it's better to ensure correctness first.
If we ever have some performance issue (I'm doubtful) we
could add something like `_ostree_file_header_known_canonicalized`
or so.

Closes: https://github.com/ostreedev/ostree/issues/3343
Signed-off-by: Colin Walters <walters@verbum.org>

diff --git a/src/libostree/ostree-core.c b/src/libostree/ostree-core.c
index fb11f85bc67f6e5adb6501d503b50172844f93d1..e67c23f81b17de057b255fac47e97e6e9150c46a 100644 (file)
--- a/src/libostree/ostree-core.c
+++ b/src/libostree/ostree-core.c
@@ -43,6 +43,7 @@ G_STATIC_ASSERT (OSTREE_REPO_MODE_BARE_USER_ONLY == 3);
 G_STATIC_ASSERT (OSTREE_REPO_MODE_BARE_SPLIT_XATTRS == 4);
 
 static GBytes *variant_to_lenprefixed_buffer (GVariant *variant);
+static GVariant *canonicalize_xattrs (GVariant *xattrs);
 
 #define ALIGN_VALUE(this, boundary) \
   ((((unsigned long)(this)) + (((unsigned long)(boundary)) - 1)) \
@@ -302,6 +303,47 @@ ostree_validate_collection_id (const char *collection_id, GError **error)
   return TRUE;
 }
 
+static int
+compare_xattrs (const void *a_pp, const void *b_pp)
+{
+  GVariant *a = *(GVariant **)a_pp;
+  GVariant *b = *(GVariant **)b_pp;
+  const char *name_a;
+  const char *name_b;
+  g_variant_get (a, "(^&ay@ay)", &name_a, NULL);
+  g_variant_get (b, "(^&ay@ay)", &name_b, NULL);
+
+  return strcmp (name_a, name_b);
+}
+
+// Sort xattrs by name
+static GVariant *
+canonicalize_xattrs (GVariant *xattrs)
+{
+  // We always need to provide data, so NULL is canonicalized to the empty array
+  if (xattrs == NULL)
+    return g_variant_ref_sink (g_variant_new_array (G_VARIANT_TYPE ("(ayay)"), NULL, 0));
+
+  g_autoptr (GPtrArray) xattr_array
+      = g_ptr_array_new_with_free_func ((GDestroyNotify)g_variant_unref);
+
+  const guint n = g_variant_n_children (xattrs);
+  for (guint i = 0; i < n; i++)
+    {
+      GVariant *child = g_variant_get_child_value (xattrs, i);
+      g_ptr_array_add (xattr_array, child);
+    }
+
+  g_ptr_array_sort (xattr_array, compare_xattrs);
+
+  GVariantBuilder builder;
+  g_variant_builder_init (&builder, G_VARIANT_TYPE ("a(ayay)"));
+  for (guint i = 0; i < xattr_array->len; i++)
+    g_variant_builder_add_value (&builder, xattr_array->pdata[i]);
+
+  return g_variant_ref_sink (g_variant_builder_end (&builder));
+}
+
 /* The file header is part of the "object stream" format
  * that's not compressed.  It's comprised of uid,gid,mode,
  * and possibly symlink targets from @file_info, as well
@@ -321,13 +363,12 @@ _ostree_file_header_new (GFileInfo *file_info, GVariant *xattrs)
   else
     symlink_target = "";
 
-  g_autoptr (GVariant) tmp_xattrs = NULL;
-  if (xattrs == NULL)
-    tmp_xattrs = g_variant_ref_sink (g_variant_new_array (G_VARIANT_TYPE ("(ayay)"), NULL, 0));
+  // We always sort the xattrs now to ensure everything is in normal/canonical form.
+  g_autoptr (GVariant) tmp_xattrs = canonicalize_xattrs (xattrs);
 
   g_autoptr (GVariant) ret
       = g_variant_new ("(uuuus@a(ayay))", GUINT32_TO_BE (uid), GUINT32_TO_BE (gid),
-                       GUINT32_TO_BE (mode), 0, symlink_target, xattrs ?: tmp_xattrs);
+                       GUINT32_TO_BE (mode), 0, symlink_target, tmp_xattrs);
   return variant_to_lenprefixed_buffer (g_variant_ref_sink (ret));
 }
 
@@ -1111,11 +1152,13 @@ ostree_create_directory_metadata (GFileInfo *dir_info, GVariant *xattrs)
 {
   GVariant *ret_metadata = NULL;
 
+  // We always sort the xattrs now to ensure everything is in normal/canonical form.
+  g_autoptr (GVariant) tmp_xattrs = canonicalize_xattrs (xattrs);
+
   ret_metadata = g_variant_new (
       "(uuu@a(ayay))", GUINT32_TO_BE (g_file_info_get_attribute_uint32 (dir_info, "unix::uid")),
       GUINT32_TO_BE (g_file_info_get_attribute_uint32 (dir_info, "unix::gid")),
-      GUINT32_TO_BE (g_file_info_get_attribute_uint32 (dir_info, "unix::mode")),
-      xattrs ? xattrs : g_variant_new_array (G_VARIANT_TYPE ("(ayay)"), NULL, 0));
+      GUINT32_TO_BE (g_file_info_get_attribute_uint32 (dir_info, "unix::mode")), tmp_xattrs);
   g_variant_ref_sink (ret_metadata);
 
   return ret_metadata;
@@ -2278,21 +2321,30 @@ ostree_validate_structureof_file_mode (guint32 mode, GError **error)
 }
 
 /* Currently ostree imposes no restrictions on xattrs on its own;
- * they can e.g. be arbitrariliy sized or in number.
- * However, we do validate the key is non-empty, as that is known
- * to always fail.
+ * they can e.g. be arbitrariliy sized or in number. The xattrs
+ * must be sorted by name (without duplicates), and keys cannot be empty.
  */
 gboolean
 _ostree_validate_structureof_xattrs (GVariant *xattrs, GError **error)
 {
   const guint n = g_variant_n_children (xattrs);
+  const char *previous = NULL;
   for (guint i = 0; i < n; i++)
     {
-      const guint8 *name;
+      const char *name;
       g_autoptr (GVariant) value = NULL;
       g_variant_get_child (xattrs, i, "(^&ay@ay)", &name, &value);
       if (!*name)
         return glnx_throw (error, "Invalid xattr name (empty or missing NUL) index=%d", i);
+      if (previous)
+        {
+          int cmp = strcmp (previous, name);
+          if (cmp == 0)
+            return glnx_throw (error, "Duplicate xattr name, index=%d", i);
+          else if (cmp > 0)
+            return glnx_throw (error, "Incorrectly sorted xattr name, index=%d", i);
+        }
+      previous = name;
       i++;
     }
   return TRUE;

diff --git a/tests/test-basic-c.c b/tests/test-basic-c.c
index 7124b5ae9387b68931172cd9ff5e3ea226cc59a5..a27dbc8fe8d098734558710f38ea66a254768ec3 100644 (file)
--- a/tests/test-basic-c.c
+++ b/tests/test-basic-c.c
@@ -130,12 +130,12 @@ test_raw_file_to_archive_stream (gconstpointer data)
 }
 
 static gboolean
-hi_content_stream_new (GInputStream **out_stream, guint64 *out_length, GError **error)
+basic_regfile_content_stream_new (const char *contents, GVariant *xattr, GInputStream **out_stream,
+                                  guint64 *out_length, GError **error)
 {
-  static const char hi[] = "hi";
-  const size_t len = sizeof (hi) - 1;
+  const size_t len = strlen (contents);
   g_autoptr (GMemoryInputStream) hi_memstream
-      = (GMemoryInputStream *)g_memory_input_stream_new_from_data (hi, len, NULL);
+      = (GMemoryInputStream *)g_memory_input_stream_new_from_data (contents, len, NULL);
   g_autoptr (GFileInfo) finfo = g_file_info_new ();
   g_file_info_set_attribute_uint32 (finfo, "standard::type", G_FILE_TYPE_REGULAR);
   g_file_info_set_attribute_boolean (finfo, "standard::is-symlink", FALSE);
@@ -147,6 +147,12 @@ hi_content_stream_new (GInputStream **out_stream, guint64 *out_length, GError **
                                             out_length, NULL, error);
 }
 
+static gboolean
+hi_content_stream_new (GInputStream **out_stream, guint64 *out_length, GError **error)
+{
+  return basic_regfile_content_stream_new ("hi", NULL, out_stream, out_length, error);
+}
+
 static void
 test_validate_remotename (void)
 {
@@ -174,6 +180,8 @@ test_object_writes (gconstpointer data)
 
   static const char hi_sha256[]
       = "2301b5923720c3edc1f0467addb5c287fd5559e3e0cd1396e7f1edb6b01be9f0";
+  static const char invalid_hi_sha256[]
+      = "cafebabecafebabecafebabecafebabecafebabecafebabecafebabecafebabe";
 
   /* Successful content write */
   {
@@ -193,12 +201,68 @@ test_object_writes (gconstpointer data)
     hi_content_stream_new (&hi_memstream, &len, &error);
     g_assert_no_error (error);
     g_autofree guchar *csum = NULL;
-    static const char invalid_hi_sha256[]
-        = "cafebabecafebabecafebabecafebabecafebabecafebabecafebabecafebabe";
     g_assert (!ostree_repo_write_content (repo, invalid_hi_sha256, hi_memstream, len, &csum, NULL,
                                           &error));
     g_assert (error);
     g_assert (strstr (error->message, "Corrupted file object"));
+    g_clear_error (&error);
+  }
+
+  /* Test a basic regfile inline write, no xattrs */
+  g_assert (ostree_repo_write_regfile_inline (repo, hi_sha256, 0, 0, S_IFREG | 0644, NULL,
+                                              (guint8 *)"hi", 2, NULL, &error));
+  g_assert_no_error (error);
+
+  /* Test a basic regfile inline write, erroring on checksum */
+  g_assert (!ostree_repo_write_regfile_inline (repo, invalid_hi_sha256, 0, 0, S_IFREG | 0644, NULL,
+                                               (guint8 *)"hi", 2, NULL, &error));
+  g_assert (error != NULL);
+  g_clear_error (&error);
+
+  static const char expected_sha256_with_xattrs[]
+      = "72473a9e8ace75f89f1504137cfb134feb5372bc1d97e04eb5300e24ad836153";
+
+  GVariantBuilder builder;
+  g_variant_builder_init (&builder, G_VARIANT_TYPE ("a(ayay)"));
+  g_variant_builder_add (&builder, "(^ay^ay)", "security.selinux", "foo_t");
+  g_variant_builder_add (&builder, "(^ay^ay)", "user.blah", "somedata");
+  g_autoptr (GVariant) inorder_xattrs = g_variant_ref_sink (g_variant_builder_end (&builder));
+  g_variant_builder_init (&builder, G_VARIANT_TYPE ("a(ayay)"));
+  g_variant_builder_add (&builder, "(^ay^ay)", "user.blah", "somedata");
+  g_variant_builder_add (&builder, "(^ay^ay)", "security.selinux", "foo_t");
+  g_autoptr (GVariant) unsorted_xattrs = g_variant_ref_sink (g_variant_builder_end (&builder));
+
+  /* Now test with xattrs */
+  g_assert (ostree_repo_write_regfile_inline (repo, expected_sha256_with_xattrs, 0, 0,
+                                              S_IFREG | 0644, inorder_xattrs, (guint8 *)"hi", 2,
+                                              NULL, &error));
+  g_assert_no_error (error);
+
+  /* And now with a swapped order */
+  g_assert (ostree_repo_write_regfile_inline (repo, expected_sha256_with_xattrs, 0, 0,
+                                              S_IFREG | 0644, unsorted_xattrs, (guint8 *)"hi", 2,
+                                              NULL, &error));
+  g_assert_no_error (error);
+
+  /* Tests of directory metadata */
+  static const char expected_dirmeta_sha256[]
+      = "f773ab98198d8e46f77be6ffff5fc1920888c0af5794426c3b1461131d509f34";
+  g_autoptr (GFileInfo) fi = g_file_info_new ();
+  g_file_info_set_attribute_uint32 (fi, "unix::uid", 0);
+  g_file_info_set_attribute_uint32 (fi, "unix::gid", 0);
+  g_file_info_set_attribute_uint32 (fi, "unix::mode", (0755 | S_IFDIR));
+  {
+    g_autoptr (GVariant) dirmeta = ostree_create_directory_metadata (fi, inorder_xattrs);
+    ostree_repo_write_metadata (repo, OSTREE_OBJECT_TYPE_DIR_META, expected_dirmeta_sha256, dirmeta,
+                                NULL, NULL, &error);
+    g_assert_no_error (error);
+  }
+  /* And now with unsorted xattrs */
+  {
+    g_autoptr (GVariant) dirmeta = ostree_create_directory_metadata (fi, unsorted_xattrs);
+    ostree_repo_write_metadata (repo, OSTREE_OBJECT_TYPE_DIR_META, expected_dirmeta_sha256, dirmeta,
+                                NULL, NULL, &error);
+    g_assert_no_error (error);
   }
 }