core: Always sort incoming xattrs #3346

cgwalters · 2024-11-27T15:35:56Z

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

cgwalters · 2024-11-27T15:43:27Z

tests/test-basic-c.c

+    g_assert_no_error (error);
+  }
+
+  /* And now with a swapped order */


And I also verified that this test successfully fails if we drop the xattr canonicalization.

prydom · 2024-11-27T18:56:46Z

I get the following error when pulling an image into ostree layered on top of quay.io/fedora/fedora-kinoite. It has user.* xattrs in the container tar stream.

2024-11-27T18:39:25.0514353Z layers already present: 0; layers needed: 89 (4.5 GB)
2024-11-27T18:40:54.5755174Z error: Importing: Parsing layer blob sha256:4c7aee33c4535de34dfdde394e64174d0177144339e7d35846be7ce1663722f4: error: ostree-tar: Failed to handle directory: Incorrectly sorted xattr name, index=2: Processing tar: Failed to commit tar: ExitStatus(unix_wait_status(256))

EDIT: If you want to see the CI job, it is here: https://github.com/prydom/my-ostree-build/actions/runs/12056266866/job/33618838686. The build container has ostree and ostree-libs build from commit 2be4c52.

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: ostreedev#3343 Signed-off-by: Colin Walters <[email protected]>

cgwalters · 2024-11-27T22:11:15Z

Thanks for testing this out! I pushed an update that also covers directory metadata - only unit tested here so far.

prydom · 2024-11-28T05:01:09Z

I've done another round of end-to-end testing and the changes work well for me.

On github actions:

Updated ostree-libs to c863558 on CI build container.
Built multiple layers upon upstream bootable container image which introduced the librepo extended attributes in tar stream.
Pulled container image into empty bare ostree repository (this step is what failed earlier)
Checked out image with ostree, made some further changes that are not appropriate to do with image layering
Checked image back into new ostree ref and used --selinux-policy= to relabel based on policy in image
Ran rpm-ostree compose container-encapsulate to re-encapsulate commit as image and upload to registry

In podman on dev machine:

Added an additional container image layer with even more extended attributes. Signed & uploaded to registry.
In build container (same as CI) with updated ostree, created empty bare-user repo. Pulled image into repo. ostree fsck passes.
Run ostree fsck on the sysroot in the container created from new image (bare-split-xattrs). Passes.

On target machine:

Used bootc to switch/rebase to new image. Existing host with bootc and the new image does not have updated ostree library. Image pulls successfully.
Booted into staged image successfully.
Confirmed that introduced user xattrs are preserved.
ostree fsck run from the booted image on sysroot fails on files that have user xattrs. This is because the ostree library hasn't been updated in the image.
Activate usr-overlay, install rpms with ostree and ostree-libs from this PR, create new bare repo, pull container into fresh repo and ostree fsck passes.

allisonkarlitskaya · 2024-11-28T09:25:27Z

src/libostree/ostree-core.c

+  GVariant *b = *(GVariant **)b_pp;
+  const char *name_a;
+  const char *name_b;
+  g_variant_get (a, "(^&ay@ay)", &name_a, NULL);


Random: you could just memcmp() the data of the GVariants directly: the key starts at byte 0. That's pretty evil, though.

Otherwise it might be a bit nicer to use g_variant_get_child(a, 0, "&ay", &name_a)

allisonkarlitskaya · 2024-11-28T09:31:49Z

src/libostree/ostree-core.c

+
+// Sort xattrs by name
+static GVariant *
+canonicalize_xattrs (GVariant *xattrs)


Yikes. This function is complicated, but I honestly can't think of any other way to do it more simply...

One thing I'd consider is scanning the "input" to see if it's already in order (trivially true also for the common case of only one item) and return a ref directly in that case.

allisonkarlitskaya · 2024-11-28T09:39:20Z

src/libostree/ostree-core.c

  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);


Pulling the (unused) value out here just to have it auto-freed at the end of the loop is wasteful. You could just give NULL there. This whole loop could also be converted to a GVariantIter.

allisonkarlitskaya · 2024-11-28T09:41:58Z

tests/test-basic-c.c

+      = "72473a9e8ace75f89f1504137cfb134feb5372bc1d97e04eb5300e24ad836153";
+
+  GVariantBuilder builder;
+  g_autoptr (GVariant) inorder_xattrs = NULL;


Could move the declaration down to the same line as the assignment, like you do for unsorted...

allisonkarlitskaya · 2024-11-28T09:43:14Z

tests/test-basic-c.c

+  g_autoptr (GVariant) unsorted_xattrs = g_variant_ref_sink (g_variant_builder_end (&builder));
+
+  /* Now test with xattrs */
+  ostree_repo_write_regfile_inline (repo, expected_sha256_with_xattrs, 0, 0, S_IFREG | 0644,


Did you also want to check the return value?

allisonkarlitskaya · 2024-11-28T09:46:53Z

tests/test-basic-c.c

+  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);
+


I notice that you don't have a testcase for what happens if someone tries to repeat an xattr. If I understand the code above, you'd sort the incoming list, but fail to detect the duplicate, which would later fail to validate.

Do you want to add that check as well?

You also don't check for incoming empty xattr key values. That might be another nice thing to figure out, and then add a test for.

cgwalters commented Nov 27, 2024

View reviewed changes

prydom mentioned this pull request Nov 27, 2024

SELinux relabeling during commit causes the wrong content hash to be stored if ACLs or user xattrs are present #3343

Open

cgwalters force-pushed the commit-label-ordering branch from 2be4c52 to c863558 Compare November 27, 2024 22:08

allisonkarlitskaya reviewed Nov 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: Always sort incoming xattrs #3346

core: Always sort incoming xattrs #3346

cgwalters commented Nov 27, 2024

cgwalters Nov 27, 2024

prydom commented Nov 27, 2024 •

edited

Loading

cgwalters commented Nov 27, 2024

prydom commented Nov 28, 2024 •

edited

Loading

allisonkarlitskaya Nov 28, 2024

allisonkarlitskaya Nov 28, 2024

allisonkarlitskaya Nov 28, 2024

allisonkarlitskaya Nov 28, 2024

allisonkarlitskaya Nov 28, 2024

allisonkarlitskaya Nov 28, 2024

allisonkarlitskaya Nov 28, 2024

core: Always sort incoming xattrs #3346

Are you sure you want to change the base?

core: Always sort incoming xattrs #3346

Conversation

cgwalters commented Nov 27, 2024

Choose a reason for hiding this comment

prydom commented Nov 27, 2024 • edited Loading

cgwalters commented Nov 27, 2024

prydom commented Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prydom commented Nov 27, 2024 •

edited

Loading

prydom commented Nov 28, 2024 •

edited

Loading