Skip to content

Commit

Permalink
core: Always sort incoming xattrs
Browse files Browse the repository at this point in the history 
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.

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: #3343

Signed-off-by: Colin Walters <[email protected]>
  • Loading branch information
@cgwalters
cgwalters committed Nov 27, 2024
1 parent 295841b commit 2be4c52
Show file tree
Hide file tree
Showing 2 changed files with 108 additions and 11 deletions.
62 changes: 57 additions & 5 deletions src/libostree/ostree-core.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,44 @@ 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)
{
g_assert (xattrs);
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
Expand All @@ -324,10 +362,15 @@ _ostree_file_header_new (GFileInfo *file_info, GVariant *xattrs)
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));
else
{
// We always sort the xattrs now to ensure everything is in normal/canonical form.
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));
}

Expand Down Expand Up @@ -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;
Expand Down
57 changes: 51 additions & 6 deletions tests/test-basic-c.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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)
{
Expand Down Expand Up @@ -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 */
{
Expand All @@ -193,12 +201,49 @@ 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";

/* Now test with xattrs */
{
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) xattrs = g_variant_ref_sink (g_variant_builder_end (&builder));
ostree_repo_write_regfile_inline (repo, expected_sha256_with_xattrs, 0, 0, S_IFREG | 0644,
xattrs, (guint8 *)"hi", 2, NULL, &error);
g_assert_no_error (error);
}

/* And now with a swapped order */
{
GVariantBuilder 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) xattrs = g_variant_ref_sink (g_variant_builder_end (&builder));
ostree_repo_write_regfile_inline (repo, expected_sha256_with_xattrs, 0, 0, S_IFREG | 0644,
xattrs, (guint8 *)"hi", 2, NULL, &error);
g_assert_no_error (error);
}
}

Expand Down

0 comments on commit 2be4c52

Please sign in to comment.