| Message ID | 20210525193735.2716374-1-keescook@chromium.org |
|---|---|
| State | Accepted |
| Commit | bfb819ea20ce8bbeeba17e1a6418bf8bda91fc28 |
| Headers | show |
| Series | proc: Check /proc/$pid/attr/ writes against file opener | expand |
On Tue, May 25, 2021 at 9:37 PM Kees Cook <keescook@chromium.org> wrote: > Fix another "confused deputy" weakness[1]. Writes to /proc/$pid/attr/ > files need to check the opener credentials, since these fds do not > transition state across execve(). Without this, it is possible to > trick another process (which may have different credentials) to write > to its own /proc/$pid/attr/ files, leading to unexpected and possibly > exploitable behaviors. > > [1] https://www.kernel.org/doc/html/latest/security/credentials.html?highlight=confused#open-file-credentials > > Fixes: 1da177e4c3f41 ("Linux-2.6.12-rc2") > Cc: stable@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > fs/proc/base.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/fs/proc/base.c b/fs/proc/base.c > index 3851bfcdba56..58bbf334265b 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -2703,6 +2703,10 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf, > void *page; > int rv; > > + /* A task may only write when it was the opener. */ > + if (file->f_cred != current_real_cred()) > + return -EPERM; With this, if a task forks, the child will then still be able to open its parent's /proc/$pid/attr/current and trick the parent into writing to that, right? Is that acceptable? If not, the ->open handler should probably also check for "current->thread_pid == proc_pid(inode)", or something like that?
Jann Horn <jannh@google.com> writes: > On Tue, May 25, 2021 at 9:37 PM Kees Cook <keescook@chromium.org> wrote: >> Fix another "confused deputy" weakness[1]. Writes to /proc/$pid/attr/ >> files need to check the opener credentials, since these fds do not >> transition state across execve(). Without this, it is possible to >> trick another process (which may have different credentials) to write >> to its own /proc/$pid/attr/ files, leading to unexpected and possibly >> exploitable behaviors. >> >> [1] https://www.kernel.org/doc/html/latest/security/credentials.html?highlight=confused#open-file-credentials >> >> Fixes: 1da177e4c3f41 ("Linux-2.6.12-rc2") >> Cc: stable@vger.kernel.org >> Signed-off-by: Kees Cook <keescook@chromium.org> >> --- >> fs/proc/base.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/fs/proc/base.c b/fs/proc/base.c >> index 3851bfcdba56..58bbf334265b 100644 >> --- a/fs/proc/base.c >> +++ b/fs/proc/base.c >> @@ -2703,6 +2703,10 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf, >> void *page; >> int rv; >> >> + /* A task may only write when it was the opener. */ >> + if (file->f_cred != current_real_cred()) >> + return -EPERM; > > With this, if a task forks, the child will then still be able to open > its parent's /proc/$pid/attr/current and trick the parent into writing > to that, right? Is that acceptable? If not, the ->open handler should > probably also check for "current->thread_pid == proc_pid(inode)", or > something like that? Currently exec always allocates a new cred. So you can only ``trick'' another process that was forked from you. I don't think it counts as tricking or any kind of danger if you are simply confusing yourself. Eric
diff --git a/fs/proc/base.c b/fs/proc/base.c index 3851bfcdba56..58bbf334265b 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -2703,6 +2703,10 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf, void *page; int rv; + /* A task may only write when it was the opener. */ + if (file->f_cred != current_real_cred()) + return -EPERM; + rcu_read_lock(); task = pid_task(proc_pid(inode), PIDTYPE_PID); if (!task) {
Fix another "confused deputy" weakness[1]. Writes to /proc/$pid/attr/ files need to check the opener credentials, since these fds do not transition state across execve(). Without this, it is possible to trick another process (which may have different credentials) to write to its own /proc/$pid/attr/ files, leading to unexpected and possibly exploitable behaviors. [1] https://www.kernel.org/doc/html/latest/security/credentials.html?highlight=confused#open-file-credentials Fixes: 1da177e4c3f41 ("Linux-2.6.12-rc2") Cc: stable@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- fs/proc/base.c | 4 ++++ 1 file changed, 4 insertions(+)