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

[prydom]

Putting a pointer to containers/bootc#920 (comment) here. Please read the initial report there for a minimal reproduction script. The observed behavior was:

• Failed container pulls via bootc and failing ostree fsck on a bootable base image created from an ostree commit.
• Successful pulls but subsequent failures of ostree fsck if further container layers added xattrs.

I originally filed in bootc since I thought the bug was in the ostree-ext-rs crate but based on the following analysis I think the issue is in OSTree.

I think I can answer why this happens for the "pure ostree" case, @cgwalters. It seems the root cause has been in https://github.com/ostreedev/ostree for a while and it may just be some coincidental changes in libdnf5, librpm or librepo which caused this to start failing container pulls now.

libglnx will always sort the extended attributes returned from glnx_dfd_name_get_all_xattrs with canonicalize_xattrs(). This ensures we should always get the same content hash as long as the same file contents + uid/gid/perms/xattrs match even if the underlying filesystem returns a different order.

However, during commit the way get_final_xattrs() works is to first delete the security.selinux attribute with _ostree_filter_selinux_xattr() and then append the new label to the end of the gvariant array.

Linux capabilities have been working because "security.capabilities" always comes before "security.selinux". However the existence of any "user.*" attributes means that list will no longer be sorted and therefore the hash calculated at commit time will not match what is stored in the bare repo and what is copied into the encapsulated commit container image.

So, preserving system.* (e.g. ACLs) or trusted.* extended attributes would be bit by this same bug. I have reproduced the same failure to pull the container by adding an ACL.

# Create a new commit with just a single file with an extended attribute
mkdir commit
cd commit
echo "hello world" > test.txt
setfattr -n security.selinux -v "system_u:object_r:etc_runtime_t:s0" test.txt
setfacl -m "u:root:rw-" test.txt
setcap 'cap_net_bind_service=ep' test.txt
getfattr -d -m - -- *
stat test.txt
cd ..

[cgwalters]

Thanks for that analysis! Indeed this looks like a basic bug in core ostree, we need to always ensure the xattrs are sorted. (composefs does this)

I recently hit a related bug in #3261 - and so a good first step here would be to change our object writing code and fsck code to enforce this.

[prydom]

@cgwalters, based on my recent test of #3346 it looks like we might need an accompanying change to ostree-ext-rs/bootc to ensure xattrs introduced by container layering are also sorted.

I undid my workaround that stripped all the user.* attributes from buildah layered images during that test.