Messages in this thread | | | From | Andy Lutomirski <> | Date | Wed, 25 Jan 2017 13:44:09 -0800 | Subject | Re: [PATCH 2/2] fs: Harden against open(..., O_CREAT, 02777) in a setgid directory |
| |
On Wed, Jan 25, 2017 at 1:31 PM, Ben Hutchings <ben@decadent.org.uk> wrote: > On Wed, 2017-01-25 at 13:06 -0800, Andy Lutomirski wrote: >> Currently, if you open("foo", O_WRONLY | O_CREAT | ..., 02777) in a >> directory that is setgid and owned by a different gid than current's >> fsgid, you end up with an SGID executable that is owned by the >> directory's GID. This is a Bad Thing (tm). Exploiting this is >> nontrivial because most ways of creating a new file create an empty >> file and empty executables aren't particularly interesting, but this >> is nevertheless quite dangerous. >> >> Harden against this type of attack by detecting this particular >> corner case (unprivileged program creates SGID executable inode in >> SGID directory owned by a different GID) and clearing the new >> inode's SGID bit. >> >> > Signed-off-by: Andy Lutomirski <luto@kernel.org> >> --- >> fs/inode.c | 21 +++++++++++++++++++-- >> 1 file changed, 19 insertions(+), 2 deletions(-) >> >> diff --git a/fs/inode.c b/fs/inode.c >> index f7029c40cfbd..d7e4b80470dd 100644 >> --- a/fs/inode.c >> +++ b/fs/inode.c >> @@ -2007,11 +2007,28 @@ void inode_init_owner(struct inode *inode, const struct inode *dir, >> { >> inode->i_uid = current_fsuid(); >> if (dir && dir->i_mode & S_ISGID) { >> + bool changing_gid = !gid_eq(inode->i_gid, dir->i_gid); > [...] > > inode->i_gid hasn't been initialised yet. This should compare with > current_fsgid(), shouldn't it?
Whoops. In v2, I'll fix it by inode->i_gid first -- that'll simplify the control flow.
> > Ben. > > -- > Ben Hutchings > It is easier to write an incorrect program than to understand a correct > one. >
-- Andy Lutomirski AMA Capital Management, LLC
| |