Messages in this thread | | | Date | Fri, 3 Sep 2004 11:21:33 +0400 | From | Andrey Savochkin <> | Subject | Re: exec: atomic MAY_EXEC check and SUID/SGID handling |
| |
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/
| |