First, the security hole (this is bigger than the chown race condition):
On some clients of a Linux NFS server, it is possible for any user to chgrp
a file s/he doesn't own. This happens because Linux does not check to
see if a changer of a file's gid owns the file. Linux's chown always
claims to be changing the uid, too, which causes an ownership check, so
only another OS can trigger this bug (I used NetBSD).
The attached patch's changes to fs/attr.c should fix this (although they
do other things as well).
On Tue, Jan 12, 1999 at 08:22:43PM -0500, David C Niemi wrote:
% There is also the principle of "least surprise", which argues against
% making implicit and nonstandard actions that the user did not ask for. If
% we clear that bit we are going against standard UNIX practice, so far
% verified on SunOS 4, Solaris 2, AIX, IRIX, and even SCO. That's not to say
% we shouldn't do it, but we had better have a very compelling reason to be
% that nonstandard.
I've verified that NetBSD (1.3.2) clears the set-id bits on a chown in
all circumstances. I wouldn't be too surprised if the other BSDs did this
as well.
% On Tue, 12 Jan 1999, Mark Jefferys wrote:
% > One could make the exact opposite argument: Since root can chmod the file
% > afterwards, there is no need to preserve the bits; root can always restore
% > them if that's what it really wants.
%
% I can buy this in the case of non-root files chown'd to root; though not in
% the case of setgid files being chown'd or setuid files being chgrp'd or
I had written the following:
"I agree with these two. Clearing the SETGID bit for a uid change or
clearing the SETUID bit for a gid change is silly."
Then I tried to implement some of this stuff and found that matters are
far more complicated. There are *four* different ways changing a file's
mode/uid/gid can potentially violate security:
#1: Setting the S_ISUID/S_ISGID bit for a uid/gid to which one does not
belong.
#2: Changing the uid/gid so that the S_ISUID/S_ISGID bit applies to a
different user/group.
#3: Setting an execute bit expanding the rights of users to execute
privileged code.
#4: Changing the uid/gid so that the execute bit applies to a different
user/group, thus giving new rights to execute privileged code.
Any of these situations requires re-validating the set-id bits to make sure
the changer has the rights to set them.
Also, see below for some comments on POSIX and The Single UNIX Specification,
Version 2.
It turns out that inode-attribute-change code is pretty screwed up.
In addition to the security hole I mentioned above, there are several
capability-specific bugs in the code. Non-FSETID-capable users could
set the S_ISUID bit on a file they don't own, chown a file without
loosing the set-id bits, or any of the other issues in #1-4. It's also
not careful about preserving the S_ISGID bit for directories.
Trying to check for precisely these circumstances was making a complete
mess of the inode_check_ok() code (even for just #1 and #2), so I gave up
on it and switched to a simpler "re-validate set-id bits when the uid,
gid, or file mode changes" rule.
% setuid nonroot files being chown'd to another nonroot user.
I disagree:
#1: There are accounts other than root that are sensitive (e.g. bin,
daemon, mail, operator, ftp, qmail accounts, etc.); they need
protection, too.
#2: In a capabilities world, no account is more special than any other,
as far as the kernel is concerned. Even root isn't special.
% > In any case there is a security problem: When chowning any file there is
% > a race condition where the owner sets the suid bit just before the chown
% > (so chmoding it first doesn't help) and executes the file just afterwards
% > (so chmoding it after doesn't, either). This means that root chowning
% > *any* file while the old owner is running a process exposes the new owner
% > to a break-in by the old. I don't see any way around this except crossing
% > one's fingers and hoping the old owner doesn't luck out and get the timing
% > right.
%
% This is true, but this race condition might exist even if the clearing of
% the suid bit is done in the kernel, it requires checking on what the lower
% levels of chown/chmod do for locking. This might even be
% file-system-specific.
This actually looks pretty good. The inode updates for local file systems
seem to be done only when holding the global kernel lock, as far as I can
tell. (But I could have missed a spot, since the lock is held by whomever
started the change: e.g. chown, chmod.) So they should be OK.
If a networked file system's network protocol doesn't support changing a
file's ids and mode simultaneously, then clearly there isn't much Linux
can do about it. But then what does one expect when using weird protocols
like that? Interestingly, NFS seems to do the changes atomically (at least
from my rather cursory look at the code).
There are some POSIX/SUSv2 issues of concern as well. Linux's current
behavior doesn't quite conform to SUSv2, and the proposed change of only
clearing S_ISUID bit for uid changes (and the GUID bit for gid changes)
conflicts even more. (I assume some of these are POSIX concerns; I only
have access to the SUSv2 specs on the web.)
SUSv2 has no place for Linux's mandatory locking setting: it is clear
that the S_ISGID bit must mean "set the egid" when executed, and must be
cleared in a number of circumstances. Personally, I don't like
overloading the bit like that, but I'll limit myself to pointing out
the problem. (My patch actually makes this a little worse.)
SUSv2 also insists that changing the user or group must clear *both*
set-id bits for unprivileged (for us this should be non-FSETID-capable)
users. Personally, I don't see how anyone would end up depending on
this behavior, and I don't have any inclination towards following
standards when they do silly things. OTOH, it's not *that* silly to
clear both bits. My patch currently clears only the needed bits,
although I think it might be best to ignore those changes (fs/open.c)
and kill the `&& current->fsuid' tests in chown_common() instead.
I'm getting to really like the idea of a chownmod command (four
arguments: file, uid, gid, mode; change uid/gid/mode simultaneously).
It would nicely bypass the question of what to do with the set-id bits.
Here's a run-down of the patch (it applies to 2.2.0-pre7 and -pre8):
fs/attr.c:
Completely reworked inode_change_ok(). I've tested that this does
the right thing for root and users, but haven't properly tested
it for how it acts with piecemeal capabilities. This needs a
really good eye-balling to make sure the logic is right.
Killed the extraneous check for the S_ISGID bit in inode_setattr().
inode_change_ok() already does this and better.
fs/open.c:
Changed chown_common() to only clear the S_IS[UG]ID bit when the
respective [ug]id is changed. Might not be a good idea from a
standards-conformance point-of-view.
fs/ext2/inode.c:
ext2_notify_change() (currently unused) didn't check to see if
the ia_attr_flags were valid before using them. Testing is
irrelevant.
fs/fat/inode.c:
fat_notify_change() was changing the inode without dirtying it.
Very poorly tested. Be wary.
fs/affs/inode.c:
Eliminated duplicate dirtying of the inode in affs_notify_change().
Completely untested, but trivial.
Mark
--LQksG6bCIzRHxTLp
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="fs-attr.patch"
diff -ur linux.bak/fs/affs/inode.c linux/fs/affs/inode.c
--- linux.bak/fs/affs/inode.c Wed Dec 16 12:17:21 1998
+++ linux/fs/affs/inode.c Tue Jan 19 14:35:10 1999
@@ -247,7 +247,6 @@
inode->u.affs_i.i_protect = mode_to_prot(attr->ia_mode);
inode_setattr(inode, attr);
- mark_inode_dirty(inode);
error = 0;
out:
return error;
diff -ur linux.bak/fs/attr.c linux/fs/attr.c
--- linux.bak/fs/attr.c Fri Nov 13 10:07:26 1998
+++ linux/fs/attr.c Mon Jan 18 19:52:36 1999
@@ -3,6 +3,7 @@
*
* Copyright (C) 1991, 1992 Linus Torvalds
* changes by Thomas Schoebel-Theuer
+ * more changes by Mark Jefferys
*/
#include <linux/sched.h>
@@ -11,43 +12,91 @@
/* Taken over from the old code... */
-/* POSIX UID/GID verification for setting inode attributes. */
+/* POSIX UID/GID verification for setting inode attributes.
+ *
+ * NB: This may change the contents of attr.
+ */
int inode_change_ok(struct inode *inode, struct iattr *attr)
{
int retval = -EPERM;
unsigned int ia_valid = attr->ia_valid;
+ umode_t new_mode;
+ int check_setid;
/* If force is set do it anyway. */
if (ia_valid & ATTR_FORCE)
goto fine;
+ /* Check for setting the inode time. */
+ if (ia_valid & (ATTR_MTIME_SET | ATTR_ATIME_SET)) {
+ if (current->fsuid != inode->i_uid && !capable(CAP_FOWNER))
+ goto error;
+ }
+
+ /* Determine final mode and determine if we need to recheck
+ * the setting of set-id bits (i.e. because the mode is being
+ * changed).
+ */
+ new_mode = inode->i_mode;
+ check_setid = 0;
+ if (ia_valid & ATTR_MODE) {
+ check_setid = (inode->i_mode != attr->ia_mode);
+ new_mode = attr->ia_mode;
+ }
+
/* Make sure a caller can chown. */
- if ((ia_valid & ATTR_UID) &&
- (current->fsuid != inode->i_uid ||
- attr->ia_uid != inode->i_uid) && !capable(CAP_CHOWN))
- goto error;
+ if ((ia_valid & ATTR_UID) && (attr->ia_uid != inode->i_uid)) {
+ if (!capable(CAP_CHOWN))
+ goto error;
+
+ check_setid = 1;
+ }
/* Make sure caller can chgrp. */
- if ((ia_valid & ATTR_GID) &&
- (!in_group_p(attr->ia_gid) && attr->ia_gid != inode->i_gid) &&
- !capable(CAP_CHOWN))
- goto error;
+ if ((ia_valid & ATTR_GID) && (attr->ia_gid != inode->i_gid)) {
+ /* Don't need to check if the new uid also matches.
+ * If it is different, we must have CAP_CHOWN to have
+ * gotten here. */
+ if (((current->fsuid != inode->i_uid) ||
+ !in_group_p(attr->ia_gid)) &&
+ !capable(CAP_CHOWN))
- /* Make sure a caller can chmod. */
- if (ia_valid & ATTR_MODE) {
- if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
goto error;
- /* Also check the setgid bit! */
- if (!in_group_p((ia_valid & ATTR_GID) ? attr->ia_gid :
- inode->i_gid) && !capable(CAP_FSETID))
- attr->ia_mode &= ~S_ISGID;
+
+ check_setid = 1;
}
- /* Check for setting the inode time. */
- if (ia_valid & (ATTR_MTIME_SET | ATTR_ATIME_SET)) {
- if (current->fsuid != inode->i_uid && !capable(CAP_FOWNER))
+ /* Check the set-id bits if necessary. */
+ if (check_setid) {
+ if ((new_mode & S_ISUID) &&
+ (((ia_valid & ATTR_UID) ? attr->ia_uid : inode->i_uid)
+ != current->fsuid) &&
+ !S_ISDIR(new_mode) && !capable(CAP_FSETID))
+
+ new_mode &= ~S_ISUID;
+
+ if ((new_mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP) &&
+ !in_group_p((ia_valid & ATTR_GID) ? attr->ia_gid :
+ inode->i_gid) &&
+ !S_ISDIR(new_mode) && !capable(CAP_FSETID))
+
+ new_mode &= ~S_ISGID;
+ }
+
+ /* Make sure a caller can chmod. */
+ if (new_mode != inode->i_mode) {
+ if ((current->fsuid != inode->i_uid) &&
+ (!(ia_valid & ATTR_UID) ||
+ (current->fsuid != attr->ia_uid)) &&
+ !capable(CAP_FOWNER))
+
goto error;
}
+
+ /* Save the new mode only when successful. */
+ if (new_mode != inode->i_mode)
+ attr->ia_valid |= ATTR_MODE;
+ attr->ia_mode = new_mode; /* In case ATTR_MODE already set. */
fine:
retval = 0;
error:
@@ -70,11 +119,8 @@
inode->i_mtime = attr->ia_mtime;
if (ia_valid & ATTR_CTIME)
inode->i_ctime = attr->ia_ctime;
- if (ia_valid & ATTR_MODE) {
+ if (ia_valid & ATTR_MODE)
inode->i_mode = attr->ia_mode;
- if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
- inode->i_mode &= ~S_ISGID;
- }
mark_inode_dirty(inode);
}
diff -ur linux.bak/fs/ext2/inode.c linux/fs/ext2/inode.c
--- linux.bak/fs/ext2/inode.c Wed May 6 10:56:05 1998
+++ linux/fs/ext2/inode.c Tue Jan 19 14:40:44 1999
@@ -722,51 +722,54 @@
unsigned int flags;
retval = -EPERM;
- if ((iattr->ia_attr_flags &
- (ATTR_FLAG_APPEND | ATTR_FLAG_IMMUTABLE)) ^
- (inode->u.ext2_i.i_flags &
- (EXT2_APPEND_FL | EXT2_IMMUTABLE_FL))) {
- if (!capable(CAP_LINUX_IMMUTABLE))
+ if (iattr->ia_valid & ATTR_ATTR_FLAG) {
+ if ((iattr->ia_attr_flags &
+ (ATTR_FLAG_APPEND | ATTR_FLAG_IMMUTABLE)) ^
+ (inode->u.ext2_i.i_flags &
+ (EXT2_APPEND_FL | EXT2_IMMUTABLE_FL))) {
+ if (!capable(CAP_LINUX_IMMUTABLE))
+ goto out;
+ } else if ((current->fsuid != inode->i_uid) &&
+ !capable(CAP_FOWNER))
goto out;
- } else if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
- goto out;
+ }
retval = inode_change_ok(inode, iattr);
if (retval != 0)
goto out;
- inode_setattr(inode, iattr);
-
- flags = iattr->ia_attr_flags;
- if (flags & ATTR_FLAG_SYNCRONOUS) {
- inode->i_flags |= MS_SYNCHRONOUS;
- inode->u.ext2_i.i_flags = EXT2_SYNC_FL;
- } else {
- inode->i_flags &= ~MS_SYNCHRONOUS;
- inode->u.ext2_i.i_flags &= ~EXT2_SYNC_FL;
- }
- if (flags & ATTR_FLAG_NOATIME) {
- inode->i_flags |= MS_NOATIME;
- inode->u.ext2_i.i_flags = EXT2_NOATIME_FL;
- } else {
- inode->i_flags &= ~MS_NOATIME;
- inode->u.ext2_i.i_flags &= ~EXT2_NOATIME_FL;
+ if (iattr->ia_valid & ATTR_ATTR_FLAG) {
+ flags = iattr->ia_attr_flags;
+ if (flags & ATTR_FLAG_SYNCRONOUS) {
+ inode->i_flags |= MS_SYNCHRONOUS;
+ inode->u.ext2_i.i_flags = EXT2_SYNC_FL;
+ } else {
+ inode->i_flags &= ~MS_SYNCHRONOUS;
+ inode->u.ext2_i.i_flags &= ~EXT2_SYNC_FL;
+ }
+ if (flags & ATTR_FLAG_NOATIME) {
+ inode->i_flags |= MS_NOATIME;
+ inode->u.ext2_i.i_flags = EXT2_NOATIME_FL;
+ } else {
+ inode->i_flags &= ~MS_NOATIME;
+ inode->u.ext2_i.i_flags &= ~EXT2_NOATIME_FL;
+ }
+ if (flags & ATTR_FLAG_APPEND) {
+ inode->i_flags |= S_APPEND;
+ inode->u.ext2_i.i_flags = EXT2_APPEND_FL;
+ } else {
+ inode->i_flags &= ~S_APPEND;
+ inode->u.ext2_i.i_flags &= ~EXT2_APPEND_FL;
+ }
+ if (flags & ATTR_FLAG_IMMUTABLE) {
+ inode->i_flags |= S_IMMUTABLE;
+ inode->u.ext2_i.i_flags = EXT2_IMMUTABLE_FL;
+ } else {
+ inode->i_flags &= ~S_IMMUTABLE;
+ inode->u.ext2_i.i_flags &= ~EXT2_IMMUTABLE_FL;
+ }
}
- if (flags & ATTR_FLAG_APPEND) {
- inode->i_flags |= S_APPEND;
- inode->u.ext2_i.i_flags = EXT2_APPEND_FL;
- } else {
- inode->i_flags &= ~S_APPEND;
- inode->u.ext2_i.i_flags &= ~EXT2_APPEND_FL;
- }
- if (flags & ATTR_FLAG_IMMUTABLE) {
- inode->i_flags |= S_IMMUTABLE;
- inode->u.ext2_i.i_flags = EXT2_IMMUTABLE_FL;
- } else {
- inode->i_flags &= ~S_IMMUTABLE;
- inode->u.ext2_i.i_flags &= ~EXT2_IMMUTABLE_FL;
- }
- mark_inode_dirty(inode);
+ inode_setattr(inode, iattr);
out:
return retval;
}
diff -ur linux.bak/fs/fat/inode.c linux/fs/fat/inode.c
--- linux.bak/fs/fat/inode.c Thu Jan 14 11:42:25 1999
+++ linux/fs/fat/inode.c Tue Jan 19 14:32:05 1999
@@ -771,16 +771,21 @@
if (error)
return MSDOS_SB(sb)->options.quiet ? 0 : error;
- inode_setattr(inode, attr);
+ if (attr->ia_valid & ATTR_MODE) {
+ if (IS_NOEXEC(inode) && !S_ISDIR(attr->ia_mode))
+ attr->ia_mode &= S_IFMT | S_IRUGO | S_IWUGO;
+ else
+ attr->ia_mode |= S_IXUGO;
- if (IS_NOEXEC(inode) && !S_ISDIR(inode->i_mode))
- inode->i_mode &= S_IFMT | S_IRUGO | S_IWUGO;
- else
- inode->i_mode |= S_IXUGO;
+ attr->ia_mode = ((attr->ia_mode & S_IFMT) |
+ ((((attr->ia_mode & S_IRWXU &
+ ~MSDOS_SB(sb)->options.fs_umask) |
+ S_IRUSR) >> 6)*S_IXUGO)) &
+ ~MSDOS_SB(sb)->options.fs_umask;
+ }
+
+ inode_setattr(inode, attr);
- inode->i_mode = ((inode->i_mode & S_IFMT) | ((((inode->i_mode & S_IRWXU
- & ~MSDOS_SB(sb)->options.fs_umask) | S_IRUSR) >> 6)*S_IXUGO)) &
- ~MSDOS_SB(sb)->options.fs_umask;
return 0;
}
diff -ur linux.bak/fs/open.c linux/fs/open.c
--- linux.bak/fs/open.c Tue Dec 29 11:40:35 1998
+++ linux/fs/open.c Mon Jan 18 16:48:09 1999
@@ -514,38 +514,39 @@
error = -EPERM;
if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
goto out;
- if (user == (uid_t) -1)
- user = inode->i_uid;
- if (group == (gid_t) -1)
- group = inode->i_gid;
newattrs.ia_mode = inode->i_mode;
- newattrs.ia_uid = user;
- newattrs.ia_gid = group;
- newattrs.ia_valid = ATTR_UID | ATTR_GID | ATTR_CTIME;
- /*
- * If the user or group of a non-directory has been changed by a
- * non-root user, remove the setuid bit.
- * 19981026 David C Niemi <niemi@tux.org>
- *
- */
- if ((inode->i_mode & S_ISUID) == S_ISUID &&
- !S_ISDIR(inode->i_mode)
- && current->fsuid)
- {
- newattrs.ia_mode &= ~S_ISUID;
- newattrs.ia_valid |= ATTR_MODE;
+ newattrs.ia_valid = ATTR_CTIME;
+
+ if ((user != (uid_t) -1) && (user != inode->i_uid)) {
+ newattrs.ia_uid = user;
+ newattrs.ia_valid |= ATTR_UID;
+
+ /*
+ * If the user or group of a non-directory has been changed,
+ * remove the setuid bit.
+ */
+ if ((inode->i_mode & S_ISUID) && !S_ISDIR(inode->i_mode)) {
+ newattrs.ia_mode &= ~S_ISUID;
+ newattrs.ia_valid |= ATTR_MODE;
+ }
}
- /*
- * Likewise, if the user or group of a non-directory has been changed
- * by a non-root user, remove the setgid bit UNLESS there is no group
- * execute bit (this would be a file marked for mandatory locking).
- * 19981026 David C Niemi <niemi@tux.org>
- */
- if (((inode->i_mode & (S_ISGID | S_IXGRP)) == (S_ISGID | S_IXGRP))
- && !S_ISDIR(inode->i_mode) && current->fsuid)
- {
- newattrs.ia_mode &= ~S_ISGID;
- newattrs.ia_valid |= ATTR_MODE;
+ if ((group != (gid_t) -1) && (group != inode->i_gid)) {
+ newattrs.ia_gid = group;
+ newattrs.ia_valid |= ATTR_GID;
+
+ /*
+ * Likewise, if the user or group of a non-directory has been
+ * changed, remove the setgid bit UNLESS there is no group
+ * execute bit (this would be a file marked for mandatory
+ * locking).
+ */
+ if (((inode->i_mode & (S_ISGID | S_IXGRP)) ==
+ (S_ISGID | S_IXGRP))
+ && !S_ISDIR(inode->i_mode))
+ {
+ newattrs.ia_mode &= ~S_ISGID;
+ newattrs.ia_valid |= ATTR_MODE;
+ }
}
error = DQUOT_TRANSFER(dentry, &newattrs);
out:
--LQksG6bCIzRHxTLp--
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/