| Message ID | 155059611887.17079.12991580316407924257.stgit@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | VFS: Provide new mount UAPI | expand |
On 19/02/2019 17:08, David Howells wrote: > Add a move_mount() system call that will move a mount from one place to > another and, in the next commit, allow to attach an unattached mount tree. > > The new system call looks like the following: > > int move_mount(int from_dfd, const char *from_path, > int to_dfd, const char *to_path, > unsigned int flags); > > Signed-off-by: David Howells <dhowells@redhat.com> > cc: linux-api@vger.kernel.org > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- > > arch/x86/entry/syscalls/syscall_32.tbl | 1 > arch/x86/entry/syscalls/syscall_64.tbl | 1 > fs/namespace.c | 126 ++++++++++++++++++++++++-------- > include/linux/lsm_hooks.h | 6 ++ > include/linux/security.h | 7 ++ > include/linux/syscalls.h | 3 + > include/uapi/linux/mount.h | 11 +++ > security/security.c | 5 + > 8 files changed, 129 insertions(+), 31 deletions(-) > diff --git a/fs/namespace.c b/fs/namespace.c > index 112d46f26fc3..f10122028a11 100644 > --- a/fs/namespace.c > +++ b/fs/namespace.c > @@ -2537,72 +2537,81 @@ static inline int tree_contains_unbindable(struct mount *mnt) > return 0; > } > > -static int do_move_mount(struct path *path, const char *old_name) > +static int do_move_mount(struct path *old_path, struct path *new_path) > { > - struct path old_path, parent_path; > + struct path parent_path = {.mnt = NULL, .dentry = NULL}; > struct mount *p; > struct mount *old; > struct mountpoint *mp; > int err; > - if (!old_name || !*old_name) > - return -EINVAL; > - err = kern_path(old_name, LOOKUP_FOLLOW, &old_path); > - if (err) > - return err; > > - mp = lock_mount(path); > - err = PTR_ERR(mp); > + mp = lock_mount(new_path); > if (IS_ERR(mp)) > - goto out; > + return PTR_ERR(mp); > > - old = real_mount(old_path.mnt); > - p = real_mount(path->mnt); > + old = real_mount(old_path->mnt); > + p = real_mount(new_path->mnt); > > err = -EINVAL; > if (!check_mnt(p) || !check_mnt(old)) > - goto out1; > + goto out; > > - if (old->mnt.mnt_flags & MNT_LOCKED) > - goto out1; > + if (!mnt_has_parent(old)) > + goto out; > > - err = -EINVAL; > - if (old_path.dentry != old_path.mnt->mnt_root) > - goto out1; > + if (old->mnt.mnt_flags & MNT_LOCKED) > + goto out; > > - if (!mnt_has_parent(old)) > - goto out1; > + if (old_path->dentry != old_path->mnt->mnt_root) > + goto out; > > - if (d_is_dir(path->dentry) != > - d_is_dir(old_path.dentry)) > - goto out1; > + if (d_is_dir(new_path->dentry) != > + d_is_dir(old_path->dentry)) > + goto out; > /* > * Don't move a mount residing in a shared parent. > */ > if (IS_MNT_SHARED(old->mnt_parent)) > - goto out1; > + goto out; > /* > * Don't move a mount tree containing unbindable mounts to a destination > * mount which is shared. > */ > if (IS_MNT_SHARED(p) && tree_contains_unbindable(old)) > - goto out1; > + goto out; > err = -ELOOP; > for (; mnt_has_parent(p); p = p->mnt_parent) > if (p == old) > - goto out1; > + goto out; > > - err = attach_recursive_mnt(old, real_mount(path->mnt), mp, &parent_path); > + err = attach_recursive_mnt(old, real_mount(new_path->mnt), mp, > + &parent_path); > if (err) > - goto out1; > + goto out; > > /* if the mount is moved, it should no longer be expire > * automatically */ > list_del_init(&old->mnt_expire); > -out1: > - unlock_mount(mp); > out: > + unlock_mount(mp); > if (!err) > path_put(&parent_path); > + return err; > +} > + > +static int do_move_mount_old(struct path *path, const char *old_name) > +{ > + struct path old_path; > + int err; > + > + if (!old_name || !*old_name) > + return -EINVAL; > + > + err = kern_path(old_name, LOOKUP_FOLLOW, &old_path); > + if (err) > + return err; > + > + err = do_move_mount(&old_path, path); > path_put(&old_path); > return err; > } > @@ -3050,7 +3059,7 @@ long do_mount(const char *dev_name, const char __user *dir_name, > else if (flags & (MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE)) > retval = do_change_type(&path, flags); > else if (flags & MS_MOVE) > - retval = do_move_mount(&path, dev_name); > + retval = do_move_mount_old(&path, dev_name); > else > retval = do_new_mount(&path, type_page, sb_flags, mnt_flags, > dev_name, data_page); > @@ -3278,6 +3287,61 @@ SYSCALL_DEFINE5(mount, char __user *, dev_name, char __user *, dir_name, > return ksys_mount(dev_name, dir_name, type, flags, data); > } > > +/* > + * Move a mount from one place to another. > + * > + * Note the flags value is a combination of MOVE_MOUNT_* flags. > + */ > +SYSCALL_DEFINE5(move_mount, > + int, from_dfd, const char *, from_pathname, > + int, to_dfd, const char *, to_pathname, > + unsigned int, flags) > +{ > + struct path from_path, to_path; > + unsigned int lflags; > + int ret = 0; > + > + if (!may_mount()) > + return -EPERM; > + > + if (flags & ~MOVE_MOUNT__MASK) > + return -EINVAL; > + > + /* If someone gives a pathname, they aren't permitted to move > + * from an fd that requires unmount as we can't get at the flag > + * to clear it afterwards. > + */ Comment is incorrect. * FMODE_NEED_UNMOUNT is never cleared. * Technically I don't see anything preventing them giving a pathname, but it needs to be "." or equivalent. Otherwise it will fail the "!attached" check in the next patch. * The only argument I remember for preventing this, was that it might confuse users (not the kernel). If you are allowed to move from a sub-mount, then in certain programming styles - like my shell script test cases - you might accidentally close the original file too early. Then you won't be able to do move_mount() from the tree, because the tree was unmounted ("dissolved") when you closed it. I think the description in the previous patch, for open_tree(), makes it clear though. "The detached tree will be dissolved on the final close of obtained file". If there is a good reason, I expect we can simply remove the "!attached" part of the check. If the constraint is generating more confusion than the added flexibility, I think that would be a good reason :-). > + lflags = 0; > + if (flags & MOVE_MOUNT_F_SYMLINKS) lflags |= LOOKUP_FOLLOW; > + if (flags & MOVE_MOUNT_F_AUTOMOUNTS) lflags |= LOOKUP_AUTOMOUNT; > + if (flags & MOVE_MOUNT_F_EMPTY_PATH) lflags |= LOOKUP_EMPTY; > + > + ret = user_path_at(from_dfd, from_pathname, lflags, &from_path); > + if (ret < 0) > + return ret; > + > + lflags = 0; > + if (flags & MOVE_MOUNT_T_SYMLINKS) lflags |= LOOKUP_FOLLOW; > + if (flags & MOVE_MOUNT_T_AUTOMOUNTS) lflags |= LOOKUP_AUTOMOUNT; > + if (flags & MOVE_MOUNT_T_EMPTY_PATH) lflags |= LOOKUP_EMPTY; > + > + ret = user_path_at(to_dfd, to_pathname, lflags, &to_path); > + if (ret < 0) > + goto out_from; > + > + ret = security_move_mount(&from_path, &to_path); > + if (ret < 0) > + goto out_to; > + > + ret = do_move_mount(&from_path, &to_path); > + > +out_to: > + path_put(&to_path); > +out_from: > + path_put(&from_path); > + return ret; > +} > + > /* > * Return true if path is reachable from root > * >
On 20/02/2019 12:32, Alan Jenkins wrote: > On 19/02/2019 17:08, David Howells wrote: >> Add a move_mount() system call that will move a mount from one place to >> another and, in the next commit, allow to attach an unattached mount >> tree. >> >> The new system call looks like the following: >> >> int move_mount(int from_dfd, const char *from_path, >> int to_dfd, const char *to_path, >> unsigned int flags); >> >> Signed-off-by: David Howells <dhowells@redhat.com> >> cc: linux-api@vger.kernel.org >> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> >> --- >> >> arch/x86/entry/syscalls/syscall_32.tbl | 1 >> arch/x86/entry/syscalls/syscall_64.tbl | 1 >> fs/namespace.c | 126 >> ++++++++++++++++++++++++-------- >> include/linux/lsm_hooks.h | 6 ++ >> include/linux/security.h | 7 ++ >> include/linux/syscalls.h | 3 + >> include/uapi/linux/mount.h | 11 +++ >> security/security.c | 5 + >> 8 files changed, 129 insertions(+), 31 deletions(-) > >> diff --git a/fs/namespace.c b/fs/namespace.c >> index 112d46f26fc3..f10122028a11 100644 >> --- a/fs/namespace.c >> +++ b/fs/namespace.c >> @@ -2537,72 +2537,81 @@ static inline int >> tree_contains_unbindable(struct mount *mnt) >> return 0; >> } >> -static int do_move_mount(struct path *path, const char *old_name) >> +static int do_move_mount(struct path *old_path, struct path *new_path) >> { >> - struct path old_path, parent_path; >> + struct path parent_path = {.mnt = NULL, .dentry = NULL}; >> struct mount *p; >> struct mount *old; >> struct mountpoint *mp; >> int err; >> - if (!old_name || !*old_name) >> - return -EINVAL; >> - err = kern_path(old_name, LOOKUP_FOLLOW, &old_path); >> - if (err) >> - return err; >> - mp = lock_mount(path); >> - err = PTR_ERR(mp); >> + mp = lock_mount(new_path); >> if (IS_ERR(mp)) >> - goto out; >> + return PTR_ERR(mp); >> - old = real_mount(old_path.mnt); >> - p = real_mount(path->mnt); >> + old = real_mount(old_path->mnt); >> + p = real_mount(new_path->mnt); >> err = -EINVAL; >> if (!check_mnt(p) || !check_mnt(old)) >> - goto out1; >> + goto out; >> - if (old->mnt.mnt_flags & MNT_LOCKED) >> - goto out1; >> + if (!mnt_has_parent(old)) >> + goto out; >> - err = -EINVAL; >> - if (old_path.dentry != old_path.mnt->mnt_root) >> - goto out1; >> + if (old->mnt.mnt_flags & MNT_LOCKED) >> + goto out; >> - if (!mnt_has_parent(old)) >> - goto out1; >> + if (old_path->dentry != old_path->mnt->mnt_root) >> + goto out; >> - if (d_is_dir(path->dentry) != >> - d_is_dir(old_path.dentry)) >> - goto out1; >> + if (d_is_dir(new_path->dentry) != >> + d_is_dir(old_path->dentry)) >> + goto out; >> /* >> * Don't move a mount residing in a shared parent. >> */ >> if (IS_MNT_SHARED(old->mnt_parent)) >> - goto out1; >> + goto out; >> /* >> * Don't move a mount tree containing unbindable mounts to a >> destination >> * mount which is shared. >> */ >> if (IS_MNT_SHARED(p) && tree_contains_unbindable(old)) >> - goto out1; >> + goto out; >> err = -ELOOP; >> for (; mnt_has_parent(p); p = p->mnt_parent) >> if (p == old) >> - goto out1; >> + goto out; >> - err = attach_recursive_mnt(old, real_mount(path->mnt), mp, >> &parent_path); >> + err = attach_recursive_mnt(old, real_mount(new_path->mnt), mp, >> + &parent_path); >> if (err) >> - goto out1; >> + goto out; >> /* if the mount is moved, it should no longer be expire >> * automatically */ >> list_del_init(&old->mnt_expire); >> -out1: >> - unlock_mount(mp); >> out: >> + unlock_mount(mp); >> if (!err) >> path_put(&parent_path); >> + return err; >> +} >> + >> +static int do_move_mount_old(struct path *path, const char *old_name) >> +{ >> + struct path old_path; >> + int err; >> + >> + if (!old_name || !*old_name) >> + return -EINVAL; >> + >> + err = kern_path(old_name, LOOKUP_FOLLOW, &old_path); >> + if (err) >> + return err; >> + >> + err = do_move_mount(&old_path, path); >> path_put(&old_path); >> return err; >> } >> @@ -3050,7 +3059,7 @@ long do_mount(const char *dev_name, const char >> __user *dir_name, >> else if (flags & (MS_SHARED | MS_PRIVATE | MS_SLAVE | >> MS_UNBINDABLE)) >> retval = do_change_type(&path, flags); >> else if (flags & MS_MOVE) >> - retval = do_move_mount(&path, dev_name); >> + retval = do_move_mount_old(&path, dev_name); >> else >> retval = do_new_mount(&path, type_page, sb_flags, mnt_flags, >> dev_name, data_page); >> @@ -3278,6 +3287,61 @@ SYSCALL_DEFINE5(mount, char __user *, >> dev_name, char __user *, dir_name, >> return ksys_mount(dev_name, dir_name, type, flags, data); >> } >> +/* >> + * Move a mount from one place to another. >> + * >> + * Note the flags value is a combination of MOVE_MOUNT_* flags. >> + */ >> +SYSCALL_DEFINE5(move_mount, >> + int, from_dfd, const char *, from_pathname, >> + int, to_dfd, const char *, to_pathname, >> + unsigned int, flags) >> +{ >> + struct path from_path, to_path; >> + unsigned int lflags; >> + int ret = 0; >> + >> + if (!may_mount()) >> + return -EPERM; >> + >> + if (flags & ~MOVE_MOUNT__MASK) >> + return -EINVAL; >> + >> + /* If someone gives a pathname, they aren't permitted to move >> + * from an fd that requires unmount as we can't get at the flag >> + * to clear it afterwards. >> + */ > > Comment is incorrect. > > * FMODE_NEED_UNMOUNT is never cleared. > > * Technically I don't see anything preventing them giving a pathname, > but it needs to be "." or equivalent. Otherwise it will fail the > "!attached" check in the next patch. > > * The only argument I remember for preventing this, was that it might > confuse users (not the kernel). If you are allowed to move from a > sub-mount, then in certain programming styles - like my shell script > test cases - you might accidentally close the original file too > early. Then you won't be able to do move_mount() from the tree, > because the tree was unmounted ("dissolved") when you closed it. > > I think the description in the previous patch, for open_tree(), makes > it clear though. "The detached tree will be dissolved on the final > close of obtained file". > > If there is a good reason, I expect we can simply remove the > "!attached" part of the check. If the constraint is generating more > confusion than the added flexibility, I think that would be a good > reason :-). Sorry, I see it. Although you are not clearing a flag, you have to free the old value of old->mnt_ns. And that is not being reference-counted, it has a single owner, the file which has FMODE_NEED_UNMOUNT. So it is not possible to simply remove the "!attached" check. I still find the comment confusing, i.e. describing this as clearing a flag. Alan
On Tue, Feb 19, 2019 at 6:08 PM David Howells <dhowells@redhat.com> wrote: > Add a move_mount() system call that will move a mount from one place to > another and, in the next commit, allow to attach an unattached mount tree. > > The new system call looks like the following: > > int move_mount(int from_dfd, const char *from_path, > int to_dfd, const char *to_path, > unsigned int flags); > > Signed-off-by: David Howells <dhowells@redhat.com> > cc: linux-api@vger.kernel.org > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- [...] > +/* > + * Move a mount from one place to another. > + * > + * Note the flags value is a combination of MOVE_MOUNT_* flags. > + */ > +SYSCALL_DEFINE5(move_mount, > + int, from_dfd, const char *, from_pathname, > + int, to_dfd, const char *, to_pathname, > + unsigned int, flags) > +{ > + struct path from_path, to_path; > + unsigned int lflags; > + int ret = 0; > + > + if (!may_mount()) > + return -EPERM; > + > + if (flags & ~MOVE_MOUNT__MASK) > + return -EINVAL; > + > + /* If someone gives a pathname, they aren't permitted to move > + * from an fd that requires unmount as we can't get at the flag > + * to clear it afterwards. > + */ > + lflags = 0; > + if (flags & MOVE_MOUNT_F_SYMLINKS) lflags |= LOOKUP_FOLLOW; > + if (flags & MOVE_MOUNT_F_AUTOMOUNTS) lflags |= LOOKUP_AUTOMOUNT; > + if (flags & MOVE_MOUNT_F_EMPTY_PATH) lflags |= LOOKUP_EMPTY; > + > + ret = user_path_at(from_dfd, from_pathname, lflags, &from_path); > + if (ret < 0) > + return ret; > + > + lflags = 0; > + if (flags & MOVE_MOUNT_T_SYMLINKS) lflags |= LOOKUP_FOLLOW; > + if (flags & MOVE_MOUNT_T_AUTOMOUNTS) lflags |= LOOKUP_AUTOMOUNT; > + if (flags & MOVE_MOUNT_T_EMPTY_PATH) lflags |= LOOKUP_EMPTY; > + > + ret = user_path_at(to_dfd, to_pathname, lflags, &to_path); > + if (ret < 0) > + goto out_from; > + > + ret = security_move_mount(&from_path, &to_path); > + if (ret < 0) > + goto out_to; Wouldn't you want to call this from do_move_mount() instead for consistency? If MS_MOVE and this thing perform the same logical operation, they should probably also call the same LSM hook. > + ret = do_move_mount(&from_path, &to_path); > + > +out_to: > + path_put(&to_path); > +out_from: > + path_put(&from_path); > + return ret; > +} [...] > diff --git a/include/uapi/linux/mount.h b/include/uapi/linux/mount.h > index fd7ae2e7eccf..3634e065836c 100644 > --- a/include/uapi/linux/mount.h > +++ b/include/uapi/linux/mount.h > @@ -61,4 +61,15 @@ > #define OPEN_TREE_CLONE 1 /* Clone the target tree and attach the clone */ > #define OPEN_TREE_CLOEXEC O_CLOEXEC /* Close the file on execve() */ > > +/* > + * move_mount() flags. > + */ > +#define MOVE_MOUNT_F_SYMLINKS 0x00000001 /* Follow symlinks on from path */ "Follow symlinks" sounds a bit misleading. LOOKUP_NOFOLLOW only applies to the last element of the path; and there is a pending patchset that's going to let userspace ask the kernel to actually not follow *any* symlinks on the path with O_NOSYMLINKS, so this might confuse people. Maybe change the comment to "don't follow trailing symlink", or something like that?
Hello, David Howells. I realized via https://lwn.net/Articles/792622/ that a new set of system calls for filesystem mounting has been added to Linux 5.2. But I feel that LSM modules are not ready to support these system calls. An example is move_mount() added by this patch. This patch added security_move_mount() LSM hook but none of in-tree LSM modules are providing "LSM_HOOK_INIT(move_mount, ...)" entry. Therefore, currently security_move_mount() is a no-op. At least for TOMOYO, I want to check mount manipulations caused by system calls because allowing mounts on arbitrary location is not acceptable for pathname based access control. What happened? I want TOMOYO to perform similar checks like mount() does. On 2019/02/20 2:08, David Howells wrote: > Add a move_mount() system call that will move a mount from one place to > another and, in the next commit, allow to attach an unattached mount tree. > > The new system call looks like the following: > > int move_mount(int from_dfd, const char *from_path, > int to_dfd, const char *to_path, > unsigned int flags); > > Signed-off-by: David Howells <dhowells@redhat.com> > cc: linux-api@vger.kernel.org > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
On Mon, Jul 08, 2019 at 09:02:10PM +0900, Tetsuo Handa wrote: > Hello, David Howells. > > I realized via https://lwn.net/Articles/792622/ that a new set of > system calls for filesystem mounting has been added to Linux 5.2. But > I feel that LSM modules are not ready to support these system calls. > > An example is move_mount() added by this patch. This patch added > security_move_mount() LSM hook but none of in-tree LSM modules are > providing "LSM_HOOK_INIT(move_mount, ...)" entry. Therefore, currently > security_move_mount() is a no-op. At least for TOMOYO, I want to check > mount manipulations caused by system calls because allowing mounts on > arbitrary location is not acceptable for pathname based access control. > What happened? I want TOMOYO to perform similar checks like mount() does. That would be like tomoyo_check_mount_acl(), right? if (need_dev) { /* Get mount point or device file. */ if (!dev_name || kern_path(dev_name, LOOKUP_FOLLOW, &path)) { error = -ENOENT; goto out; } obj.path1 = path; requested_dev_name = tomoyo_realpath_from_path(&path); if (!requested_dev_name) { error = -ENOENT; goto out; } } else { is an obvious crap for *ALL* cases. You are doing pathname resolution, followed by normalization and checks. Then the result of said pathname resolution is thrown out and it's redone (usually by something in fs/super.c). Results of _that_ get used. Could you spell TOCTOU? And yes, exploiting that takes a lot less than being able to do mount(2) in the first place - just pass it /proc/self/fd/69/<some acceptable path>/. with descriptor refering to opened root directory. With ~/<some acceptable path> being a symlink to whatever you actually want to hit. And descriptor 42 being your opened homedir. Now have that call of mount(2) overlap with dup2(42, 69) from another thread sharing your descriptor table. It doesn't take much to get the timing right, especially if you can arrange for some other activity frequently hitting namespace_sem at least shared (e.g. reading /proc/mounts in a loop from another process); that's likely to stall mount(2) at the point of lock_mount(), which comes *AFTER* the point where LSM hook is stuck into. Again, *ANY* checks on "dev_name" in ->sb_mount() instances are so much snake oil. Always had been.
Al Viro <viro@zeniv.linux.org.uk> writes: > On Mon, Jul 08, 2019 at 09:02:10PM +0900, Tetsuo Handa wrote: >> Hello, David Howells. >> >> I realized via https://lwn.net/Articles/792622/ that a new set of >> system calls for filesystem mounting has been added to Linux 5.2. But >> I feel that LSM modules are not ready to support these system calls. >> >> An example is move_mount() added by this patch. This patch added >> security_move_mount() LSM hook but none of in-tree LSM modules are >> providing "LSM_HOOK_INIT(move_mount, ...)" entry. Therefore, currently >> security_move_mount() is a no-op. At least for TOMOYO, I want to check >> mount manipulations caused by system calls because allowing mounts on >> arbitrary location is not acceptable for pathname based access control. >> What happened? I want TOMOYO to perform similar checks like mount() does. > > That would be like tomoyo_check_mount_acl(), right? > if (need_dev) { > /* Get mount point or device file. */ > if (!dev_name || kern_path(dev_name, LOOKUP_FOLLOW, &path)) { > error = -ENOENT; > goto out; > } > obj.path1 = path; > requested_dev_name = tomoyo_realpath_from_path(&path); > if (!requested_dev_name) { > error = -ENOENT; > goto out; > } > } else { > is an obvious crap for *ALL* cases. You are doing pathname resolution, > followed by normalization and checks. Then the result of said pathname > resolution is thrown out and it's redone (usually by something in fs/super.c). > Results of _that_ get used. > > Could you spell TOCTOU? And yes, exploiting that takes a lot less than > being able to do mount(2) in the first place - just pass it > /proc/self/fd/69/<some acceptable path>/. with descriptor refering to > opened root directory. With ~/<some acceptable path> being a symlink > to whatever you actually want to hit. And descriptor 42 being your > opened homedir. Now have that call of mount(2) overlap with dup2(42, 69) > from another thread sharing your descriptor table. It doesn't take much > to get the timing right, especially if you can arrange for some other > activity frequently hitting namespace_sem at least shared (e.g. reading > /proc/mounts in a loop from another process); that's likely to stall > mount(2) at the point of lock_mount(), which comes *AFTER* the point > where LSM hook is stuck into. > > Again, *ANY* checks on "dev_name" in ->sb_mount() instances are so much > snake oil. Always had been. Al you do realize that the TOCTOU you are talking about comes the system call API. TOMOYO can only be faulted for not playing in their own sandbox and not reaching out and fixing the vfs implementation details. Userspace has always had to very careful to only mount filesystems on paths that root completely controls and won't change. Further system calls for manipulating the mount tree have historically done a crap job of validating their inputs. Relying on the fact that only root can call them. So the idea of guarding against root doing something silly was silly. So I figure at the end of the day if the new security hooks for the new mount system calls don't make it possible to remove the TOCTOU that is on you and Dave. You two touched that code last after all. Not updating the new security hooks to at least do as good a job as the old security hooks is the definition of regression. So Al. Please simmer down and take the valid criticism. Eric
On Mon, Jul 08, 2019 at 12:12:21PM -0500, Eric W. Biederman wrote: > Al you do realize that the TOCTOU you are talking about comes the system > call API. TOMOYO can only be faulted for not playing in their own > sandbox and not reaching out and fixing the vfs implementation details. > > Userspace has always had to very careful to only mount filesystems > on paths that root completely controls and won't change. That has nothing whatsoever to do with the path where you are mounting something. _That_ is actually looked up before ->sb_mount() gets called; no TOCTOU there. The thing where ->sb_mount() is fucked by design is its handling of * device name * old tree in mount --bind * old tree in mount --move * things like journal name (not that any of the instances had tried to do anything with that) All of those *do* have TOCTOU, and that's an inevitable result of the idiotic hook fetishism of LSM design. Instead of "we want something to happen when such-and-such predicate is about to change", it's "lemme run my code, the earlier the better, I don't care about any damn predicates, it's all too complicated anyway, whaddya mean racy?" Any time you have pathname resolution done twice, it's a built-in race. If you want *ALL* checks on mount(2) to be done before the mean, nasty kernel code gets to decide anything (bind/move/mount/etc. all squashed together, just let us have at the syscall arguments, mmkay?) - that's precisely what you get. And no, that TOCTOU is not in syscall API. "open() of an untrusted pathname may end up trying to open hell knows what" is one thing; "open() of an untrusted pathname may apply MAC checks to one object and open something entirely different" is another. The former is inherent to syscall API. The latter would be a badly fucked up implementation (we don't have that issue on open(2), thankfully). To make it clear, TOMOYO is not at fault here; LSM "architecture" is. Note, BTW, that TOMOYO checks there do *NOT* limit the input pathname at all - only the destination of the first pathwalk. Repeating it may easily lead to an entirely different place. Canonicalized pathname is derived from pathwalk result; having concluded that it's perfectly fine for the operation requested is pure security theatre - it * says nothing about the trustedness of the original pathname * may have nothing whatsoever to the object yielded by the second pathwalk, which is what'll end up actually used. It's not even "this thing walks through /proc, and thus not to be trusted to be stable" - the checks won't notice where the damn thing had been. When somebody proposes _useful_ MAC for mount --move (and that really can't be done at the level of syscall entry - we need to have already figured out that with given combination of flags the 1st argument of mount(2) will be a pathname *and* already looked it up), sure - it will be added to do_move_mount(), which is where we have all lookups done, and apply both for mount() and move_mount(). Right now anyone relying upon DAC enforced for MS_MOVE has worse problems than "attacker will use move_mount(2) and bypass my policy" - the same attacker can bloody well bypass those with nothing more exotic than clone(2) and dup2(2) (and mount(2), of course). And it's not just MS_MOVE (or MS_BIND). Anyone trying to prevent mounting e.g. ext2 from untrusted device and do that on the level of ->sb_mount() *is* *bloody* *well* *fucked*. ->sb_mount() is simply the wrong place for that.
On Mon, Jul 08, 2019 at 07:01:32PM +0100, Al Viro wrote:
> Right now anyone relying upon DAC enforced for MS_MOVE has worse problems
That'd be MAC, of course. Sorry.
On Mon, Jul 08, 2019 at 07:01:32PM +0100, Al Viro wrote: > On Mon, Jul 08, 2019 at 12:12:21PM -0500, Eric W. Biederman wrote: > > > Al you do realize that the TOCTOU you are talking about comes the system > > call API. TOMOYO can only be faulted for not playing in their own > > sandbox and not reaching out and fixing the vfs implementation details. PS: the fact that mount(2) has been overloaded to hell and back (including MS_MOVE, which goes back to v2.5.0.5) predates the introduction of ->sb_mount() and LSM in general (2.5.27). MS_BIND is 2.4.0-test9pre2. In all the years since the introduction of ->sb_mount() I've seen zero questions from LSM folks regarding a sane place for those checks. What I have seen was "we want it immediately upon the syscall entry, let the module figure out what to do" in reply to several times I tried to tell them "folks, it's called in a bad place; you want the checks applied to objects, not to raw string arguments". As it is, we have easily bypassable checks on mount(2) (by way of ->sb_mount(); there are other hooks also in the game for remounts and new mounts). I see no point whatsoever trying to duplicate ->sb_mount() on the top level of move_mount(2). When and if sane checks are agreed upon for that thing, they certainly should be used both for MS_MOVE case of mount(2) and for move_mount(2). And that'll come for free from calling those in do_move_mount(). They won't be the first thing called in mount(2) - we demultiplex first, decide that we have a move and do pathname resolution on source. And that's precisely what we need to avoid the TOCTOU there. I'm sorry, but this "run the hook at the very beginning, the modules know better what they want, just give them as close to syscall arguments as possible before even looking at the flags" model is wrong, plain and simple. As for the MS_MOVE checks, the arguments are clear enough (two struct path *, same as what we pass to do_move_mount()). AFAICS, only tomoyo and apparmor are trying to do anything for MS_MOVE in ->sb_mount(), and both look like it should be easy enough to implement what said checks intend to do (probably - assuming that aa_move_mount() doesn't depend upon having its kern_path() inside the __begin_current_label_crit_section()/ __end_current_label_crit_section() pair; looks like it shouldn't be, but that's for apparmor folks to decide). That's really for LSM folks, though - I've given up on convincing (or even getting a rationale out of) them on anything related to hook semantics years ago.
Al Viro <viro@zeniv.linux.org.uk> writes: > On Mon, Jul 08, 2019 at 07:01:32PM +0100, Al Viro wrote: >> On Mon, Jul 08, 2019 at 12:12:21PM -0500, Eric W. Biederman wrote: >> >> > Al you do realize that the TOCTOU you are talking about comes the system >> > call API. TOMOYO can only be faulted for not playing in their own >> > sandbox and not reaching out and fixing the vfs implementation details. > > PS: the fact that mount(2) has been overloaded to hell and back (including > MS_MOVE, which goes back to v2.5.0.5) predates the introduction of ->sb_mount() > and LSM in general (2.5.27). MS_BIND is 2.4.0-test9pre2. > > In all the years since the introduction of ->sb_mount() I've seen zero > questions from LSM folks regarding a sane place for those checks. What I have > seen was "we want it immediately upon the syscall entry, let the module > figure out what to do" in reply to several times I tried to tell them "folks, > it's called in a bad place; you want the checks applied to objects, not to > raw string arguments". > > As it is, we have easily bypassable checks on mount(2) (by way of ->sb_mount(); > there are other hooks also in the game for remounts and new mounts). > > I see no point whatsoever trying to duplicate ->sb_mount() on the top level > of move_mount(2). When and if sane checks are agreed upon for that thing, > they certainly should be used both for MS_MOVE case of mount(2) and for > move_mount(2). And that'll come for free from calling those in do_move_mount(). > They won't be the first thing called in mount(2) - we demultiplex first, > decide that we have a move and do pathname resolution on source. And that's > precisely what we need to avoid the TOCTOU there. > > I'm sorry, but this "run the hook at the very beginning, the modules know > better what they want, just give them as close to syscall arguments as > possible before even looking at the flags" model is wrong, plain and simple. > > As for the MS_MOVE checks, the arguments are clear enough (two struct path *, > same as what we pass to do_move_mount()). AFAICS, only tomoyo and > apparmor are trying to do anything for MS_MOVE in ->sb_mount(), and both > look like it should be easy enough to implement what said checks intend > to do (probably - assuming that aa_move_mount() doesn't depend upon > having its kern_path() inside the __begin_current_label_crit_section()/ > __end_current_label_crit_section() pair; looks like it shouldn't be, > but that's for apparmor folks to decide). > > That's really for LSM folks, though - I've given up on convincing > (or even getting a rationale out of) them on anything related to hook > semantics years ago. I have found the LSM folks in recent years to be rather reasonable, especially when something concrete has been proposed. A quick look suggests that the new security_mount_move is a reasonable hook for the mount_move check. Tetsuo, do you think you can implement the checks you need for Tomoyo for mount_move on top of the new security_mount_move? Al is proposing that similar hooks be added for the other subcases of mount so that less racy hooks can be implemented. Tetsuo do you have any problem with that? Tetsuo whatever the virtues of this patchset getting merged into Linus's tree it is merged now, so the only thing we can do now is roll up our sleeves go through everything and fix the regressions/bugs/issues that have emerged with the new mount api. Eric
On 2019/07/09 9:13, Eric W. Biederman wrote: > Tetsuo, do you think you can implement the checks you need for Tomoyo > for mount_move on top of the new security_mount_move? I hope "Yes", for I'm not aware what will become possible with the new mount syscalls. For example, if it becomes possible to use multiple source device files or directories, TOMOYO is not ready to check multiple source device files or directories, for currently TOMOYO uses file mount $DEVICE $MOUNTPOINT $FILESYSTEM $OPTIONS ( https://tomoyo.osdn.jp/2.6/policy-specification/domain-policy-syntax.html#file_mount ) syntax. If only optional part (any arguments which were previously passed via "const void *data" of mount() syscall) differs, current syntax can be reused. > > Al is proposing that similar hooks be added for the other subcases of > mount so that less racy hooks can be implemented. Tetsuo do you have > any problem with that? I welcome improvements that get rid of calling kern_path() from LSM hooks. In the past, I incorrectly implemented which MS_* flag should be checked (commit df91e49477a9be15 ("TOMOYO: Fix mount flags checking order.")). If LSM hooks for mount manipulation are divided into multiple LSM hooks (along with changing the argument from "const char *" to "const struct path *"), such mistakes can be avoided. > Tetsuo whatever the virtues of this patchset getting merged into Linus's > tree it is merged now, so the only thing we can do now is roll up our > sleeves go through everything and fix the regressions/bugs/issues that > have emerged with the new mount api. For now, can we apply this patch for 5.2-stable ? From dd62fab0592e02580fd5a34222a2d11bfc179f61 Mon Sep 17 00:00:00 2001 From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Date: Tue, 9 Jul 2019 19:05:49 +0900 Subject: [PATCH] LSM: Disable move_mount() syscall when TOMOYO or AppArmor is enabled. Commit 2db154b3ea8e14b0 ("vfs: syscall: Add move_mount(2) to move mounts around") introduced security_move_mount() LSM hook, but we missed that TOMOYO and AppArmor did not implement hooks for checking move_mount(2). For pathname based access controls like TOMOYO and AppArmor, unchecked mount manipulation is not acceptable. Therefore, until TOMOYO and AppArmor implement hooks, in order to avoid unchecked mount manipulation, pretend as if move_mount(2) is unavailable when either TOMOYO or AppArmor is enabled. Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Fixes: 2db154b3ea8e14b0 ("vfs: syscall: Add move_mount(2) to move mounts around") Cc: stable@vger.kernel.org # 5.2 --- include/linux/lsm_hooks.h | 6 ++++++ security/apparmor/lsm.c | 1 + security/tomoyo/tomoyo.c | 1 + 3 files changed, 8 insertions(+) diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 47f58cf..cd411b7 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -2142,4 +2142,10 @@ static inline void security_delete_hooks(struct security_hook_list *hooks, extern int lsm_inode_alloc(struct inode *inode); +static inline int no_move_mount(const struct path *from_path, + const struct path *to_path) +{ + return -ENOSYS; +} + #endif /* ! __LINUX_LSM_HOOKS_H */ diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index ec3a928..5cdf63b 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -1158,6 +1158,7 @@ struct lsm_blob_sizes apparmor_blob_sizes __lsm_ro_after_init = { LSM_HOOK_INIT(capable, apparmor_capable), LSM_HOOK_INIT(sb_mount, apparmor_sb_mount), + LSM_HOOK_INIT(move_mount, no_move_mount), LSM_HOOK_INIT(sb_umount, apparmor_sb_umount), LSM_HOOK_INIT(sb_pivotroot, apparmor_sb_pivotroot), diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c index 716c92e..be1b1a1 100644 --- a/security/tomoyo/tomoyo.c +++ b/security/tomoyo/tomoyo.c @@ -558,6 +558,7 @@ static void tomoyo_task_free(struct task_struct *task) LSM_HOOK_INIT(path_chown, tomoyo_path_chown), LSM_HOOK_INIT(path_chroot, tomoyo_path_chroot), LSM_HOOK_INIT(sb_mount, tomoyo_sb_mount), + LSM_HOOK_INIT(move_mount, no_move_mount), LSM_HOOK_INIT(sb_umount, tomoyo_sb_umount), LSM_HOOK_INIT(sb_pivotroot, tomoyo_sb_pivotroot), LSM_HOOK_INIT(socket_bind, tomoyo_socket_bind),
I did not see AppArmor patch for this problem in 5.3-rc1. John, are you OK with this patch for 5.2-stable and 5.3 ? On 2019/07/09 19:51, Tetsuo Handa wrote: > For now, can we apply this patch for 5.2-stable ? > > >>From dd62fab0592e02580fd5a34222a2d11bfc179f61 Mon Sep 17 00:00:00 2001 > From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Date: Tue, 9 Jul 2019 19:05:49 +0900 > Subject: [PATCH] LSM: Disable move_mount() syscall when TOMOYO or AppArmor is enabled. > > Commit 2db154b3ea8e14b0 ("vfs: syscall: Add move_mount(2) to move mounts > around") introduced security_move_mount() LSM hook, but we missed that > TOMOYO and AppArmor did not implement hooks for checking move_mount(2). > For pathname based access controls like TOMOYO and AppArmor, unchecked > mount manipulation is not acceptable. Therefore, until TOMOYO and AppArmor > implement hooks, in order to avoid unchecked mount manipulation, pretend > as if move_mount(2) is unavailable when either TOMOYO or AppArmor is > enabled. > > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > Fixes: 2db154b3ea8e14b0 ("vfs: syscall: Add move_mount(2) to move mounts around") > Cc: stable@vger.kernel.org # 5.2 > --- > include/linux/lsm_hooks.h | 6 ++++++ > security/apparmor/lsm.c | 1 + > security/tomoyo/tomoyo.c | 1 + > 3 files changed, 8 insertions(+) > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 47f58cf..cd411b7 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -2142,4 +2142,10 @@ static inline void security_delete_hooks(struct security_hook_list *hooks, > > extern int lsm_inode_alloc(struct inode *inode); > > +static inline int no_move_mount(const struct path *from_path, > + const struct path *to_path) > +{ > + return -ENOSYS; > +} > + > #endif /* ! __LINUX_LSM_HOOKS_H */ > diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c > index ec3a928..5cdf63b 100644 > --- a/security/apparmor/lsm.c > +++ b/security/apparmor/lsm.c > @@ -1158,6 +1158,7 @@ struct lsm_blob_sizes apparmor_blob_sizes __lsm_ro_after_init = { > LSM_HOOK_INIT(capable, apparmor_capable), > > LSM_HOOK_INIT(sb_mount, apparmor_sb_mount), > + LSM_HOOK_INIT(move_mount, no_move_mount), > LSM_HOOK_INIT(sb_umount, apparmor_sb_umount), > LSM_HOOK_INIT(sb_pivotroot, apparmor_sb_pivotroot), > > diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c > index 716c92e..be1b1a1 100644 > --- a/security/tomoyo/tomoyo.c > +++ b/security/tomoyo/tomoyo.c > @@ -558,6 +558,7 @@ static void tomoyo_task_free(struct task_struct *task) > LSM_HOOK_INIT(path_chown, tomoyo_path_chown), > LSM_HOOK_INIT(path_chroot, tomoyo_path_chroot), > LSM_HOOK_INIT(sb_mount, tomoyo_sb_mount), > + LSM_HOOK_INIT(move_mount, no_move_mount), > LSM_HOOK_INIT(sb_umount, tomoyo_sb_umount), > LSM_HOOK_INIT(sb_pivotroot, tomoyo_sb_pivotroot), > LSM_HOOK_INIT(socket_bind, tomoyo_socket_bind), >
On 7/22/19 3:12 AM, Tetsuo Handa wrote: > I did not see AppArmor patch for this problem in 5.3-rc1. > John, are you OK with this patch for 5.2-stable and 5.3 ? > yes, I have some larger mount rework and clean-up to do and an apparmor patch for this is waiting on that. Looking at the thread I am wondering where my previous reply is, probably lost in a mail client crash, I have had a few of those lately. Acked-by: John Johansen <john.johansen@canonical.com> > On 2019/07/09 19:51, Tetsuo Handa wrote: >> For now, can we apply this patch for 5.2-stable ? >> >> >> >From dd62fab0592e02580fd5a34222a2d11bfc179f61 Mon Sep 17 00:00:00 2001 >> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> >> Date: Tue, 9 Jul 2019 19:05:49 +0900 >> Subject: [PATCH] LSM: Disable move_mount() syscall when TOMOYO or AppArmor is enabled. >> >> Commit 2db154b3ea8e14b0 ("vfs: syscall: Add move_mount(2) to move mounts >> around") introduced security_move_mount() LSM hook, but we missed that >> TOMOYO and AppArmor did not implement hooks for checking move_mount(2). >> For pathname based access controls like TOMOYO and AppArmor, unchecked >> mount manipulation is not acceptable. Therefore, until TOMOYO and AppArmor >> implement hooks, in order to avoid unchecked mount manipulation, pretend >> as if move_mount(2) is unavailable when either TOMOYO or AppArmor is >> enabled. >> >> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> >> Fixes: 2db154b3ea8e14b0 ("vfs: syscall: Add move_mount(2) to move mounts around") >> Cc: stable@vger.kernel.org # 5.2 >> --- >> include/linux/lsm_hooks.h | 6 ++++++ >> security/apparmor/lsm.c | 1 + >> security/tomoyo/tomoyo.c | 1 + >> 3 files changed, 8 insertions(+) >> >> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h >> index 47f58cf..cd411b7 100644 >> --- a/include/linux/lsm_hooks.h >> +++ b/include/linux/lsm_hooks.h >> @@ -2142,4 +2142,10 @@ static inline void security_delete_hooks(struct security_hook_list *hooks, >> >> extern int lsm_inode_alloc(struct inode *inode); >> >> +static inline int no_move_mount(const struct path *from_path, >> + const struct path *to_path) >> +{ >> + return -ENOSYS; >> +} >> + >> #endif /* ! __LINUX_LSM_HOOKS_H */ >> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c >> index ec3a928..5cdf63b 100644 >> --- a/security/apparmor/lsm.c >> +++ b/security/apparmor/lsm.c >> @@ -1158,6 +1158,7 @@ struct lsm_blob_sizes apparmor_blob_sizes __lsm_ro_after_init = { >> LSM_HOOK_INIT(capable, apparmor_capable), >> >> LSM_HOOK_INIT(sb_mount, apparmor_sb_mount), >> + LSM_HOOK_INIT(move_mount, no_move_mount), >> LSM_HOOK_INIT(sb_umount, apparmor_sb_umount), >> LSM_HOOK_INIT(sb_pivotroot, apparmor_sb_pivotroot), >> >> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c >> index 716c92e..be1b1a1 100644 >> --- a/security/tomoyo/tomoyo.c >> +++ b/security/tomoyo/tomoyo.c >> @@ -558,6 +558,7 @@ static void tomoyo_task_free(struct task_struct *task) >> LSM_HOOK_INIT(path_chown, tomoyo_path_chown), >> LSM_HOOK_INIT(path_chroot, tomoyo_path_chroot), >> LSM_HOOK_INIT(sb_mount, tomoyo_sb_mount), >> + LSM_HOOK_INIT(move_mount, no_move_mount), >> LSM_HOOK_INIT(sb_umount, tomoyo_sb_umount), >> LSM_HOOK_INIT(sb_pivotroot, tomoyo_sb_pivotroot), >> LSM_HOOK_INIT(socket_bind, tomoyo_socket_bind), >> >
Al, will you send this patch to 5.3-rcX via vfs.git tree? On 2019/07/23 13:16, John Johansen wrote: > On 7/22/19 3:12 AM, Tetsuo Handa wrote: >> I did not see AppArmor patch for this problem in 5.3-rc1. >> John, are you OK with this patch for 5.2-stable and 5.3 ? >> > yes, I have some larger mount rework and clean-up to do and an apparmor > patch for this is waiting on that. Looking at the thread I am wondering > where my previous reply is, probably lost in a mail client crash, I have > had a few of those lately. > > Acked-by: John Johansen <john.johansen@canonical.com> > > >> On 2019/07/09 19:51, Tetsuo Handa wrote: >>> For now, can we apply this patch for 5.2-stable ? >>> >>> >>> >From dd62fab0592e02580fd5a34222a2d11bfc179f61 Mon Sep 17 00:00:00 2001 >>> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> >>> Date: Tue, 9 Jul 2019 19:05:49 +0900 >>> Subject: [PATCH] LSM: Disable move_mount() syscall when TOMOYO or AppArmor is enabled. >>> >>> Commit 2db154b3ea8e14b0 ("vfs: syscall: Add move_mount(2) to move mounts >>> around") introduced security_move_mount() LSM hook, but we missed that >>> TOMOYO and AppArmor did not implement hooks for checking move_mount(2). >>> For pathname based access controls like TOMOYO and AppArmor, unchecked >>> mount manipulation is not acceptable. Therefore, until TOMOYO and AppArmor >>> implement hooks, in order to avoid unchecked mount manipulation, pretend >>> as if move_mount(2) is unavailable when either TOMOYO or AppArmor is >>> enabled. >>> >>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> >>> Fixes: 2db154b3ea8e14b0 ("vfs: syscall: Add move_mount(2) to move mounts around") >>> Cc: stable@vger.kernel.org # 5.2 >>> --- >>> include/linux/lsm_hooks.h | 6 ++++++ >>> security/apparmor/lsm.c | 1 + >>> security/tomoyo/tomoyo.c | 1 + >>> 3 files changed, 8 insertions(+) >>> >>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h >>> index 47f58cf..cd411b7 100644 >>> --- a/include/linux/lsm_hooks.h >>> +++ b/include/linux/lsm_hooks.h >>> @@ -2142,4 +2142,10 @@ static inline void security_delete_hooks(struct security_hook_list *hooks, >>> >>> extern int lsm_inode_alloc(struct inode *inode); >>> >>> +static inline int no_move_mount(const struct path *from_path, >>> + const struct path *to_path) >>> +{ >>> + return -ENOSYS; >>> +} >>> + >>> #endif /* ! __LINUX_LSM_HOOKS_H */ >>> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c >>> index ec3a928..5cdf63b 100644 >>> --- a/security/apparmor/lsm.c >>> +++ b/security/apparmor/lsm.c >>> @@ -1158,6 +1158,7 @@ struct lsm_blob_sizes apparmor_blob_sizes __lsm_ro_after_init = { >>> LSM_HOOK_INIT(capable, apparmor_capable), >>> >>> LSM_HOOK_INIT(sb_mount, apparmor_sb_mount), >>> + LSM_HOOK_INIT(move_mount, no_move_mount), >>> LSM_HOOK_INIT(sb_umount, apparmor_sb_umount), >>> LSM_HOOK_INIT(sb_pivotroot, apparmor_sb_pivotroot), >>> >>> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c >>> index 716c92e..be1b1a1 100644 >>> --- a/security/tomoyo/tomoyo.c >>> +++ b/security/tomoyo/tomoyo.c >>> @@ -558,6 +558,7 @@ static void tomoyo_task_free(struct task_struct *task) >>> LSM_HOOK_INIT(path_chown, tomoyo_path_chown), >>> LSM_HOOK_INIT(path_chroot, tomoyo_path_chroot), >>> LSM_HOOK_INIT(sb_mount, tomoyo_sb_mount), >>> + LSM_HOOK_INIT(move_mount, no_move_mount), >>> LSM_HOOK_INIT(sb_umount, tomoyo_sb_umount), >>> LSM_HOOK_INIT(sb_pivotroot, tomoyo_sb_pivotroot), >>> LSM_HOOK_INIT(socket_bind, tomoyo_socket_bind), >>> >> > >
On Mon, 8 Jul 2019, Al Viro wrote: > On Mon, Jul 08, 2019 at 07:01:32PM +0100, Al Viro wrote: > > On Mon, Jul 08, 2019 at 12:12:21PM -0500, Eric W. Biederman wrote: > > > > > Al you do realize that the TOCTOU you are talking about comes the system > > > call API. TOMOYO can only be faulted for not playing in their own > > > sandbox and not reaching out and fixing the vfs implementation details. > > PS: the fact that mount(2) has been overloaded to hell and back (including > MS_MOVE, which goes back to v2.5.0.5) predates the introduction of ->sb_mount() > and LSM in general (2.5.27). MS_BIND is 2.4.0-test9pre2. > > In all the years since the introduction of ->sb_mount() I've seen zero > questions from LSM folks regarding a sane place for those checks. What I have > seen was "we want it immediately upon the syscall entry, let the module > figure out what to do" in reply to several times I tried to tell them "folks, > it's called in a bad place; you want the checks applied to objects, not to > raw string arguments". > > As it is, we have easily bypassable checks on mount(2) (by way of ->sb_mount(); > there are other hooks also in the game for remounts and new mounts). What are your recommendations for placing these checks?
On Wed, Jul 24, 2019 at 07:45:07AM +1000, James Morris wrote: > On Mon, 8 Jul 2019, Al Viro wrote: > > > On Mon, Jul 08, 2019 at 07:01:32PM +0100, Al Viro wrote: > > > On Mon, Jul 08, 2019 at 12:12:21PM -0500, Eric W. Biederman wrote: > > > > > > > Al you do realize that the TOCTOU you are talking about comes the system > > > > call API. TOMOYO can only be faulted for not playing in their own > > > > sandbox and not reaching out and fixing the vfs implementation details. > > > > PS: the fact that mount(2) has been overloaded to hell and back (including > > MS_MOVE, which goes back to v2.5.0.5) predates the introduction of ->sb_mount() > > and LSM in general (2.5.27). MS_BIND is 2.4.0-test9pre2. > > > > In all the years since the introduction of ->sb_mount() I've seen zero > > questions from LSM folks regarding a sane place for those checks. What I have > > seen was "we want it immediately upon the syscall entry, let the module > > figure out what to do" in reply to several times I tried to tell them "folks, > > it's called in a bad place; you want the checks applied to objects, not to > > raw string arguments". > > > > As it is, we have easily bypassable checks on mount(2) (by way of ->sb_mount(); > > there are other hooks also in the game for remounts and new mounts). > > What are your recommendations for placing these checks? For MS_MOVE: do_move_mount(), after lock_mount(), when the mount tree is stable and pathnames are resolved. For MS_BIND: do_loopback(), ditto. Incidentally, for pivot_root(2) I would also suggest moving that past the lock_mount(), for the same reasons. For propagation flag changes: do_change_type(), after namespace_lock(). For per-mount flag changes: do_reconfigure_mnt(), possibly after having grabbed ->s_umount. For fs remount: IMO it should be handled purely in ->sb_remount(). For new mount: really depends upon the filesystem type, I'm afraid. There's nothing type-independent that can be done - in the best case you can say "no mounting block filesystems from that device"; note that for NFS the meaning of the argument is entirely different and you *can* have something like foo.local.org: as a name of symlink (or directory), so blanket "you can mount foo.local.org:/srv/nfs/blah" is asking for trouble - mount -t ext4 foo.local.org:/srv/nfs/blah /mnt can bloody well end up successfully mounting a very untrusted usb disk. Note, BTW, that things like cramfs can be given * mtd:mtd_device_name * mtd<decimal number> * block device pathname The last one needs to be resolved/canonicalized/whatnot. The other two must *NOT* - there's nothing to stop the attacker from ln -s /dev/mtdblock0 ./mtd12 and confusing the fsck out of your LSM (it would assume that we are trying to get mtd0 when in reality it'll mount mtd12). The rules really need to be type-dependent; ->sb_set_mnt_opts() has the state after the fs has been initialized to work with, but anything trying to stop the attempt to set the damn thing up in the first place (as current ->sb_mount() would) must be called from the inside of individual ->get_tree()/->mount() instance (or a helper used by such).
This 5.2-stable/5.3 patch is stalling. Should I ask different route?
On 2019/07/23 22:45, Tetsuo Handa wrote:
> Al, will you send this patch to 5.3-rcX via vfs.git tree?
diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl index ea1b413afd47..76d092b7d1b0 100644 --- a/arch/x86/entry/syscalls/syscall_32.tbl +++ b/arch/x86/entry/syscalls/syscall_32.tbl @@ -399,3 +399,4 @@ 385 i386 io_pgetevents sys_io_pgetevents __ia32_compat_sys_io_pgetevents 386 i386 rseq sys_rseq __ia32_sys_rseq 387 i386 open_tree sys_open_tree __ia32_sys_open_tree +388 i386 move_mount sys_move_mount __ia32_sys_move_mount diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl index 0545bed581dc..37ba4e65eee6 100644 --- a/arch/x86/entry/syscalls/syscall_64.tbl +++ b/arch/x86/entry/syscalls/syscall_64.tbl @@ -344,6 +344,7 @@ 333 common io_pgetevents __x64_sys_io_pgetevents 334 common rseq __x64_sys_rseq 335 common open_tree __x64_sys_open_tree +336 common move_mount __x64_sys_move_mount # # x32-specific system call numbers start at 512 to avoid cache impact diff --git a/fs/namespace.c b/fs/namespace.c index 112d46f26fc3..f10122028a11 100644 --- a/fs/namespace.c +++ b/fs/namespace.c @@ -2537,72 +2537,81 @@ static inline int tree_contains_unbindable(struct mount *mnt) return 0; } -static int do_move_mount(struct path *path, const char *old_name) +static int do_move_mount(struct path *old_path, struct path *new_path) { - struct path old_path, parent_path; + struct path parent_path = {.mnt = NULL, .dentry = NULL}; struct mount *p; struct mount *old; struct mountpoint *mp; int err; - if (!old_name || !*old_name) - return -EINVAL; - err = kern_path(old_name, LOOKUP_FOLLOW, &old_path); - if (err) - return err; - mp = lock_mount(path); - err = PTR_ERR(mp); + mp = lock_mount(new_path); if (IS_ERR(mp)) - goto out; + return PTR_ERR(mp); - old = real_mount(old_path.mnt); - p = real_mount(path->mnt); + old = real_mount(old_path->mnt); + p = real_mount(new_path->mnt); err = -EINVAL; if (!check_mnt(p) || !check_mnt(old)) - goto out1; + goto out; - if (old->mnt.mnt_flags & MNT_LOCKED) - goto out1; + if (!mnt_has_parent(old)) + goto out; - err = -EINVAL; - if (old_path.dentry != old_path.mnt->mnt_root) - goto out1; + if (old->mnt.mnt_flags & MNT_LOCKED) + goto out; - if (!mnt_has_parent(old)) - goto out1; + if (old_path->dentry != old_path->mnt->mnt_root) + goto out; - if (d_is_dir(path->dentry) != - d_is_dir(old_path.dentry)) - goto out1; + if (d_is_dir(new_path->dentry) != + d_is_dir(old_path->dentry)) + goto out; /* * Don't move a mount residing in a shared parent. */ if (IS_MNT_SHARED(old->mnt_parent)) - goto out1; + goto out; /* * Don't move a mount tree containing unbindable mounts to a destination * mount which is shared. */ if (IS_MNT_SHARED(p) && tree_contains_unbindable(old)) - goto out1; + goto out; err = -ELOOP; for (; mnt_has_parent(p); p = p->mnt_parent) if (p == old) - goto out1; + goto out; - err = attach_recursive_mnt(old, real_mount(path->mnt), mp, &parent_path); + err = attach_recursive_mnt(old, real_mount(new_path->mnt), mp, + &parent_path); if (err) - goto out1; + goto out; /* if the mount is moved, it should no longer be expire * automatically */ list_del_init(&old->mnt_expire); -out1: - unlock_mount(mp); out: + unlock_mount(mp); if (!err) path_put(&parent_path); + return err; +} + +static int do_move_mount_old(struct path *path, const char *old_name) +{ + struct path old_path; + int err; + + if (!old_name || !*old_name) + return -EINVAL; + + err = kern_path(old_name, LOOKUP_FOLLOW, &old_path); + if (err) + return err; + + err = do_move_mount(&old_path, path); path_put(&old_path); return err; } @@ -3050,7 +3059,7 @@ long do_mount(const char *dev_name, const char __user *dir_name, else if (flags & (MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE)) retval = do_change_type(&path, flags); else if (flags & MS_MOVE) - retval = do_move_mount(&path, dev_name); + retval = do_move_mount_old(&path, dev_name); else retval = do_new_mount(&path, type_page, sb_flags, mnt_flags, dev_name, data_page); @@ -3278,6 +3287,61 @@ SYSCALL_DEFINE5(mount, char __user *, dev_name, char __user *, dir_name, return ksys_mount(dev_name, dir_name, type, flags, data); } +/* + * Move a mount from one place to another. + * + * Note the flags value is a combination of MOVE_MOUNT_* flags. + */ +SYSCALL_DEFINE5(move_mount, + int, from_dfd, const char *, from_pathname, + int, to_dfd, const char *, to_pathname, + unsigned int, flags) +{ + struct path from_path, to_path; + unsigned int lflags; + int ret = 0; + + if (!may_mount()) + return -EPERM; + + if (flags & ~MOVE_MOUNT__MASK) + return -EINVAL; + + /* If someone gives a pathname, they aren't permitted to move + * from an fd that requires unmount as we can't get at the flag + * to clear it afterwards. + */ + lflags = 0; + if (flags & MOVE_MOUNT_F_SYMLINKS) lflags |= LOOKUP_FOLLOW; + if (flags & MOVE_MOUNT_F_AUTOMOUNTS) lflags |= LOOKUP_AUTOMOUNT; + if (flags & MOVE_MOUNT_F_EMPTY_PATH) lflags |= LOOKUP_EMPTY; + + ret = user_path_at(from_dfd, from_pathname, lflags, &from_path); + if (ret < 0) + return ret; + + lflags = 0; + if (flags & MOVE_MOUNT_T_SYMLINKS) lflags |= LOOKUP_FOLLOW; + if (flags & MOVE_MOUNT_T_AUTOMOUNTS) lflags |= LOOKUP_AUTOMOUNT; + if (flags & MOVE_MOUNT_T_EMPTY_PATH) lflags |= LOOKUP_EMPTY; + + ret = user_path_at(to_dfd, to_pathname, lflags, &to_path); + if (ret < 0) + goto out_from; + + ret = security_move_mount(&from_path, &to_path); + if (ret < 0) + goto out_to; + + ret = do_move_mount(&from_path, &to_path); + +out_to: + path_put(&to_path); +out_from: + path_put(&from_path); + return ret; +} + /* * Return true if path is reachable from root * diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 356e78fe90a8..89892a031ecf 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -160,6 +160,10 @@ * Parse a string of security data filling in the opts structure * @options string containing all mount options known by the LSM * @opts binary data structure usable by the LSM + * @move_mount: + * Check permission before a mount is moved. + * @from_path indicates the mount that is going to be moved. + * @to_path indicates the mountpoint that will be mounted upon. * @dentry_init_security: * Compute a context for a dentry as the inode is not yet available * since NFSv4 has no label backed by an EA anyway. @@ -1500,6 +1504,7 @@ union security_list_options { unsigned long *set_kern_flags); int (*sb_add_mnt_opt)(const char *option, const char *val, int len, void **mnt_opts); + int (*move_mount)(const struct path *from_path, const struct path *to_path); int (*dentry_init_security)(struct dentry *dentry, int mode, const struct qstr *name, void **ctx, u32 *ctxlen); @@ -1835,6 +1840,7 @@ struct security_hook_heads { struct hlist_head sb_set_mnt_opts; struct hlist_head sb_clone_mnt_opts; struct hlist_head sb_add_mnt_opt; + struct hlist_head move_mount; struct hlist_head dentry_init_security; struct hlist_head dentry_create_files_as; #ifdef CONFIG_SECURITY_PATH diff --git a/include/linux/security.h b/include/linux/security.h index f28a1ebfd78e..edc48e61c8b4 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -247,6 +247,7 @@ int security_sb_clone_mnt_opts(const struct super_block *oldsb, unsigned long *set_kern_flags); int security_add_mnt_opt(const char *option, const char *val, int len, void **mnt_opts); +int security_move_mount(const struct path *from_path, const struct path *to_path); int security_dentry_init_security(struct dentry *dentry, int mode, const struct qstr *name, void **ctx, u32 *ctxlen); @@ -609,6 +610,12 @@ static inline int security_add_mnt_opt(const char *option, const char *val, return 0; } +static inline int security_move_mount(const struct path *from_path, + const struct path *to_path) +{ + return 0; +} + static inline int security_inode_alloc(struct inode *inode) { return 0; diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h index 07bca6796498..13289cdf6b52 100644 --- a/include/linux/syscalls.h +++ b/include/linux/syscalls.h @@ -927,6 +927,9 @@ asmlinkage long sys_statx(int dfd, const char __user *path, unsigned flags, asmlinkage long sys_rseq(struct rseq __user *rseq, uint32_t rseq_len, int flags, uint32_t sig); asmlinkage long sys_open_tree(int dfd, const char __user *path, unsigned flags); +asmlinkage long sys_move_mount(int from_dfd, const char __user *from_path, + int to_dfd, const char __user *to_path, + unsigned int ms_flags); /* * Architecture-specific system calls diff --git a/include/uapi/linux/mount.h b/include/uapi/linux/mount.h index fd7ae2e7eccf..3634e065836c 100644 --- a/include/uapi/linux/mount.h +++ b/include/uapi/linux/mount.h @@ -61,4 +61,15 @@ #define OPEN_TREE_CLONE 1 /* Clone the target tree and attach the clone */ #define OPEN_TREE_CLOEXEC O_CLOEXEC /* Close the file on execve() */ +/* + * move_mount() flags. + */ +#define MOVE_MOUNT_F_SYMLINKS 0x00000001 /* Follow symlinks on from path */ +#define MOVE_MOUNT_F_AUTOMOUNTS 0x00000002 /* Follow automounts on from path */ +#define MOVE_MOUNT_F_EMPTY_PATH 0x00000004 /* Empty from path permitted */ +#define MOVE_MOUNT_T_SYMLINKS 0x00000010 /* Follow symlinks on to path */ +#define MOVE_MOUNT_T_AUTOMOUNTS 0x00000020 /* Follow automounts on to path */ +#define MOVE_MOUNT_T_EMPTY_PATH 0x00000040 /* Empty to path permitted */ +#define MOVE_MOUNT__MASK 0x00000077 + #endif /* _UAPI_LINUX_MOUNT_H */ diff --git a/security/security.c b/security/security.c index 5759339319dc..6fe31b31dc74 100644 --- a/security/security.c +++ b/security/security.c @@ -476,6 +476,11 @@ int security_add_mnt_opt(const char *option, const char *val, int len, } EXPORT_SYMBOL(security_add_mnt_opt); +int security_move_mount(const struct path *from_path, const struct path *to_path) +{ + return call_int_hook(move_mount, 0, from_path, to_path); +} + int security_inode_alloc(struct inode *inode) { inode->i_security = NULL;