From ef202b848bb6635dec17d3ec0041b04cd2301bed Mon Sep 17 00:00:00 2001 From: Filipe Brandenburger Date: Mon, 4 Jun 2018 14:23:14 -0700 Subject: copy: only check for traversing mount points on directories This fixes the copy routines on overlay filesystem, which typically returns the underlying st_dev for files, symlinks, etc. The value of st_dev is guaranteed to be the same for directories, so checking it on directories only fixes this code on overlay filesystem and still keeps it from traversing mount points (which was the original intent.) There's a small side effect here, by which regular (non-directory) files with bind mounts will be copied by the new logic (while they were skipped by the previous logic.) Tested: ./build/test-copy with an overlay on /tmp. Fixes: #9134 --- src/basic/copy.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) (limited to 'src/basic') diff --git a/src/basic/copy.c b/src/basic/copy.c index 650de612b8..6ed46241fc 100644 --- a/src/basic/copy.c +++ b/src/basic/copy.c @@ -530,13 +530,12 @@ static int fd_copy_directory( continue; } - if (buf.st_dev != original_device) - continue; - - if (S_ISREG(buf.st_mode)) - q = fd_copy_regular(dirfd(d), de->d_name, &buf, fdt, de->d_name, override_uid, override_gid, copy_flags); - else if (S_ISDIR(buf.st_mode)) + if (S_ISDIR(buf.st_mode)) { + if (buf.st_dev != original_device) + continue; q = fd_copy_directory(dirfd(d), de->d_name, &buf, fdt, de->d_name, original_device, override_uid, override_gid, copy_flags); + } else if (S_ISREG(buf.st_mode)) + q = fd_copy_regular(dirfd(d), de->d_name, &buf, fdt, de->d_name, override_uid, override_gid, copy_flags); else if (S_ISLNK(buf.st_mode)) q = fd_copy_symlink(dirfd(d), de->d_name, &buf, fdt, de->d_name, override_uid, override_gid, copy_flags); else if (S_ISFIFO(buf.st_mode)) -- cgit v1.2.3 From f6a77804c9d743e8d01051d2cb0511b53a49c56e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 6 Jun 2018 17:33:28 +0200 Subject: copy: extend check for mount point crossing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We do this checks as protection against bind mount cycles on the same file system. However, the check wasn't really effective for that, as it would only detect cycles A → B → A this way. By using fs_is_mount_point() we'll also detect cycles A → A. Also, while we are at it, make these file system boundary checks optional. This is not used anywhere, but might be eventually... Most importantly though add a longer blurb explanation the why. --- src/basic/copy.c | 31 +++++++++++++++++++++++++++++-- src/basic/copy.h | 7 ++++--- 2 files changed, 33 insertions(+), 5 deletions(-) (limited to 'src/basic') diff --git a/src/basic/copy.c b/src/basic/copy.c index 6ed46241fc..8d8f907e29 100644 --- a/src/basic/copy.c +++ b/src/basic/copy.c @@ -29,6 +29,7 @@ #include "io-util.h" #include "macro.h" #include "missing.h" +#include "mount-util.h" #include "string-util.h" #include "strv.h" #include "time-util.h" @@ -531,8 +532,34 @@ static int fd_copy_directory( } if (S_ISDIR(buf.st_mode)) { - if (buf.st_dev != original_device) - continue; + /* + * Don't descend into directories on other file systems, if this is requested. We do a simple + * .st_dev check here, which basically comes for free. Note that we do this check only on + * directories, not other kind of file system objects, for two reason: + * + * • The kernel's overlayfs pseudo file system that overlays multiple real file systems + * propagates the .st_dev field of the file system a file originates from all the way up + * through the stack to stat(). It doesn't do that for directories however. This means that + * comparing .st_dev on non-directories suggests that they all are mount points. To avoid + * confusion we hence avoid relying on this check for regular files. + * + * • The main reason we do this check at all is to protect ourselves from bind mount cycles, + * where we really want to avoid descending down in all eternity. However the .st_dev check + * is usually not sufficient for this protection anyway, as bind mount cycles from the same + * file system onto itself can't be detected that way. + */ + + if (FLAGS_SET(copy_flags, COPY_SAME_MOUNT)) { + if (buf.st_dev != original_device) + continue; + + r = fd_is_mount_point(dirfd(d), de->d_name, 0); + if (r < 0) + return r; + if (r > 0) + continue; + } + q = fd_copy_directory(dirfd(d), de->d_name, &buf, fdt, de->d_name, original_device, override_uid, override_gid, copy_flags); } else if (S_ISREG(buf.st_mode)) q = fd_copy_regular(dirfd(d), de->d_name, &buf, fdt, de->d_name, override_uid, override_gid, copy_flags); diff --git a/src/basic/copy.h b/src/basic/copy.h index 0c50bcef50..fa84ba494e 100644 --- a/src/basic/copy.h +++ b/src/basic/copy.h @@ -13,9 +13,10 @@ #include typedef enum CopyFlags { - COPY_REFLINK = 1U << 0, /* Try to reflink */ - COPY_MERGE = 1U << 1, /* Merge existing trees with our new one to copy */ - COPY_REPLACE = 1U << 2, /* Replace an existing file if there's one */ + COPY_REFLINK = 1U << 0, /* Try to reflink */ + COPY_MERGE = 1U << 1, /* Merge existing trees with our new one to copy */ + COPY_REPLACE = 1U << 2, /* Replace an existing file if there's one */ + COPY_SAME_MOUNT = 1U << 3, /* Don't descend recursively into other file systems, across mount point boundaries */ } CopyFlags; int copy_file_fd(const char *from, int to, CopyFlags copy_flags); -- cgit v1.2.3 From 575a07d2dc1d1f4bc36ec0358819da92e2aef32f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 7 Jun 2018 13:24:03 +0200 Subject: copy: put a depth limit on copying file system trees recursively This is a safety net against bind mount cycles, as such pick it relatively high at 2048 for now. As suggested by @filbranden on #9213 --- src/basic/copy.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) (limited to 'src/basic') diff --git a/src/basic/copy.c b/src/basic/copy.c index 8d8f907e29..1921047714 100644 --- a/src/basic/copy.c +++ b/src/basic/copy.c @@ -37,7 +37,12 @@ #include "user-util.h" #include "xattr-util.h" -#define COPY_BUFFER_SIZE (16*1024u) +#define COPY_BUFFER_SIZE (16U*1024U) + +/* A safety net for descending recursively into file system trees to copy. On Linux PATH_MAX is 4096, which means the + * deepest valid path one can build is around 2048, which we hence use as a safety net here, to not spin endlessly in + * case of bind mount cycles and suchlike. */ +#define COPY_DEPTH_MAX 2048U static ssize_t try_copy_file_range( int fd_in, loff_t *off_in, @@ -480,6 +485,7 @@ static int fd_copy_directory( int dt, const char *to, dev_t original_device, + unsigned depth_left, uid_t override_uid, gid_t override_gid, CopyFlags copy_flags) { @@ -493,6 +499,9 @@ static int fd_copy_directory( assert(st); assert(to); + if (depth_left == 0) + return -ENAMETOOLONG; + if (from) fdf = openat(df, from, O_RDONLY|O_DIRECTORY|O_CLOEXEC|O_NOCTTY|O_NOFOLLOW); else @@ -546,7 +555,9 @@ static int fd_copy_directory( * • The main reason we do this check at all is to protect ourselves from bind mount cycles, * where we really want to avoid descending down in all eternity. However the .st_dev check * is usually not sufficient for this protection anyway, as bind mount cycles from the same - * file system onto itself can't be detected that way. + * file system onto itself can't be detected that way. (Note we also do a recursion depth + * check, which is probably the better protection in this regard, which is why + * COPY_SAME_MOUNT is optional). */ if (FLAGS_SET(copy_flags, COPY_SAME_MOUNT)) { @@ -560,7 +571,7 @@ static int fd_copy_directory( continue; } - q = fd_copy_directory(dirfd(d), de->d_name, &buf, fdt, de->d_name, original_device, override_uid, override_gid, copy_flags); + q = fd_copy_directory(dirfd(d), de->d_name, &buf, fdt, de->d_name, original_device, depth_left-1, override_uid, override_gid, copy_flags); } else if (S_ISREG(buf.st_mode)) q = fd_copy_regular(dirfd(d), de->d_name, &buf, fdt, de->d_name, override_uid, override_gid, copy_flags); else if (S_ISLNK(buf.st_mode)) @@ -612,7 +623,7 @@ int copy_tree_at(int fdf, const char *from, int fdt, const char *to, uid_t overr if (S_ISREG(st.st_mode)) return fd_copy_regular(fdf, from, &st, fdt, to, override_uid, override_gid, copy_flags); else if (S_ISDIR(st.st_mode)) - return fd_copy_directory(fdf, from, &st, fdt, to, st.st_dev, override_uid, override_gid, copy_flags); + return fd_copy_directory(fdf, from, &st, fdt, to, st.st_dev, COPY_DEPTH_MAX, override_uid, override_gid, copy_flags); else if (S_ISLNK(st.st_mode)) return fd_copy_symlink(fdf, from, &st, fdt, to, override_uid, override_gid, copy_flags); else if (S_ISFIFO(st.st_mode)) @@ -639,7 +650,7 @@ int copy_directory_fd(int dirfd, const char *to, CopyFlags copy_flags) { if (!S_ISDIR(st.st_mode)) return -ENOTDIR; - return fd_copy_directory(dirfd, NULL, &st, AT_FDCWD, to, st.st_dev, UID_INVALID, GID_INVALID, copy_flags); + return fd_copy_directory(dirfd, NULL, &st, AT_FDCWD, to, st.st_dev, COPY_DEPTH_MAX, UID_INVALID, GID_INVALID, copy_flags); } int copy_directory(const char *from, const char *to, CopyFlags copy_flags) { @@ -654,7 +665,7 @@ int copy_directory(const char *from, const char *to, CopyFlags copy_flags) { if (!S_ISDIR(st.st_mode)) return -ENOTDIR; - return fd_copy_directory(AT_FDCWD, from, &st, AT_FDCWD, to, st.st_dev, UID_INVALID, GID_INVALID, copy_flags); + return fd_copy_directory(AT_FDCWD, from, &st, AT_FDCWD, to, st.st_dev, COPY_DEPTH_MAX, UID_INVALID, GID_INVALID, copy_flags); } int copy_file_fd(const char *from, int fdt, CopyFlags copy_flags) { -- cgit v1.2.3