Message-ID: <20040903112133.A1834@castle.nmd.msu.ru>
Date: Fri, 3 Sep 2004 11:21:33 +0400
From: Andrey Savochkin <saw@saw.sw.com.sg>
To: Chris Wright <chrisw@osdl.org>
Cc: Alexander Viro <viro@parcelfarce.linux.theplanet.co.uk>,
       Linus Torvalds <torvalds@osdl.org>, linux-kernel@vger.kernel.org
Subject: Re: exec: atomic MAY_EXEC check and SUID/SGID handling
References: <20040902174521.A13656@castle.nmd.msu.ru> <20040902133109.H1973@build.pdx.osdl.net>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
In-Reply-To: <20040902133109.H1973@build.pdx.osdl.net>; from "Chris Wright" on Thu, Sep 02, 2004 at 01:31:09PM
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2439
Lines: 54

On Thu, Sep 02, 2004 at 01:31:09PM -0700, Chris Wright wrote:
> * Andrey Savochkin (saw@saw.sw.com.sg) wrote:
> > There is a time window between permission(MAY_EXEC) check in
> > open_exec() and S_ISUID check plus bprm->e_uid setting in prepare_binprm().
> > And S_ISUID is checked and bprm->e_uid is copied from the inode without
> > any serialization with attribute updates.
> > 
> > That means that some executable may have permissions
> > -rwxr-xr-x    root     disk     /bin/file
> > at the moment of MAY_EXEC check and
> > -rwsr-x---    root     disk     /bin/file
> > at the moment of S_ISUID check, providing lucky users starting /bin/file at
> > the moment of permission change with a setuid-root program.
> > 
> > It's arguable whether it's a big security issue, but certainly such behavior
> > is not what administrators may expect.
> 
> If you can find a way for a user to exploit this it's an issue.  Looks

Exploiting it requires waiting for the administrator to change file
permissions...  May be, some social engineering.
But THERE IS a race, which may result in user having more permissions than
he is expected to have.
I'm not comfortable living with such a race.

Instead of
	inode->i_mode = attr->ia_mode;
we can write inode_setattr() as
	inode->i_mode |= 06777;
	inode->i_mode &= attr->ia_mode;

Will it be easily exploitable?  I guess, no.
Will I be comfortable if the code is vulnerable in this way?  No.

> like it's not, and doesn't warrant such a big change as your patch.
> The fact that you introduce a new field and then almost always supply it
> with NULL is a clue that it's not the right direction IMO.  Something
> simple (as you mentioned) that grabs i_sem and rechecks during suid
> setup in binprm_prepare is sufficient.  Worth it?  Guess I'm not
> convinced.

I explained my arguments against re-checking permissions:
 - the locking convention where ->permission() method may be called with or
   without i_sem doesn't look suberb;
 - it's better to avoid calling permission() with the same arguments for
   the second time, especially if it does something complicated in
   security_inode_permission(), with ACLs or in case of a remote filesystem.

	Andrey
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/