Message ID | 1451930639-94331-4-git-send-email-seth.forshee@canonical.com |
---|---|
State | Not Applicable |
Headers | show |
On Tue, Mar 15, 2016 at 03:09:00PM +0300, Pavel Tikhomirov wrote: > If in_userns returns false mnt_may_suid also returns false, and we > will reach second(removed) if-check only in case it does not trigger, > so remove it. We had a somewhat lengthy discussion previously where one of the conclusions was that we'd have that check in both places even though it's redundant. Iirc the reason was that though they're doing the same test they're doing so to answer different questions, so we should have the test in both places (or something along those lines). Thanks, Seth
On Tue, 15 Mar 2016, Seth Forshee wrote: > On Tue, Mar 15, 2016 at 03:09:00PM +0300, Pavel Tikhomirov wrote: > > If in_userns returns false mnt_may_suid also returns false, and we > > will reach second(removed) if-check only in case it does not trigger, > > so remove it. > > We had a somewhat lengthy discussion previously where one of the > conclusions was that we'd have that check in both places even though > it's redundant. Iirc the reason was that though they're doing the same > test they're doing so to answer different questions, so we should have > the test in both places (or something along those lines). A comment in the code might be useful here.
diff --git a/fs/exec.c b/fs/exec.c index b06623a9347f..ea7311d72cc3 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1295,7 +1295,7 @@ static void bprm_fill_uid(struct linux_binprm *bprm) bprm->cred->euid = current_euid(); bprm->cred->egid = current_egid(); - if (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID) + if (!mnt_may_suid(bprm->file->f_path.mnt)) return; if (task_no_new_privs(current)) diff --git a/fs/namespace.c b/fs/namespace.c index da70f7c4ece1..2101ce7b96ab 100644 --- a/fs/namespace.c +++ b/fs/namespace.c @@ -3276,6 +3276,19 @@ found: return visible; } +bool mnt_may_suid(struct vfsmount *mnt) +{ + /* + * Foreign mounts (accessed via fchdir or through /proc + * symlinks) are always treated as if they are nosuid. This + * prevents namespaces from trusting potentially unsafe + * suid/sgid bits, file caps, or security labels that originate + * in other namespaces. + */ + return !(mnt->mnt_flags & MNT_NOSUID) && check_mnt(real_mount(mnt)) && + in_userns(current_user_ns(), mnt->mnt_sb->s_user_ns); +} + static struct ns_common *mntns_get(struct task_struct *task) { struct ns_common *ns = NULL; diff --git a/include/linux/mount.h b/include/linux/mount.h index f822c3c11377..54a594d49733 100644 --- a/include/linux/mount.h +++ b/include/linux/mount.h @@ -81,6 +81,7 @@ extern void mntput(struct vfsmount *mnt); extern struct vfsmount *mntget(struct vfsmount *mnt); extern struct vfsmount *mnt_clone_internal(struct path *path); extern int __mnt_is_readonly(struct vfsmount *mnt); +extern bool mnt_may_suid(struct vfsmount *mnt); struct path; extern struct vfsmount *clone_private_mount(struct path *path); diff --git a/security/commoncap.c b/security/commoncap.c index 400aa224b491..6243aef5860e 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -448,7 +448,7 @@ static int get_file_caps(struct linux_binprm *bprm, bool *effective, bool *has_c if (!file_caps_enabled) return 0; - if (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID) + if (!mnt_may_suid(bprm->file->f_path.mnt)) return 0; if (!in_userns(current_user_ns(), bprm->file->f_path.mnt->mnt_sb->s_user_ns)) return 0; diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index d0cfaa9f19d0..a5b93df6553f 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -2171,7 +2171,7 @@ static int check_nnp_nosuid(const struct linux_binprm *bprm, const struct task_security_struct *new_tsec) { int nnp = (bprm->unsafe & LSM_UNSAFE_NO_NEW_PRIVS); - int nosuid = (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID); + int nosuid = !mnt_may_suid(bprm->file->f_path.mnt); int rc; if (!nnp && !nosuid)