From: Colin Walters Date: Fri, 20 Oct 2017 18:15:14 +0000 (-0400) Subject: lib/commit: Fix hardlink checkout commit with bare-user + mod xattrs X-Git-Tag: archive/raspbian/2022.1-3+rpi1~1^2~4^2~30^2~22 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=ed15723cd1688a7f2c003c7cbc95be202166c33d;p=ostree.git lib/commit: Fix hardlink checkout commit with bare-user + mod xattrs This is more subtle fallout from: https://github.com/ostreedev/ostree/pull/1170 AKA commit: 8fe45362578a43260876134d6547ebd0bb2485c3 Before, if we found a devino cache hit, we'd use it unconditionally. Recall that `bare-user` repositories are very special in that they're the only mode where the on disk state ("physical state") is not the "real" state. The latter is stored in the `user.ostreemeta` xattr. (`bare-user` repos are also highly special in that symlinks are regular files physically, but that's not immediately relevant here). Since we now have `bare-user-only` for the "pure unprivileged container" case, `bare-user` should just be used for "OS builds" which have nonzero uids (and possibly SELinux labels etc.) In an experimental tool I'm writing "skopeo2ostree" which imports OCI images into refs, then squashes them together into a single final commit, we lost the the `81` group ID for `/usr/libexec/dbus-1/dbus-daemon-launch-helper`. This happened because the commit code was loading the "physical" disk state, where the uid/gid are zero because that's the uid I happened to be using. We didn't just directly do the link speedup because I was using `--selinux-policy` which caused the xattrs to change, which caused us to re-commit objects from the physical state. The unit test I added actually doesn't quite trigger this, but I left it because "why not". Really testing this requires the installed test which uses SELinux policy from `/`. The behavior without this fix looks like: ``` -00755 0 0 12 { [(b'user.ostreemeta', [byte 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x51, 0x00, 0x00, 0x81, 0xed]), (b'security.selinux', b'system_u:object_r:lib_t:s0')] } /usr/lib/dbus-daemon-helper ``` which was obviously totally broken - we shouldn't be picking up the `user.ostreemeta` xattr and actually committing it of course. Closes: #1297 Approved by: jlebon --- diff --git a/src/libostree/ostree-repo-commit.c b/src/libostree/ostree-repo-commit.c index d6a425d8..cf1a513f 100644 --- a/src/libostree/ostree-repo-commit.c +++ b/src/libostree/ostree-repo-commit.c @@ -2543,6 +2543,7 @@ get_final_xattrs (OstreeRepo *self, GFile *path, int dfd, const char *dfd_subpath, + GVariant *source_xattrs, GVariant **out_xattrs, gboolean *out_modified, GCancellable *cancellable, @@ -2558,7 +2559,9 @@ get_final_xattrs (OstreeRepo *self, g_autoptr(GVariant) original_xattrs = NULL; if (!skip_xattrs && !self->disable_xattrs) { - if (path && OSTREE_IS_REPO_FILE (path)) + if (source_xattrs) + original_xattrs = g_variant_ref (source_xattrs); + else if (path && OSTREE_IS_REPO_FILE (path)) { if (!ostree_repo_file_get_xattrs (OSTREE_REPO_FILE (path), &original_xattrs, cancellable, error)) @@ -2691,11 +2694,35 @@ write_directory_content_to_mtree_internal (OstreeRepo *self, { g_assert (dir_enum != NULL || dfd_iter != NULL); + GFileType file_type = g_file_info_get_file_type (child_info); + const char *name = g_file_info_get_name (child_info); g_ptr_array_add (path, (char*)name); g_autofree char *child_relpath = ptrarray_path_join (path); + /* See if we have a devino hit; this is used below. Further, for bare-user + * repos we'll reload our file info from the object (specifically the + * ostreemeta xattr). The on-disk state is not normally what we want to + * commit. Basically we're making sure that we pick up "real" uid/gid and any + * xattrs there. + */ + const char *loose_checksum = NULL; + g_autoptr(GVariant) source_xattrs = NULL; + if (dfd_iter != NULL && (file_type != G_FILE_TYPE_DIRECTORY)) + { + guint32 dev = g_file_info_get_attribute_uint32 (child_info, "unix::device"); + guint64 inode = g_file_info_get_attribute_uint64 (child_info, "unix::inode"); + loose_checksum = devino_cache_lookup (self, modifier, dev, inode); + if (loose_checksum && self->mode == OSTREE_REPO_MODE_BARE_USER) + { + child_info = NULL; + if (!ostree_repo_load_file (self, loose_checksum, NULL, &child_info, &source_xattrs, + cancellable, error)) + return FALSE; + } + } + g_autoptr(GFileInfo) modified_info = NULL; OstreeRepoCommitFilterResult filter_result = _ostree_repo_commit_modifier_apply (self, modifier, child_relpath, child_info, &modified_info); @@ -2723,7 +2750,6 @@ write_directory_content_to_mtree_internal (OstreeRepo *self, return TRUE; } - GFileType file_type = g_file_info_get_file_type (child_info); switch (file_type) { case G_FILE_TYPE_DIRECTORY: @@ -2801,7 +2827,7 @@ write_directory_content_to_mtree_internal (OstreeRepo *self, if (dir_enum != NULL) { if (!get_final_xattrs (self, modifier, child_relpath, child_info, child, - -1, name, &xattrs, &xattrs_were_modified, + -1, name, source_xattrs, &xattrs, &xattrs_were_modified, cancellable, error)) return FALSE; } @@ -2813,20 +2839,14 @@ write_directory_content_to_mtree_internal (OstreeRepo *self, int xattr_fd_arg = (file_input_fd != -1) ? file_input_fd : dfd_iter->fd; const char *xattr_path_arg = (file_input_fd != -1) ? NULL : name; if (!get_final_xattrs (self, modifier, child_relpath, child_info, child, - xattr_fd_arg, xattr_path_arg, &xattrs, &xattrs_were_modified, + xattr_fd_arg, xattr_path_arg, source_xattrs, + &xattrs, &xattrs_were_modified, cancellable, error)) return FALSE; } - /* only check the devino cache if the file info & xattrs were not modified */ + /* Used below to see whether we can do a fast path commit */ const gboolean modified_file_meta = child_info_was_modified || xattrs_were_modified; - const char *loose_checksum = NULL; - if (!modified_file_meta) - { - guint32 dev = g_file_info_get_attribute_uint32 (child_info, "unix::device"); - guint64 inode = g_file_info_get_attribute_uint64 (child_info, "unix::inode"); - loose_checksum = devino_cache_lookup (self, modifier, dev, inode); - } /* A big prerequisite list of conditions for whether or not we can * "adopt", i.e. just checksum and rename() into place @@ -2856,7 +2876,7 @@ write_directory_content_to_mtree_internal (OstreeRepo *self, gboolean did_adopt = FALSE; /* The very fast path - we have a devino cache hit, nothing to write */ - if (loose_checksum) + if (loose_checksum && !modified_file_meta) { if (!ostree_mutable_tree_replace_file (mtree, name, loose_checksum, error)) @@ -2979,7 +2999,7 @@ write_directory_to_mtree_internal (OstreeRepo *self, if (filter_result == OSTREE_REPO_COMMIT_FILTER_ALLOW) { if (!get_final_xattrs (self, modifier, relpath, child_info, dir, -1, NULL, - &xattrs, NULL, cancellable, error)) + NULL, &xattrs, NULL, cancellable, error)) return FALSE; g_autofree guchar *child_file_csum = NULL; @@ -3065,7 +3085,7 @@ write_dfd_iter_to_mtree_internal (OstreeRepo *self, if (filter_result == OSTREE_REPO_COMMIT_FILTER_ALLOW) { if (!get_final_xattrs (self, modifier, relpath, modified_info, NULL, src_dfd_iter->fd, - NULL, &xattrs, NULL, cancellable, error)) + NULL, NULL, &xattrs, NULL, cancellable, error)) return FALSE; if (!_ostree_repo_write_directory_meta (self, modified_info, xattrs, &child_file_csum, diff --git a/tests/installed/itest-bare-user-root.sh b/tests/installed/itest-bare-user-root.sh new file mode 100755 index 00000000..f5feac96 --- /dev/null +++ b/tests/installed/itest-bare-user-root.sh @@ -0,0 +1,39 @@ +#!/bin/bash + +# Tests of the "raw ostree" functionality using the host's ostree repo as uid 0. + +set -xeuo pipefail + +dn=$(dirname $0) +. ${dn}/libinsttest.sh + +echo "1..1" + +prepare_tmpdir +ostree --repo=repo init --mode=bare-user +mkdir -p components/{dbus,systemd}/usr/{bin,lib} +echo dbus binary > components/dbus/usr/bin/dbus-daemon +chmod a+x components/dbus/usr/bin/dbus-daemon +echo dbus lib > components/dbus/usr/lib/libdbus.so.1 +echo dbus helper > components/dbus/usr/lib/dbus-daemon-helper +chmod a+x components/dbus/usr/lib/dbus-daemon-helper +echo systemd binary > components/systemd/usr/bin/systemd +chmod a+x components/systemd/usr/bin/systemd +echo systemd lib > components/systemd/usr/lib/libsystemd.so.1 + +# Make the gid on dbus 81 like fedora, also ensure no xattrs +ostree --repo=repo commit --no-xattrs -b component-dbus --owner-uid 0 --owner-gid 81 --tree=dir=components/dbus +ostree --repo=repo commit --no-xattrs -b component-systemd --owner-uid 0 --owner-gid 0 --tree=dir=components/systemd +rm rootfs -rf +for component in dbus systemd; do + ostree --repo=repo checkout -U -H component-${component} --union rootfs +done +echo 'some rootfs data' > rootfs/usr/lib/cache.txt +# Commit using the host's selinux policy +ostree --repo=repo commit --selinux-policy / -b rootfs --link-checkout-speedup --tree=dir=rootfs +ostree --repo=repo ls rootfs /usr/bin/systemd >ls.txt +assert_file_has_content ls.txt '^-007.. 0 0 .*/usr/bin/systemd' +ostree --repo=repo ls -X rootfs /usr/lib/dbus-daemon-helper >ls.txt +assert_file_has_content ls.txt '^-007.. 0 81 .*security.selinux.*/usr/lib/dbus-daemon-helper' +assert_not_file_has_content ls.txt 'user.ostreemeta' +echo "ok bare-user link-checkout-speedup with modified xattrs maintains uids" diff --git a/tests/test-basic-user.sh b/tests/test-basic-user.sh index 291806c8..bc08b65a 100755 --- a/tests/test-basic-user.sh +++ b/tests/test-basic-user.sh @@ -25,7 +25,7 @@ skip_without_user_xattrs setup_test_repository "bare-user" -extra_basic_tests=4 +extra_basic_tests=5 . $(dirname $0)/basic-test.sh # Reset things so we don't inherit a lot of state from earlier tests @@ -73,3 +73,29 @@ rm test2-checkout -rf $OSTREE checkout -U -H test2-unreadable test2-checkout assert_file_has_mode test2-checkout/unreadable 400 echo "ok bare-user handled unreadable file" + +cd ${test_tmpdir} +mkdir -p components/{dbus,systemd}/usr/{bin,lib} +echo dbus binary > components/dbus/usr/bin/dbus-daemon +chmod a+x components/dbus/usr/bin/dbus-daemon +echo dbus lib > components/dbus/usr/lib/libdbus.so.1 +echo dbus helper > components/dbus/usr/lib/dbus-daemon-helper +chmod a+x components/dbus/usr/lib/dbus-daemon-helper +echo systemd binary > components/systemd/usr/bin/systemd +chmod a+x components/systemd/usr/bin/systemd +echo systemd lib > components/systemd/usr/lib/libsystemd.so.1 + +# Make the gid on dbus 81 like fedora +$OSTREE commit -b component-dbus --owner-uid 0 --owner-gid 81 --tree=dir=components/dbus +$OSTREE commit -b component-systemd --owner-uid 0 --owner-gid 0 --tree=dir=components/systemd +rm rootfs -rf +for component in dbus systemd; do + $OSTREE checkout -U -H component-${component} --union rootfs +done +echo 'some rootfs data' > rootfs/usr/lib/cache.txt +$OSTREE commit -b rootfs --link-checkout-speedup --tree=dir=rootfs +$OSTREE ls rootfs /usr/bin/systemd >ls.txt +assert_file_has_content ls.txt '^-007.. 0 0 .*/usr/bin/systemd' +$OSTREE ls rootfs /usr/lib/dbus-daemon-helper >ls.txt +assert_file_has_content ls.txt '^-007.. 0 81 .*/usr/lib/dbus-daemon-helper' +echo "ok bare-user link-checkout-speedup maintains uids"