| Message ID | 1430138638-16537-1-git-send-email-luis.henriques@canonical.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Apr 27, 2015 at 01:43:58PM +0100, Luis Henriques wrote: > From: Jann Horn <jann@thejh.net> > > This prevents a race between chown() and execve(), where chowning a > setuid-user binary to root would momentarily make the binary setuid > root. > > This patch was mostly written by Linus Torvalds. > > Signed-off-by: Jann Horn <jann@thejh.net> > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> > (backported from commit 8b01fc86b9f425899f8a3a8fc1c47d73c2c20543) > [ luis: backport to Lucid: > - replaced kuid_t/kgid_t by uid_t/gid_t > - replaced READ_ONCE() by ACCESS_ONCE() > - replaced task_no_new_privs() by current->no_new_privs <snip> > +static void bprm_fill_uid(struct linux_binprm *bprm) > +{ > + struct inode *inode; > + unsigned int mode; > + uid_t uid; > + gid_t gid; > + > + /* clear any previous set[ug]id data from a previous binary */ > + bprm->cred->euid = current_euid(); > + bprm->cred->egid = current_egid(); > + > + if (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID) > + return; The current->no_new_privs bit is missing here, not sure if it's missing or if the commit log is wrong and that part isn't relevant for lucid. Otherwise the backport looks correct to me.
On Mon, Apr 27, 2015 at 08:46:03AM -0500, Seth Forshee wrote: > On Mon, Apr 27, 2015 at 01:43:58PM +0100, Luis Henriques wrote: > > From: Jann Horn <jann@thejh.net> > > > > This prevents a race between chown() and execve(), where chowning a > > setuid-user binary to root would momentarily make the binary setuid > > root. > > > > This patch was mostly written by Linus Torvalds. > > > > Signed-off-by: Jann Horn <jann@thejh.net> > > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> > > (backported from commit 8b01fc86b9f425899f8a3a8fc1c47d73c2c20543) > > [ luis: backport to Lucid: > > - replaced kuid_t/kgid_t by uid_t/gid_t > > - replaced READ_ONCE() by ACCESS_ONCE() > > - replaced task_no_new_privs() by current->no_new_privs > > <snip> > Doh! This is a copy&paste problem -- I should have removed this line from the Lucid backport. I'll do that when applying it to Lucid (let me know if you rather have me resubmitting with this fixed). > > +static void bprm_fill_uid(struct linux_binprm *bprm) > > +{ > > + struct inode *inode; > > + unsigned int mode; > > + uid_t uid; > > + gid_t gid; > > + > > + /* clear any previous set[ug]id data from a previous binary */ > > + bprm->cred->euid = current_euid(); > > + bprm->cred->egid = current_egid(); > > + > > + if (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID) > > + return; > > The current->no_new_privs bit is missing here, not sure if it's missing > or if the commit log is wrong and that part isn't relevant for lucid. > My understanding is that it is not relevant. If you look at the backport for Precise, the "current->no_new_privs" was dropped from the prepare_binprm() and moved into bprm_fill_uid(). Since Lucid doesn't have this check in prepare_binprm(), I haven't added it into the new function. Actually, struct task_struct in Lucid doesn't even have that field. Cheers, -- Luís > Otherwise the backport looks correct to me. >
On Mon, Apr 27, 2015 at 03:10:37PM +0100, Luis Henriques wrote: > On Mon, Apr 27, 2015 at 08:46:03AM -0500, Seth Forshee wrote: > > On Mon, Apr 27, 2015 at 01:43:58PM +0100, Luis Henriques wrote: > > > From: Jann Horn <jann@thejh.net> > > > > > > This prevents a race between chown() and execve(), where chowning a > > > setuid-user binary to root would momentarily make the binary setuid > > > root. > > > > > > This patch was mostly written by Linus Torvalds. > > > > > > Signed-off-by: Jann Horn <jann@thejh.net> > > > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> > > > (backported from commit 8b01fc86b9f425899f8a3a8fc1c47d73c2c20543) > > > [ luis: backport to Lucid: > > > - replaced kuid_t/kgid_t by uid_t/gid_t > > > - replaced READ_ONCE() by ACCESS_ONCE() > > > - replaced task_no_new_privs() by current->no_new_privs > > > > <snip> > > > > Doh! This is a copy&paste problem -- I should have removed this line > from the Lucid backport. I'll do that when applying it to Lucid (let > me know if you rather have me resubmitting with this fixed). > > > > +static void bprm_fill_uid(struct linux_binprm *bprm) > > > +{ > > > + struct inode *inode; > > > + unsigned int mode; > > > + uid_t uid; > > > + gid_t gid; > > > + > > > + /* clear any previous set[ug]id data from a previous binary */ > > > + bprm->cred->euid = current_euid(); > > > + bprm->cred->egid = current_egid(); > > > + > > > + if (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID) > > > + return; > > > > The current->no_new_privs bit is missing here, not sure if it's missing > > or if the commit log is wrong and that part isn't relevant for lucid. > > > > My understanding is that it is not relevant. If you look at the > backport for Precise, the "current->no_new_privs" was dropped from the > prepare_binprm() and moved into bprm_fill_uid(). Since Lucid doesn't > have this check in prepare_binprm(), I haven't added it into the new > function. Actually, struct task_struct in Lucid doesn't even have > that field. Well it will be awfully hard to check then :-) I'm cool with just fixing up the commit message when the patch is applied. Acked-by: Seth Forshee <seth.forshee@canonical.com>
On 04/27/2015 05:43 AM, Luis Henriques wrote: > From: Jann Horn <jann@thejh.net> > > This prevents a race between chown() and execve(), where chowning a > setuid-user binary to root would momentarily make the binary setuid > root. > > This patch was mostly written by Linus Torvalds. > > Signed-off-by: Jann Horn <jann@thejh.net> > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> > (backported from commit 8b01fc86b9f425899f8a3a8fc1c47d73c2c20543) > [ luis: backport to Lucid: > - replaced kuid_t/kgid_t by uid_t/gid_t > - replaced READ_ONCE() by ACCESS_ONCE() > - replaced task_no_new_privs() by current->no_new_privs > - replaced file_inode() by bprm->file->f_path.dentry->d_inode > - dropped user_ns bits and task_no_new_privs() ] > CVE-2015-3339 > BugLink: https://bugs.launchpad.net/bugs/1447373 > Signed-off-by: Luis Henriques <luis.henriques@canonical.com> looks good Acked-by: John Johansen <john.johansen@canonical.com > --- > fs/exec.c | 65 +++++++++++++++++++++++++++++++++++++++------------------------ > 1 file changed, 40 insertions(+), 25 deletions(-) > > diff --git a/fs/exec.c b/fs/exec.c > index c47c88892721..99670db65d4c 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -1185,6 +1185,45 @@ int check_unsafe_exec(struct linux_binprm *bprm) > return res; > } > > +static void bprm_fill_uid(struct linux_binprm *bprm) > +{ > + struct inode *inode; > + unsigned int mode; > + uid_t uid; > + gid_t gid; > + > + /* clear any previous set[ug]id data from a previous binary */ > + bprm->cred->euid = current_euid(); > + bprm->cred->egid = current_egid(); > + > + if (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID) > + return; > + > + inode = bprm->file->f_path.dentry->d_inode; > + mode = ACCESS_ONCE(inode->i_mode); > + if (!(mode & (S_ISUID|S_ISGID))) > + return; > + > + /* Be careful if suid/sgid is set */ > + mutex_lock(&inode->i_mutex); > + > + /* reload atomically mode/uid/gid now that lock held */ > + mode = inode->i_mode; > + uid = inode->i_uid; > + gid = inode->i_gid; > + mutex_unlock(&inode->i_mutex); > + > + if (mode & S_ISUID) { > + bprm->per_clear |= PER_CLEAR_ON_SETID; > + bprm->cred->euid = uid; > + } > + > + if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) { > + bprm->per_clear |= PER_CLEAR_ON_SETID; > + bprm->cred->egid = gid; > + } > +} > + > /* > * Fill the binprm structure from the inode. > * Check permissions, then read the first 128 (BINPRM_BUF_SIZE) bytes > @@ -1193,36 +1232,12 @@ int check_unsafe_exec(struct linux_binprm *bprm) > */ > int prepare_binprm(struct linux_binprm *bprm) > { > - umode_t mode; > - struct inode * inode = bprm->file->f_path.dentry->d_inode; > int retval; > > - mode = inode->i_mode; > if (bprm->file->f_op == NULL) > return -EACCES; > > - /* clear any previous set[ug]id data from a previous binary */ > - bprm->cred->euid = current_euid(); > - bprm->cred->egid = current_egid(); > - > - if (!(bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID)) { > - /* Set-uid? */ > - if (mode & S_ISUID) { > - bprm->per_clear |= PER_CLEAR_ON_SETID; > - bprm->cred->euid = inode->i_uid; > - } > - > - /* Set-gid? */ > - /* > - * If setgid is set but no group execute bit then this > - * is a candidate for mandatory locking, not a setgid > - * executable. > - */ > - if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) { > - bprm->per_clear |= PER_CLEAR_ON_SETID; > - bprm->cred->egid = inode->i_gid; > - } > - } > + bprm_fill_uid(bprm); > > /* fill in binprm security blob */ > retval = security_bprm_set_creds(bprm); >
On 04/27/2015 07:10 AM, Luis Henriques wrote: > On Mon, Apr 27, 2015 at 08:46:03AM -0500, Seth Forshee wrote: >> On Mon, Apr 27, 2015 at 01:43:58PM +0100, Luis Henriques wrote: >>> From: Jann Horn <jann@thejh.net> >>> >>> This prevents a race between chown() and execve(), where chowning a >>> setuid-user binary to root would momentarily make the binary setuid >>> root. >>> >>> This patch was mostly written by Linus Torvalds. >>> >>> Signed-off-by: Jann Horn <jann@thejh.net> >>> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> >>> (backported from commit 8b01fc86b9f425899f8a3a8fc1c47d73c2c20543) >>> [ luis: backport to Lucid: >>> - replaced kuid_t/kgid_t by uid_t/gid_t >>> - replaced READ_ONCE() by ACCESS_ONCE() >>> - replaced task_no_new_privs() by current->no_new_privs >> >> <snip> >> > > Doh! This is a copy&paste problem -- I should have removed this line > from the Lucid backport. I'll do that when applying it to Lucid (let > me know if you rather have me resubmitting with this fixed). > >>> +static void bprm_fill_uid(struct linux_binprm *bprm) >>> +{ >>> + struct inode *inode; >>> + unsigned int mode; >>> + uid_t uid; >>> + gid_t gid; >>> + >>> + /* clear any previous set[ug]id data from a previous binary */ >>> + bprm->cred->euid = current_euid(); >>> + bprm->cred->egid = current_egid(); >>> + >>> + if (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID) >>> + return; >> >> The current->no_new_privs bit is missing here, not sure if it's missing >> or if the commit log is wrong and that part isn't relevant for lucid. >> > > My understanding is that it is not relevant. If you look at the > backport for Precise, the "current->no_new_privs" was dropped from the > prepare_binprm() and moved into bprm_fill_uid(). Since Lucid doesn't > have this check in prepare_binprm(), I haven't added it into the new > function. Actually, struct task_struct in Lucid doesn't even have > that field. > right no_new_privs got added in 3.5 (commit 259e5e6c)
Applied to Lucid master-next, with the commit message edited (removed "replaced task_no_new_privs() by current->no_new_privs"). Cheers, -- Luís
Applied to all the series. Cheers, -- Luís
diff --git a/fs/exec.c b/fs/exec.c index c47c88892721..99670db65d4c 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1185,6 +1185,45 @@ int check_unsafe_exec(struct linux_binprm *bprm) return res; } +static void bprm_fill_uid(struct linux_binprm *bprm) +{ + struct inode *inode; + unsigned int mode; + uid_t uid; + gid_t gid; + + /* clear any previous set[ug]id data from a previous binary */ + bprm->cred->euid = current_euid(); + bprm->cred->egid = current_egid(); + + if (bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID) + return; + + inode = bprm->file->f_path.dentry->d_inode; + mode = ACCESS_ONCE(inode->i_mode); + if (!(mode & (S_ISUID|S_ISGID))) + return; + + /* Be careful if suid/sgid is set */ + mutex_lock(&inode->i_mutex); + + /* reload atomically mode/uid/gid now that lock held */ + mode = inode->i_mode; + uid = inode->i_uid; + gid = inode->i_gid; + mutex_unlock(&inode->i_mutex); + + if (mode & S_ISUID) { + bprm->per_clear |= PER_CLEAR_ON_SETID; + bprm->cred->euid = uid; + } + + if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) { + bprm->per_clear |= PER_CLEAR_ON_SETID; + bprm->cred->egid = gid; + } +} + /* * Fill the binprm structure from the inode. * Check permissions, then read the first 128 (BINPRM_BUF_SIZE) bytes @@ -1193,36 +1232,12 @@ int check_unsafe_exec(struct linux_binprm *bprm) */ int prepare_binprm(struct linux_binprm *bprm) { - umode_t mode; - struct inode * inode = bprm->file->f_path.dentry->d_inode; int retval; - mode = inode->i_mode; if (bprm->file->f_op == NULL) return -EACCES; - /* clear any previous set[ug]id data from a previous binary */ - bprm->cred->euid = current_euid(); - bprm->cred->egid = current_egid(); - - if (!(bprm->file->f_path.mnt->mnt_flags & MNT_NOSUID)) { - /* Set-uid? */ - if (mode & S_ISUID) { - bprm->per_clear |= PER_CLEAR_ON_SETID; - bprm->cred->euid = inode->i_uid; - } - - /* Set-gid? */ - /* - * If setgid is set but no group execute bit then this - * is a candidate for mandatory locking, not a setgid - * executable. - */ - if ((mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP)) { - bprm->per_clear |= PER_CLEAR_ON_SETID; - bprm->cred->egid = inode->i_gid; - } - } + bprm_fill_uid(bprm); /* fill in binprm security blob */ retval = security_bprm_set_creds(bprm);