| Message ID | 20220518081227.1278192-1-chengzhihao1@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [-next] exec: Remove redundant check in do_open_execat/uselib | expand |
On Wed, 18 May 2022 16:12:27 +0800 Zhihao Cheng <chengzhihao1@huawei.com> wrote: > There is a false positive WARNON happening in execve(2)/uselib(2) > syscalls with concurrent noexec-remount. > > execveat remount > do_open_execat(path/bin) > do_filp_open > path_openat > do_open > may_open > path_noexec() // PASS > remount(path->mnt, MS_NOEXEC) > WARNON(path_noexec(&file->f_path)) // path_noexec() checks fail You're saying this is a race condition? A concurrent remount causes this warning? > Since may_open() has already checked the same conditions, fix it by > removing 'S_ISREG' and 'path_noexec' check in do_open_execat()/uselib(2). > > ... > > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -141,16 +141,6 @@ SYSCALL_DEFINE1(uselib, const char __user *, library) > if (IS_ERR(file)) > goto out; > > - /* > - * may_open() has already checked for this, so it should be > - * impossible to trip now. But we need to be extra cautious > - * and check again at the very end too. > - */ > - error = -EACCES; > - if (WARN_ON_ONCE(!S_ISREG(file_inode(file)->i_mode) || > - path_noexec(&file->f_path))) > - goto exit; > - Maybe we should retain the `goto exit'. The remount has now occurred, so the execution attempt should be denied. If so, the comment should be updated to better explain what's happening. I guess we'd still be racy against `mount -o exec', but accidentally denying something seems less serious than accidentally permitting it.
On Wed, May 18, 2022 at 10:46:01AM -0700, Andrew Morton wrote: > On Wed, 18 May 2022 16:12:27 +0800 Zhihao Cheng <chengzhihao1@huawei.com> wrote: > > > There is a false positive WARNON happening in execve(2)/uselib(2) > > syscalls with concurrent noexec-remount. > > > > execveat remount > > do_open_execat(path/bin) > > do_filp_open > > path_openat > > do_open > > may_open > > path_noexec() // PASS > > remount(path->mnt, MS_NOEXEC) > > WARNON(path_noexec(&file->f_path)) // path_noexec() checks fail Did you encounter this in the real world? > > You're saying this is a race condition? A concurrent remount causes > this warning? It seems not an unreasonable thing to warn about. Perhaps since it's technically reachable from userspace, it could be downgraded to pr_warn(), but I certainly don't want to remove the checks. > > > Since may_open() has already checked the same conditions, fix it by > > removing 'S_ISREG' and 'path_noexec' check in do_open_execat()/uselib(2). > > > > ... > > > > --- a/fs/exec.c > > +++ b/fs/exec.c > > @@ -141,16 +141,6 @@ SYSCALL_DEFINE1(uselib, const char __user *, library) > > if (IS_ERR(file)) > > goto out; > > > > - /* > > - * may_open() has already checked for this, so it should be > > - * impossible to trip now. But we need to be extra cautious > > - * and check again at the very end too. > > - */ > > - error = -EACCES; > > - if (WARN_ON_ONCE(!S_ISREG(file_inode(file)->i_mode) || > > - path_noexec(&file->f_path))) > > - goto exit; > > - > > Maybe we should retain the `goto exit'. The remount has now occurred, > so the execution attempt should be denied. If so, the comment should > be updated to better explain what's happening. > > I guess we'd still be racy against `mount -o exec', but accidentally > denying something seems less serious than accidentally permitting it. I'd like to leave this as-is, since we _do_ want to find the cases where we're about to allow an exec and a very important security check was NOT handled.
On Wed, 18 May 2022 12:17:45 -0700 Kees Cook <keescook@chromium.org> wrote: > > > - /* > > > - * may_open() has already checked for this, so it should be > > > - * impossible to trip now. But we need to be extra cautious > > > - * and check again at the very end too. > > > - */ > > > - error = -EACCES; > > > - if (WARN_ON_ONCE(!S_ISREG(file_inode(file)->i_mode) || > > > - path_noexec(&file->f_path))) > > > - goto exit; > > > - > > > > Maybe we should retain the `goto exit'. The remount has now occurred, > > so the execution attempt should be denied. If so, the comment should > > be updated to better explain what's happening. > > > > I guess we'd still be racy against `mount -o exec', but accidentally > > denying something seems less serious than accidentally permitting it. > > I'd like to leave this as-is, since we _do_ want to find the cases where > we're about to allow an exec and a very important security check was NOT > handled. In which case we don't want the "_ONCE". If some app is hammering away at this trying to hit a race window then the operator wants that log flood. Or,umm, fix the dang race?
在 2022/5/19 3:17, Kees Cook 写道: >>> WARNON(path_noexec(&file->f_path)) // path_noexec() checks fail > > Did you encounter this in the real world? I found the problem by running fuzz test.(syzkaller) Here is a brief reproducer. 1. Apply diff 2. Complie and run repo.c diff diff --git a/fs/exec.c b/fs/exec.c index e3e55d5e0be1..388d38b87e9a 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -897,6 +897,7 @@ EXPORT_SYMBOL(transfer_args_to_stack); #endif /* CONFIG_MMU */ +#include <linux/delay.h> static struct file *do_open_execat(int fd, struct filename *name, int flags) { struct file *file; @@ -925,9 +926,15 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags) * and check again at the very end too. */ err = -EACCES; + if (!strcmp(file->f_path.dentry->d_iname, "my_bin")) { + pr_err("wait ...\n"); + msleep(3000); + } if (WARN_ON_ONCE(!S_ISREG(file_inode(file)->i_mode) || - path_noexec(&file->f_path))) + path_noexec(&file->f_path))) { + pr_err("exec %pd %d %d %s\n", file->f_path.dentry, file->f_path.mnt->mnt_flags & MNT_NOEXEC, file->f_path.mnt->mnt_sb->s_iflags & SB_I_NOEXEC, file->f_path.mnt->mnt_sb->s_type->name); goto exit; + } err = deny_write_access(file); if (err) repo.c int main(void) { int ret; system("umount temp 2>&1 > /dev/null"); system("mount -t tmpfs none temp"); system("echo 12312 > temp/my_bin && chmod +x temp/my_bin"); ret = fork(); if (ret < 0) { perror("fork fail"); return 0; } if (ret == 0) { system("mount -oremount,noexec temp"); exit(0); } else { execve("/root/temp/my_bin", NULL, 0); //syscall(__NR_uselib, "/root/temp/my_bin"); } return 0; } > >> >> You're saying this is a race condition? A concurrent remount causes >> this warning? > > It seems not an unreasonable thing to warn about. Perhaps since it's > technically reachable from userspace, it could be downgraded to > pr_warn(), but I certainly don't want to remove the checks. > > I'd like to leave this as-is, since we _do_ want to find the cases where > we're about to allow an exec and a very important security check was NOT > handled. >I think removing redundant checking is okay, do_open_execat/uselib has initialized the acc_mode and open_flag for exec file, the check is equivalent to check in may_open(). Remount(noexec) operations can alos happen after the latest check, double check has no means for the concurrent situation. The MNT_NOEXEC flag only affects the open operation, it won't cause any problems that an opened bin file is executing in a non-exec mounted filesystem.
diff --git a/fs/exec.c b/fs/exec.c index e3e55d5e0be1..0f8ea7e9e03c 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -141,16 +141,6 @@ SYSCALL_DEFINE1(uselib, const char __user *, library) if (IS_ERR(file)) goto out; - /* - * may_open() has already checked for this, so it should be - * impossible to trip now. But we need to be extra cautious - * and check again at the very end too. - */ - error = -EACCES; - if (WARN_ON_ONCE(!S_ISREG(file_inode(file)->i_mode) || - path_noexec(&file->f_path))) - goto exit; - fsnotify_open(file); error = -ENOEXEC; @@ -169,7 +159,7 @@ SYSCALL_DEFINE1(uselib, const char __user *, library) break; } read_unlock(&binfmt_lock); -exit: + fput(file); out: return error; @@ -919,16 +909,6 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags) if (IS_ERR(file)) goto out; - /* - * may_open() has already checked for this, so it should be - * impossible to trip now. But we need to be extra cautious - * and check again at the very end too. - */ - err = -EACCES; - if (WARN_ON_ONCE(!S_ISREG(file_inode(file)->i_mode) || - path_noexec(&file->f_path))) - goto exit; - err = deny_write_access(file); if (err) goto exit;
There is a false positive WARNON happening in execve(2)/uselib(2) syscalls with concurrent noexec-remount. execveat remount do_open_execat(path/bin) do_filp_open path_openat do_open may_open path_noexec() // PASS remount(path->mnt, MS_NOEXEC) WARNON(path_noexec(&file->f_path)) // path_noexec() checks fail Since may_open() has already checked the same conditions, fix it by removing 'S_ISREG' and 'path_noexec' check in do_open_execat()/uselib(2). Fixes: 0fd338b2d2cdf8 ("exec: move path_noexec() check earlier") Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com> --- fs/exec.c | 22 +--------------------- 1 file changed, 1 insertion(+), 21 deletions(-)