From 19ba462b57b7e5c67f90cccb8d7c9843beecc921 Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Mon, 17 Jun 2024 21:34:11 +0900 Subject: [PATCH 1/9] tests: Update run-on-arch-action to v2.7.2 Signed-off-by: Akihiko Odaki --- .github/workflows/test.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index 146c200..aa1d9fe 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -20,7 +20,7 @@ jobs: distro: ubuntu20.04 steps: - uses: actions/checkout@v2.1.0 - - uses: uraimo/run-on-arch-action@v2.0.5 + - uses: uraimo/run-on-arch-action@v2.7.2 name: Build id: build with: From b644635d670baed7ac1f89213c45942494e7a386 Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Mon, 17 Jun 2024 21:31:14 +0900 Subject: [PATCH 2/9] tests: Update to Ubuntu 22.04 Signed-off-by: Akihiko Odaki --- .github/workflows/test.yaml | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index aa1d9fe..5bc3a69 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -4,20 +4,20 @@ on: [push, pull_request] jobs: build_job: - runs-on: ubuntu-20.04 + runs-on: ubuntu-22.04 name: Build on ${{ matrix.arch }} strategy: matrix: include: - arch: armv7 - distro: ubuntu20.04 + distro: ubuntu22.04 - arch: aarch64 - distro: ubuntu20.04 + distro: ubuntu22.04 - arch: s390x - distro: ubuntu20.04 + distro: ubuntu22.04 - arch: ppc64le - distro: ubuntu20.04 + distro: ubuntu22.04 steps: - uses: actions/checkout@v2.1.0 - uses: uraimo/run-on-arch-action@v2.7.2 @@ -34,7 +34,7 @@ jobs: install: | apt-get update -q -y - apt-get install -q -y attr automake autotools-dev git make gcc pkg-config xz-utils python3.8 g++ python3-setuptools libdevmapper-dev btrfs-progs libbtrfs-dev go-md2man parallel libfuse3-dev bats + apt-get install -q -y attr automake autotools-dev git make gcc pkg-config xz-utils python3 g++ python3-setuptools libdevmapper-dev btrfs-progs libbtrfs-dev go-md2man parallel libfuse3-dev bats run: | ./autogen.sh @@ -49,7 +49,7 @@ jobs: fuse-overlayfs Test: - runs-on: ubuntu-20.04 + runs-on: ubuntu-22.04 strategy: fail-fast: false matrix: @@ -66,7 +66,7 @@ jobs: - name: install dependencies run: | sudo apt-get update -q -y - sudo apt-get install -q -y attr automake autotools-dev git make gcc pkg-config xz-utils python3.8 g++ python3-setuptools libdevmapper-dev btrfs-progs libbtrfs-dev go-md2man parallel wget libfuse3-dev bats + sudo apt-get install -q -y attr automake autotools-dev git make gcc pkg-config xz-utils python3 g++ python3-setuptools libdevmapper-dev btrfs-progs libbtrfs-dev go-md2man parallel wget libfuse3-dev bats sudo mkdir -p /lower /upper /mnt $GOPATH/src/github.com/containers sudo sh -c "cd $GOPATH/src/github.com/containers; git clone --depth=1 https://github.com/containers/storage" @@ -90,7 +90,7 @@ jobs: - name: Archive build artifacts uses: actions/upload-artifact@v3 with: - name: fuse-overlayfs-x86_64-ubuntu20.04 + name: fuse-overlayfs-x86_64-ubuntu22.04 path: | fuse-overlayfs if: ${{ matrix.test == 'ovl-whiteouts' }} From a13a9e71dc5e6228078c06966088767b978db789 Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Wed, 29 May 2024 15:59:45 +0900 Subject: [PATCH 3/9] utils: Always suppress ENODATA override_mode () used to suppress ENODATA only in a certain condition. ENODATA errors in other situations made load_dir () fail because it indirectly calls override_mode () when the underlying file system reports DT_UNKNOWN for an opaque whiteout file and such an file does not have mode xattrs. do_fchmod () and do_chmod () worked around the problem by supressing ENODATA by themselves, but that led to code duplication. Always suppress ENODATA to resolve these problems. Signed-off-by: Akihiko Odaki --- utils.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/utils.c b/utils.c index 2e014ac..fbe06a4 100644 --- a/utils.c +++ b/utils.c @@ -270,14 +270,10 @@ override_mode (struct ovl_layer *l, int fd, const char *abs_path, const char *pa if (fd >= 0) { ret = fgetxattr (fd, xattr_name, buf, sizeof (buf) - 1); - if (ret < 0) - return ret; } else if (abs_path) { ret = lgetxattr (abs_path, xattr_name, buf, sizeof (buf) - 1); - if (ret < 0) - return ret; } else { @@ -292,16 +288,12 @@ override_mode (struct ovl_layer *l, int fd, const char *abs_path, const char *pa if (fd >= 0) ret = fgetxattr (fd, xattr_name, buf, sizeof (buf) - 1); else - { - ret = lgetxattr (full_path, xattr_name, buf, sizeof (buf) - 1); - if (ret < 0 && errno == ENODATA) - return 0; - } - - if (ret < 0) - return ret; + ret = lgetxattr (full_path, xattr_name, buf, sizeof (buf) - 1); } + if (ret < 0) + return errno == ENODATA ? 0 : ret; + buf[ret] = '\0'; ret = sscanf (buf, "%d:%d:%o", &uid, &gid, &mode); From 136aefd2f6cc9eef67e7a118d2a330aab3c38265 Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Wed, 29 May 2024 16:11:00 +0900 Subject: [PATCH 4/9] main: Fix file owner retrieval for chmod do_fchmod () and do_chmod () used to call override_mode () directly to retrieve the owner information, but the usage of override_mode () was wrong; override_mode () expects struct stat is already populated by the information provided by the underlying filesystem, but do_fchmod () and do_chmod () only zeroed st_uid and st_gid. override_mode () does not update the owner information when st_mode is not S_IFDIR nor S_IFREG so this caused chmod to change the file owner to root at random. Use the logic rpl_stat () employs to file owner retrieval for chmod functions to ensure they provide the owner information consistent with rpl_stat (). Signed-off-by: Akihiko Odaki --- main.c | 53 +++++++++++++++++++++++++---------------------------- 1 file changed, 25 insertions(+), 28 deletions(-) diff --git a/main.c b/main.c index 3a49a1b..fc5cc79 100644 --- a/main.c +++ b/main.c @@ -622,22 +622,32 @@ do_fchownat (struct ovl_data *lo, int dfd, const char *path, uid_t uid, gid_t gi #define fchownat ERROR static int -do_fchmod (struct ovl_data *lo, int fd, mode_t mode) +do_stat (struct ovl_node *node, int fd, const char *path, struct stat *st) +{ + struct ovl_layer *l = node->layer; + + if (fd >= 0) + return l->ds->fstat (l, fd, path, STATX_BASIC_STATS, st); + + if (path != NULL) + return stat (path, st); + + if (node->hidden) + return fstatat (node_dirfd (node), node->path, st, AT_SYMLINK_NOFOLLOW); + + return l->ds->statat (l, node->path, st, AT_SYMLINK_NOFOLLOW, STATX_BASIC_STATS); +} + +static int +do_fchmod (struct ovl_data *lo, struct ovl_node *node, int fd, mode_t mode) { if (lo->xattr_permissions) { - struct ovl_layer *upper = get_upper_layer (lo); struct stat st; - if (upper == NULL) - { - errno = EROFS; - return -1; - } - st.st_uid = 0; st.st_gid = 0; - if (override_mode (upper, fd, NULL, NULL, &st) < 0 && errno != ENODATA) + if (do_stat (node, fd, NULL, &st) < 0) return -1; return write_permission_xattr (lo, fd, NULL, st.st_uid, st.st_gid, mode); @@ -648,22 +658,15 @@ do_fchmod (struct ovl_data *lo, int fd, mode_t mode) #define fchmod ERROR static int -do_chmod (struct ovl_data *lo, const char *path, mode_t mode) +do_chmod (struct ovl_data *lo, struct ovl_node *node, const char *path, mode_t mode) { if (lo->xattr_permissions) { - struct ovl_layer *upper = get_upper_layer (lo); struct stat st; - if (upper == NULL) - { - errno = EROFS; - return -1; - } - st.st_uid = 0; st.st_gid = 0; - if (override_mode (upper, -1, path, NULL, &st) < 0 && errno != ENODATA) + if (do_stat (node, -1, path, &st) < 0) return -1; return write_permission_xattr (lo, -1, path, st.st_uid, st.st_gid, mode); @@ -921,14 +924,8 @@ rpl_stat (fuse_req_t req, struct ovl_node *node, int fd, const char *path, struc if (st_in) memcpy (st, st_in, sizeof (*st)); - else if (fd >= 0) - ret = l->ds->fstat (l, fd, path, STATX_BASIC_STATS, st); - else if (path != NULL) - ret = stat (path, st); - else if (node->hidden) - ret = fstatat (node_dirfd (node), node->path, st, AT_SYMLINK_NOFOLLOW); else - ret = l->ds->statat (l, node->path, st, AT_SYMLINK_NOFOLLOW, STATX_BASIC_STATS); + ret = do_stat (node, fd, path, st); if (ret < 0) return ret; @@ -3823,7 +3820,7 @@ ovl_write_buf (fuse_req_t req, fuse_ino_t ino, /* if it is a writepage request, make sure to restore the setuid bit. */ if (fi->writepage && (inode->mode & (S_ISUID | S_ISGID))) { - if (do_fchmod (lo, fi->fh, inode->mode) < 0) + if (do_fchmod (lo, inode->node, fi->fh, inode->mode) < 0) { fuse_reply_err (req, errno); return; @@ -4135,9 +4132,9 @@ ovl_setattr (fuse_req_t req, fuse_ino_t ino, struct stat *attr, int to_set, stru if (to_set & FUSE_SET_ATTR_MODE) { if (fd >= 0) - ret = do_fchmod (lo, fd, attr->st_mode); + ret = do_fchmod (lo, node, fd, attr->st_mode); else - ret = do_chmod (lo, path, attr->st_mode); + ret = do_chmod (lo, node, path, attr->st_mode); if (ret < 0) { fuse_reply_err (req, errno); From 9610adf7abf9bfb31c69b093e72321c54af35629 Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Wed, 29 May 2024 16:24:21 +0900 Subject: [PATCH 5/9] main: Do not set -1 for owner overriding xattrs ovl_setattr () used to pass -1 as uid or gid when either of them is not changed for do_fchown () / do_chown (), but if these functions use overriding xattrs instead of real fchown () or chown (), it causes -1 to be written in owner overriding xattrs and break them. Replace -1 with the current uid or gid before calling do_fchown () / do_chown (). Signed-off-by: Akihiko Odaki --- main.c | 18 ++++++++++++++++++ tests/unpriv.sh | 18 ++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/main.c b/main.c index fc5cc79..5bd1993 100644 --- a/main.c +++ b/main.c @@ -4158,6 +4158,24 @@ ovl_setattr (fuse_req_t req, fuse_ino_t ino, struct stat *attr, int to_set, stru if (uid != -1 || gid != -1) { + struct stat st; + + if (do_stat (node, fd, NULL, &st) < 0) + { + fuse_reply_err (req, errno); + return; + } + + if (uid == -1) + { + uid = st.st_uid; + } + + if (gid == -1) + { + gid = st.st_gid; + } + if (fd >= 0) ret = do_fchown (lo, fd, uid, gid, node->ino->mode); else diff --git a/tests/unpriv.sh b/tests/unpriv.sh index 7bb19ed..3ce252e 100755 --- a/tests/unpriv.sh +++ b/tests/unpriv.sh @@ -29,3 +29,21 @@ else fi fusermount -u merged || [ $? -eq "${EXPECT_UMOUNT_STATUS:-0}" ] + +# xattr_permissions=2 +rm -rf lower upper workdir merged +mkdir lower upper workdir merged + +touch upper/file + +fuse-overlayfs -o lowerdir=lower,upperdir=upper,workdir=workdir,xattr_permissions=2 merged + +# Ensure UID is preserved with chgrp. +podman unshare chgrp 1 merged/file +test $(podman unshare stat -c %u:%g merged/file) = 0:1 + +# Ensure UID and GID are preserved with chmod. +chmod 600 merged/file +test $(podman unshare stat -c %u:%g merged/file) = 0:1 + +fusermount -u merged || [ $? -eq "${EXPECT_UMOUNT_STATUS:-0}" ] From e16818c042af0d1c58c11192fa5e7507ba19afc2 Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Mon, 10 Jun 2024 17:18:53 +0900 Subject: [PATCH 6/9] Fix printed extended attribute name Signed-off-by: Akihiko Odaki --- contrib/fix-mode.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/fix-mode.py b/contrib/fix-mode.py index b088218..533002a 100755 --- a/contrib/fix-mode.py +++ b/contrib/fix-mode.py @@ -23,7 +23,7 @@ def fix_path(path): os.setxattr(path, xattr_name, str.encode(content), flags=os.XATTR_CREATE, follow_symlinks=False) except Exception as e: if e.errno == errno.EEXIST: - print("attr %s already present for %s: %s" % (XATTR_OVERRIDE_STAT, path, e.errno)) + print("attr %s already present for %s: %s" % (xattr_name, path, e.errno)) return raise e From 90bea22c73d5d881b18aa90de63e7b0bb71b4949 Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Mon, 10 Jun 2024 17:19:19 +0900 Subject: [PATCH 7/9] Prefer user.containers.override_stat over user.fuseoverlayfs. Previously, fuse-overlayfs always used user.fuseoverlayfs.override_stat for the upper layer while honoring user.containers.override_stat for lower layers so that it can consume a layer created by containers/storage. It turned out that containers/storage also needs to get the overriding extended attribute set by fuse-overlayfs and to set one for the upper layer to make the root directory of the upper layer inherit the mode of a lower layer. Adding code to get and to set user.fuseoverlayfs.override_stat to containers/storage is a bit ugly. The underlying problem is that fuse-overlayfs changes what name to use ad hoc. Fix it by always preferring user.containers.override_stat, which containers/storage honors, over user.fuseoverlayfs.overlayfs, which is specific to fuse-overlayfs. Signed-off-by: Akihiko Odaki --- contrib/fix-mode.py | 4 ++-- direct.c | 4 ++-- main.c | 43 ++++++++++++++++++++++++++++++------------- 3 files changed, 34 insertions(+), 17 deletions(-) diff --git a/contrib/fix-mode.py b/contrib/fix-mode.py index 533002a..ed0e024 100755 --- a/contrib/fix-mode.py +++ b/contrib/fix-mode.py @@ -6,12 +6,12 @@ import errno XATTR_OVERRIDE_STAT_PRIVILEGED = "security.fuseoverlayfs.override_stat" -XATTR_OVERRIDE_STAT = "user.fuseoverlayfs.override_stat" +XATTR_OVERRIDE_CONTAINERS_STAT = "user.fuseoverlayfs.override_stat" if os.geteuid() == 0: xattr_name = XATTR_OVERRIDE_STAT_PRIVILEGED else: - xattr_name = XATTR_OVERRIDE_STAT + xattr_name = XATTR_OVERRIDE_CONTAINERS_STAT cwd_fd = os.open(".", os.O_PATH) diff --git a/direct.c b/direct.c index a1a5d7d..272631f 100644 --- a/direct.c +++ b/direct.c @@ -186,10 +186,10 @@ direct_load_data_source (struct ovl_layer *l, const char *opaque, const char *pa if (fgetxattr (l->fd, XATTR_PRIVILEGED_OVERRIDE_STAT, tmp, sizeof (tmp)) >= 0) l->stat_override_mode = STAT_OVERRIDE_PRIVILEGED; - else if (fgetxattr (l->fd, XATTR_OVERRIDE_STAT, tmp, sizeof (tmp)) >= 0) - l->stat_override_mode = STAT_OVERRIDE_USER; else if (fgetxattr (l->fd, XATTR_OVERRIDE_CONTAINERS_STAT, tmp, sizeof (tmp)) >= 0) l->stat_override_mode = STAT_OVERRIDE_CONTAINERS; + else if (fgetxattr (l->fd, XATTR_OVERRIDE_STAT, tmp, sizeof (tmp)) >= 0) + l->stat_override_mode = STAT_OVERRIDE_USER; return 0; } diff --git a/main.c b/main.c index 5bd1993..85abb01 100644 --- a/main.c +++ b/main.c @@ -539,17 +539,21 @@ write_permission_xattr (struct ovl_data *lo, int fd, const char *path, uid_t uid int ret; const char *name = NULL; - switch (lo->xattr_permissions) + switch (get_upper_layer (lo)->stat_override_mode) { - case 0: + case STAT_OVERRIDE_NONE: return 0; - case 1: + case STAT_OVERRIDE_USER: + name = XATTR_OVERRIDE_STAT; + break; + + case STAT_OVERRIDE_PRIVILEGED: name = XATTR_PRIVILEGED_OVERRIDE_STAT; break; - case 2: - name = XATTR_OVERRIDE_STAT; + case STAT_OVERRIDE_CONTAINERS: + name = XATTR_OVERRIDE_CONTAINERS_STAT; break; default: @@ -5792,13 +5796,22 @@ main (int argc, char *argv[]) } else if (lo.xattr_permissions == 2) { - get_upper_layer (&lo)->stat_override_mode = STAT_OVERRIDE_USER; - name = XATTR_OVERRIDE_STAT; + get_upper_layer (&lo)->stat_override_mode = STAT_OVERRIDE_CONTAINERS; + name = XATTR_OVERRIDE_CONTAINERS_STAT; } else error (EXIT_FAILURE, 0, "invalid value for xattr_permissions"); s = fgetxattr (get_upper_layer (&lo)->fd, name, data, sizeof (data)); + if (s < 0 && errno == ENODATA && lo.xattr_permissions == 2) + { + s = fgetxattr (get_upper_layer (&lo)->fd, XATTR_OVERRIDE_STAT, data, sizeof (data)); + if (s >= 0) + { + get_upper_layer (&lo)->stat_override_mode = STAT_OVERRIDE_USER; + name = XATTR_OVERRIDE_STAT; + } + } if (s < 0) { bool found = false; @@ -5809,15 +5822,19 @@ main (int argc, char *argv[]) for (l = get_lower_layers (&lo); l; l = l->next) { - s = fgetxattr (l->fd, name, data, sizeof (data)); - if (s < 0 && errno != ENODATA) - error (EXIT_FAILURE, errno, "fgetxattr mode from lower layer"); - if (s < 0 && lo.xattr_permissions == 2) + switch (lo.xattr_permissions) { + case 1: + s = fgetxattr (l->fd, name, data, sizeof (data)); + break; + + case 2: s = fgetxattr (l->fd, XATTR_OVERRIDE_CONTAINERS_STAT, data, sizeof (data)); - if (s < 0 && errno != ENODATA) - error (EXIT_FAILURE, errno, "fgetxattr mode from lower layer"); + if (s < 0 && errno == ENODATA) + s = fgetxattr (l->fd, XATTR_OVERRIDE_STAT, data, sizeof (data)); + break; } + if (s > 0) { ret = fsetxattr (get_upper_layer (&lo)->fd, name, data, s, 0); From 9810b85aad8c34c8913011041b0f98fd6242569c Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Thu, 13 Jun 2024 15:07:30 +0900 Subject: [PATCH 8/9] main: Override inaccessible xattrs Signed-off-by: Akihiko Odaki --- main.c | 103 ++++++++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 80 insertions(+), 23 deletions(-) diff --git a/main.c b/main.c index 85abb01..135b962 100644 --- a/main.c +++ b/main.c @@ -141,6 +141,7 @@ open_by_handle_at (int mount_fd, struct file_handle *handle, int flags) #define ORIGIN_XATTR "user.fuseoverlayfs.origin" #define OPAQUE_XATTR "user.fuseoverlayfs.opaque" #define XATTR_CONTAINERS_PREFIX "user.containers." +#define XATTR_CONTAINERS_OVERRIDE_PREFIX "user.containers.override_" #define UNPRIVILEGED_XATTR_PREFIX "user.overlay." #define UNPRIVILEGED_OPAQUE_XATTR "user.overlay.opaque" #define PRIVILEGED_XATTR_PREFIX "trusted.overlay." @@ -531,6 +532,39 @@ can_access_xattr (const char *name) && ! has_prefix (name, UNPRIVILEGED_XATTR_PREFIX); } +static bool encoded_xattr_name (const char *name) +{ + return has_prefix (name, XATTR_CONTAINERS_OVERRIDE_PREFIX) && + ! can_access_xattr (name + sizeof(XATTR_CONTAINERS_OVERRIDE_PREFIX) - 1); +} + +static const char *decode_xattr_name (const char *name) +{ + if (encoded_xattr_name (name)) + return name + sizeof(XATTR_CONTAINERS_OVERRIDE_PREFIX) - 1; + + if (can_access_xattr (name)) + return name; + + return NULL; +} + +static const char *encode_xattr_name (const struct ovl_layer *l, char *buf, + const char *name) +{ + if (can_access_xattr (name)) + return name; + + if (l->stat_override_mode != STAT_OVERRIDE_CONTAINERS || + strlen(name) > XATTR_NAME_MAX + 1 - sizeof(XATTR_CONTAINERS_OVERRIDE_PREFIX)) + return NULL; + + strcpy(buf, XATTR_CONTAINERS_OVERRIDE_PREFIX); + strcpy(buf + sizeof(XATTR_CONTAINERS_OVERRIDE_PREFIX) - 1, name); + + return buf; +} + static ssize_t write_permission_xattr (struct ovl_data *lo, int fd, const char *path, uid_t uid, gid_t gid, mode_t mode) { @@ -2606,7 +2640,10 @@ filter_xattrs_list (char *buf, ssize_t len) } else { - char *next = it + it_len; + char *next = it; + + next += encoded_xattr_name (it) ? + sizeof(XATTR_CONTAINERS_OVERRIDE_PREFIX) - 1 : it_len; memmove (it, next, buf + len - next); len -= it_len; @@ -2681,7 +2718,8 @@ ovl_getxattr (fuse_req_t req, fuse_ino_t ino, const char *name, size_t size) ssize_t len; struct ovl_node *node; struct ovl_data *lo = ovl_data (req); - cleanup_free char *buf = NULL; + cleanup_free char *value_buf = NULL; + char name_buf[XATTR_NAME_MAX + 1]; int ret; if (UNLIKELY (ovl_debug (req))) @@ -2693,23 +2731,24 @@ ovl_getxattr (fuse_req_t req, fuse_ino_t ino, const char *name, size_t size) return; } - if (! can_access_xattr (name)) + node = do_lookup_file (lo, ino, NULL); + if (node == NULL || node->whiteout) { - fuse_reply_err (req, ENODATA); + fuse_reply_err (req, ENOENT); return; } - node = do_lookup_file (lo, ino, NULL); - if (node == NULL || node->whiteout) + name = encode_xattr_name (node->layer, name_buf, name); + if (!name) { - fuse_reply_err (req, ENOENT); + fuse_reply_err (req, ENODATA); return; } if (size > 0) { - buf = malloc (size); - if (buf == NULL) + value_buf = malloc (size); + if (value_buf == NULL) { fuse_reply_err (req, errno); return; @@ -2717,12 +2756,12 @@ ovl_getxattr (fuse_req_t req, fuse_ino_t ino, const char *name, size_t size) } if (! node->hidden) - ret = node->layer->ds->getxattr (node->layer, node->path, name, buf, size); + ret = node->layer->ds->getxattr (node->layer, node->path, name, value_buf, size); else { char path[PATH_MAX]; strconcat3 (path, PATH_MAX, lo->workdir, "/", node->path); - ret = getxattr (path, name, buf, size); + ret = getxattr (path, name, value_buf, size); } if (ret < 0) @@ -2736,7 +2775,7 @@ ovl_getxattr (fuse_req_t req, fuse_ino_t ino, const char *name, size_t size) if (size == 0) fuse_reply_xattr (req, len); else - fuse_reply_buf (req, buf, len); + fuse_reply_buf (req, value_buf, len); } static void @@ -2757,7 +2796,8 @@ ovl_access (fuse_req_t req, fuse_ino_t ino, int mask) } static int -copy_xattr (int sfd, int dfd, char *buf, size_t buf_size) +copy_xattr (int sfd, + const struct ovl_layer *dl, int dfd, char *buf, size_t buf_size) { ssize_t xattr_len; @@ -2768,9 +2808,16 @@ copy_xattr (int sfd, int dfd, char *buf, size_t buf_size) for (it = buf; it - buf < xattr_len; it += strlen (it) + 1) { cleanup_free char *v = NULL; + const char *decoded_name = decode_xattr_name (it); + const char *encoded_name; + char buf[XATTR_NAME_MAX + 1]; ssize_t s; - if (! can_access_xattr (it)) + if (! decoded_name) + continue; + + encoded_name = encode_xattr_name (dl, buf, decoded_name); + if (! encoded_name) continue; s = safe_read_xattr (&v, sfd, it, 256); @@ -2781,7 +2828,7 @@ copy_xattr (int sfd, int dfd, char *buf, size_t buf_size) return -1; } - if (fsetxattr (dfd, it, v, s, 0) < 0) + if (fsetxattr (dfd, encoded_name, v, s, 0) < 0) { if (errno == EINVAL || errno == EOPNOTSUPP) continue; @@ -2921,7 +2968,7 @@ create_directory (struct ovl_data *lo, int dirfd, const char *name, const struct goto out; } - ret = copy_xattr (xattr_sfd, dfd, buf, buf_size); + ret = copy_xattr (xattr_sfd, get_upper_layer (lo), dfd, buf, buf_size); if (ret < 0) goto out; } @@ -3193,7 +3240,7 @@ copyup (struct ovl_data *lo, struct ovl_node *node) if (ret < 0) goto exit; - ret = copy_xattr (sfd, dfd, buf, buf_size); + ret = copy_xattr (sfd, get_upper_layer (lo), dfd, buf, buf_size); if (ret < 0) goto exit; @@ -3473,6 +3520,7 @@ ovl_setxattr (fuse_req_t req, fuse_ino_t ino, const char *name, cleanup_lock int l = enter_big_lock (); struct ovl_data *lo = ovl_data (req); struct ovl_node *node; + char name_buf[XATTR_NAME_MAX + 1]; int ret; if (UNLIKELY (ovl_debug (req))) @@ -3485,12 +3533,6 @@ ovl_setxattr (fuse_req_t req, fuse_ino_t ino, const char *name, return; } - if (has_prefix (name, PRIVILEGED_XATTR_PREFIX) || has_prefix (name, XATTR_PREFIX) || has_prefix (name, XATTR_CONTAINERS_PREFIX)) - { - fuse_reply_err (req, EPERM); - return; - } - node = do_lookup_file (lo, ino, NULL); if (node == NULL || node->whiteout) { @@ -3505,6 +3547,13 @@ ovl_setxattr (fuse_req_t req, fuse_ino_t ino, const char *name, return; } + name = encode_xattr_name (node->layer, name_buf, name); + if (!name) + { + fuse_reply_err (req, EPERM); + return; + } + if (! node->hidden) ret = direct_setxattr (node->layer, node->path, name, value, size, flags); else @@ -3546,6 +3595,7 @@ ovl_removexattr (fuse_req_t req, fuse_ino_t ino, const char *name) cleanup_lock int l = enter_big_lock (); struct ovl_node *node; struct ovl_data *lo = ovl_data (req); + char name_buf[XATTR_NAME_MAX + 1]; int ret; if (UNLIKELY (ovl_debug (req))) @@ -3565,6 +3615,13 @@ ovl_removexattr (fuse_req_t req, fuse_ino_t ino, const char *name) return; } + name = encode_xattr_name (node->layer, name_buf, name); + if (!name) + { + fuse_reply_err (req, EPERM); + return; + } + if (! node->hidden) ret = direct_removexattr (node->layer, node->path, name); else From 20161f96d7a73e180814365bf21a3550dc5f3bd8 Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Thu, 13 Jun 2024 16:44:44 +0900 Subject: [PATCH 9/9] main: Isolate security xattrs for STAT_OVERRIDE_CONTAINERS The major use case of stat override is to enable rootless containers on network filesystems, and they also lack security xattr support in non-root user namespaces. Trying to set security xattrs on them result in ENOTSUP and break things. It makes little sense to share security xattrs with the underlying filesystems when overriding stat in the first place. Linux's NFS server exposes security xattrs only when the user explicitly claims the security consistencies between the server and clients, and hide them otherwise. Following this precedent, we should isolate security xattrs since we know the security policy enforced by fuse-overlayfs is already distinct from the underlying filesystem when overriding owners and file mode. Mark security xattrs inaccessible with STAT_OVERRIDE_CONTAINERS to prefix all access to them with XATTR_CONTAINERS_OVERRIDE_PREFIX. Signed-off-by: Akihiko Odaki --- main.c | 46 +++++++++++++++++++++++++--------------------- tests/unpriv.sh | 6 ++++++ 2 files changed, 31 insertions(+), 21 deletions(-) diff --git a/main.c b/main.c index 135b962..b5753db 100644 --- a/main.c +++ b/main.c @@ -59,6 +59,7 @@ #include #include #include +#include #include #include #include @@ -525,25 +526,27 @@ has_prefix (const char *str, const char *pref) } static bool -can_access_xattr (const char *name) +can_access_xattr (const struct ovl_layer *l, const char *name) { - return ! has_prefix (name, XATTR_PREFIX) - && ! has_prefix (name, PRIVILEGED_XATTR_PREFIX) - && ! has_prefix (name, UNPRIVILEGED_XATTR_PREFIX); + return ! (has_prefix (name, XATTR_PREFIX) + || has_prefix (name, PRIVILEGED_XATTR_PREFIX) + || has_prefix (name, UNPRIVILEGED_XATTR_PREFIX) + || (l->stat_override_mode == STAT_OVERRIDE_CONTAINERS && + has_prefix (name, XATTR_SECURITY_PREFIX))); } -static bool encoded_xattr_name (const char *name) +static bool encoded_xattr_name (const struct ovl_layer *l, const char *name) { return has_prefix (name, XATTR_CONTAINERS_OVERRIDE_PREFIX) && - ! can_access_xattr (name + sizeof(XATTR_CONTAINERS_OVERRIDE_PREFIX) - 1); + ! can_access_xattr (l, name + sizeof(XATTR_CONTAINERS_OVERRIDE_PREFIX) - 1); } -static const char *decode_xattr_name (const char *name) +static const char *decode_xattr_name (const struct ovl_layer *l, const char *name) { - if (encoded_xattr_name (name)) + if (encoded_xattr_name (l, name)) return name + sizeof(XATTR_CONTAINERS_OVERRIDE_PREFIX) - 1; - if (can_access_xattr (name)) + if (can_access_xattr (l, name)) return name; return NULL; @@ -552,7 +555,7 @@ static const char *decode_xattr_name (const char *name) static const char *encode_xattr_name (const struct ovl_layer *l, char *buf, const char *name) { - if (can_access_xattr (name)) + if (can_access_xattr (l, name)) return name; if (l->stat_override_mode != STAT_OVERRIDE_CONTAINERS || @@ -2617,7 +2620,7 @@ inherit_acl (struct ovl_data *lo, struct ovl_node *parent, int targetfd, const c /* in-place filter xattrs that cannot be accessed. */ static ssize_t -filter_xattrs_list (char *buf, ssize_t len) +filter_xattrs_list (struct ovl_layer *l, char *buf, ssize_t len) { ssize_t ret = 0; char *it; @@ -2633,7 +2636,7 @@ filter_xattrs_list (char *buf, ssize_t len) it_len = strlen (it) + 1; - if (can_access_xattr (it)) + if (can_access_xattr (l, it)) { it += it_len; ret += it_len; @@ -2642,7 +2645,7 @@ filter_xattrs_list (char *buf, ssize_t len) { char *next = it; - next += encoded_xattr_name (it) ? + next += encoded_xattr_name (l, it) ? sizeof(XATTR_CONTAINERS_OVERRIDE_PREFIX) - 1 : it_len; memmove (it, next, buf + len - next); @@ -2703,7 +2706,7 @@ ovl_listxattr (fuse_req_t req, fuse_ino_t ino, size_t size) return; } - len = filter_xattrs_list (buf, ret); + len = filter_xattrs_list (node->layer, buf, ret); if (size == 0) fuse_reply_xattr (req, len); @@ -2796,7 +2799,7 @@ ovl_access (fuse_req_t req, fuse_ino_t ino, int mask) } static int -copy_xattr (int sfd, +copy_xattr (const struct ovl_layer *sl, int sfd, const struct ovl_layer *dl, int dfd, char *buf, size_t buf_size) { ssize_t xattr_len; @@ -2808,7 +2811,7 @@ copy_xattr (int sfd, for (it = buf; it - buf < xattr_len; it += strlen (it) + 1) { cleanup_free char *v = NULL; - const char *decoded_name = decode_xattr_name (it); + const char *decoded_name = decode_xattr_name (sl, it); const char *encoded_name; char buf[XATTR_NAME_MAX + 1]; ssize_t s; @@ -2904,7 +2907,8 @@ static int create_node_directory (struct ovl_data *lo, struct ovl_node *src); static int create_directory (struct ovl_data *lo, int dirfd, const char *name, const struct timespec *times, - struct ovl_node *parent, int xattr_sfd, uid_t uid, gid_t gid, mode_t mode, bool set_opaque, struct stat *st_out) + struct ovl_node *parent, struct ovl_layer *sl, int xattr_sfd, + uid_t uid, gid_t gid, mode_t mode, bool set_opaque, struct stat *st_out) { int ret; int saved_errno; @@ -2968,7 +2972,7 @@ create_directory (struct ovl_data *lo, int dirfd, const char *name, const struct goto out; } - ret = copy_xattr (xattr_sfd, get_upper_layer (lo), dfd, buf, buf_size); + ret = copy_xattr (sl, xattr_sfd, get_upper_layer (lo), dfd, buf, buf_size); if (ret < 0) goto out; } @@ -3061,7 +3065,7 @@ create_node_directory (struct ovl_data *lo, struct ovl_node *src) if (override_mode (src->layer, sfd, NULL, NULL, &st) < 0 && errno != ENODATA && errno != EOPNOTSUPP) return -1; - ret = create_directory (lo, get_upper_layer (lo)->fd, src->path, times, src->parent, sfd, st.st_uid, st.st_gid, st.st_mode, false, NULL); + ret = create_directory (lo, get_upper_layer (lo)->fd, src->path, times, src->parent, src->layer, sfd, st.st_uid, st.st_gid, st.st_mode, false, NULL); if (ret == 0) { src->layer = get_upper_layer (lo); @@ -3240,7 +3244,7 @@ copyup (struct ovl_data *lo, struct ovl_node *node) if (ret < 0) goto exit; - ret = copy_xattr (sfd, get_upper_layer (lo), dfd, buf, buf_size); + ret = copy_xattr (node->layer, sfd, get_upper_layer (lo), dfd, buf, buf_size); if (ret < 0) goto exit; @@ -5165,7 +5169,7 @@ ovl_mkdir (fuse_req_t req, fuse_ino_t parent, const char *name, mode_t mode) return; } - ret = create_directory (lo, get_upper_layer (lo)->fd, path, NULL, pnode, -1, + ret = create_directory (lo, get_upper_layer (lo)->fd, path, NULL, pnode, NULL, -1, get_uid (lo, ctx->uid), get_gid (lo, ctx->gid), mode & ~ctx->umask, true, &st); if (ret < 0) diff --git a/tests/unpriv.sh b/tests/unpriv.sh index 3ce252e..f255137 100755 --- a/tests/unpriv.sh +++ b/tests/unpriv.sh @@ -35,9 +35,15 @@ rm -rf lower upper workdir merged mkdir lower upper workdir merged touch upper/file +unshare -r setcap cap_net_admin+ep upper/file fuse-overlayfs -o lowerdir=lower,upperdir=upper,workdir=workdir,xattr_permissions=2 merged +# Ensure the security xattr namespace is isolated. +test "$(unshare -r getcap merged/file)" = '' +unshare -r setcap cap_net_admin+ep merged/file +test "$(unshare -r getcap merged/file)" = 'merged/file cap_net_admin=ep' + # Ensure UID is preserved with chgrp. podman unshare chgrp 1 merged/file test $(podman unshare stat -c %u:%g merged/file) = 0:1