[PATCH] utime(s): Honour CAP_FOWNER when times==NULL

[PATCH] utime(s): Honour CAP_FOWNER when times==NULL

[Satyam Sharma]

[PATCH] utime(s): Honour CAP_FOWNER when times==NULL

do_utimes() does not honour CAP_FOWNER when times==NULL.
Trivial and obvious one-line fix.

Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>

---

BTW this bug was hidden by the fact that we call vfs_permission() from the
code below (for) this condition. vfs_permission() inevitably degenerates to
generic_permission() which returns 0 (success) for CAP_DAC_OVERRIDE.

However, a privileged process that has released CAP_DAC_OVERRIDE (but carries
CAP_FOWNER) would still get bit by this issue. There is a sample userspace
program (with instructions on how to reproduce) available at:

http://www.cse.iitk.ac.in/users/ssatyam/test.c

 fs/utimes.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/utimes.c b/fs/utimes.c
index b3c8895..83a7e69 100644
--- a/fs/utimes.c
+++ b/fs/utimes.c
@@ -106,7 +106,7 @@ long do_utimes(int dfd, char __user *filename, struct timespec *times, int flags
                 if (IS_IMMUTABLE(inode))
                         goto dput_and_out;
 
-		if (current->fsuid != inode->i_uid) {
+		if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER)) {
 			if (f) {
 				if (!(f->f_mode & FMODE_WRITE))
 					goto dput_and_out;

Re: [PATCH] utime(s): Honour CAP_FOWNER when times==NULL

[Linus Torvalds]

On Tue, 17 Jul 2007, Satyam Sharma wrote:
>
> [PATCH] utime(s): Honour CAP_FOWNER when times==NULL
> 
> do_utimes() does not honour CAP_FOWNER when times==NULL.
> Trivial and obvious one-line fix.

Ahh, ok. Is this old, or was it introduced recently (I'm looking at my 
recent change to that area, it doesn't seem to introduce this)?

IOW, how did you even notice this?

Also, while your one-liner looks correct, it does make me wonder: the 
whole

	if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))

test is a rather common test, and in fact, arguably, every time you see 
one part of it, you should probably see the other. Would it make sense to 
make a helper inline function to do this, and replace all users? Doing a

	git grep 'fsuid.*\<i_uid\>'

seems to show quite a few cases of this pattern..

		Linus

Re: [PATCH] utime(s): Honour CAP_FOWNER when times==NULL

[Satyam Sharma]

On Mon, 16 Jul 2007, Linus Torvalds wrote:
> 
> 
> On Tue, 17 Jul 2007, Satyam Sharma wrote:
> >
> > [PATCH] utime(s): Honour CAP_FOWNER when times==NULL
> > 
> > do_utimes() does not honour CAP_FOWNER when times==NULL.
> > Trivial and obvious one-line fix.
> 
> Ahh, ok. Is this old, or was it introduced recently (I'm looking at my 
> recent change to that area, it doesn't seem to introduce this)?

Old, I'd think. Note that this issue was hidden behind the fact that
we would call vfs_permission()->generic_permission() anyway even if
that check failed.

Now generic_permission() returns 0 (success) if capable(CAP_DAC_OVERRIDE)
and -EACCES otherwise (that's for MAY_WRITE tests, which is what this
one is). Now I suspect most (all?) common userspace programs out there
just don't ever have a capable(CAP_DAC_OVERRIDE) && !capable(CAP_FOWNER)
kind of capability mix, so we never saw this before.

[ That does not mean this is not an issue, of course, though. ]

> IOW, how did you even notice this?

Just code inspection. In fact I got interested in following this
codepath precisely after the recent discussion on this list ...

> Also, while your one-liner looks correct, it does make me wonder: the 
> whole
> 
> 	if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
> 
> test is a rather common test, and in fact, arguably, every time you see 
> one part of it, you should probably see the other. Would it make sense to 
> make a helper inline function to do this, and replace all users? Doing a
> 
> 	git grep 'fsuid.*\<i_uid\>'
> 
> seems to show quite a few cases of this pattern..

Yes, I thought of writing a helper function for this myself. The semantics
of CAP_FOWNER sort of justify that, but probably better to get Al's views
on this first.

Satyam

Re: [PATCH] utime(s): Honour CAP_FOWNER when times==NULL

[Al Viro]

On Tue, Jul 17, 2007 at 01:00:42AM +0530, Satyam Sharma wrote:
> > 	if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
> > 
> > test is a rather common test, and in fact, arguably, every time you see 
> > one part of it, you should probably see the other. Would it make sense to 
> > make a helper inline function to do this, and replace all users? Doing a
> > 
> > 	git grep 'fsuid.*\<i_uid\>'
> > 
> > seems to show quite a few cases of this pattern..
> 
> Yes, I thought of writing a helper function for this myself. The semantics
> of CAP_FOWNER sort of justify that, but probably better to get Al's views
> on this first.

Helper makes sense (and most of these places will become its call), but...
E.g. IIRC the change of UID requires CAP_CHOWN; CAP_FOWNER is not enough.
Ditto for change of GID.  setlease() is using CAP_LEASE and that appears
to be intentional (no idea what relevant standards say here)...

I'd suggest converting the obvious cases with new helper and taking the
rest one-by-one after that.  Some of those might want CAP_FOWNER added,
some not...

Re: [PATCH] utime(s): Honour CAP_FOWNER when times==NULL

[Satyam Sharma]

On Mon, 16 Jul 2007, Al Viro wrote:
> On Tue, Jul 17, 2007 at 01:00:42AM +0530, Satyam Sharma wrote:
> > > 	if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
> > > 
> > > test is a rather common test, and in fact, arguably, every time you see 
> > > one part of it, you should probably see the other. Would it make sense to 
> > > make a helper inline function to do this, and replace all users? Doing a
> > > 
> > > 	git grep 'fsuid.*\<i_uid\>'
> > > 
> > > seems to show quite a few cases of this pattern..
> > 
> > Yes, I thought of writing a helper function for this myself. The semantics
> > of CAP_FOWNER sort of justify that, but probably better to get Al's views
> > on this first.
> 
> Helper makes sense (and most of these places will become its call), but...
> E.g. IIRC the change of UID requires CAP_CHOWN; CAP_FOWNER is not enough.
> Ditto for change of GID.  setlease() is using CAP_LEASE and that appears
> to be intentional (no idea what relevant standards say here)...
> 
> I'd suggest converting the obvious cases with new helper and taking the
> rest one-by-one after that.  Some of those might want CAP_FOWNER added,
> some not...

There aren't too many negative results, here's a little audit:

fs/attr.c:32:
fs/attr.c:38:

-> Both are from inode_change_ok(). [ for chown(2) and chgrp(2) ]
-> CAP_FOWNER is not checked for either case, I think it should be.
-> CAP_CHOWN is anyway checked for explicitly later in that condition.

fs/namei.c:186: if (current->fsuid == inode->i_uid)

-> generic_permission().
-> I wonder if CAP_FOWNER processes should ever even be calling into
   this function in the first place (?)
-> So best to keep CAP_FOWNER out of this condition (?)

fs/namei.c:438: if (current->fsuid == inode->i_uid)

-> exec_permission_lite().
-> This is a clone function of the previous one, so again CAP_FOWNER
   out of this (?)

fs/reiserfs/ioctl.c:54:
fs/xattr.c:63:
-> False positives, CAP_FOWNER checked on line below.
-> Helper would help for both cases.

Anwyay, so I'm thinking of adding:

struct inode;

int is_not_owner(struct inode *)
{
	return ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER));
}

to linux/capability.h inside the __KERNEL__ #ifdef, asm/current.h is
included in there already.

And then do the necessary conversions. Sounds OK?

Satyam

Re: [PATCH] utime(s): Honour CAP_FOWNER when times==NULL

[Satyam Sharma]

On Tue, 17 Jul 2007, Satyam Sharma wrote:
> [...]
> Anwyay, so I'm thinking of adding:
> 
> struct inode;
> 
> int is_not_owner(struct inode *)

  ^static inline                 ^inode

of course.

> {
> 	return ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER));
> }
> 
> to linux/capability.h inside the __KERNEL__ #ifdef, asm/current.h is
> included in there already.
> 
> And then do the necessary conversions. Sounds OK?
> 
> Satyam
> 

Re: [PATCH] utime(s): Honour CAP_FOWNER when times==NULL

[Al Viro]

On Tue, Jul 17, 2007 at 03:24:14AM +0530, Satyam Sharma wrote:
> On Tue, 17 Jul 2007, Satyam Sharma wrote:
> > [...]
> > Anwyay, so I'm thinking of adding:
> > 
> > struct inode;
> > 
> > int is_not_owner(struct inode *)
> 
>   ^static inline                 ^inode
> 
> of course.
 
> > {
> > 	return ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER));
> > }

This is pointless.  If you do not have definition of struct inode
already available, you'll get breakage on access to ->i_uid.

Re: [PATCH] utime(s): Honour CAP_FOWNER when times==NULL

[Satyam Sharma]

On Tue, 17 Jul 2007, Al Viro wrote:
> On Tue, Jul 17, 2007 at 03:24:14AM +0530, Satyam Sharma wrote:
> > On Tue, 17 Jul 2007, Satyam Sharma wrote:
> > > [...]
> > > Anwyay, so I'm thinking of adding:
> > > 
> > > struct inode;
> > > 
> > > int is_not_owner(struct inode *)
> > 
> >   ^static inline                 ^inode
> > 
> > of course.
>  
> > > {
> > > 	return ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER));
> > > }
> 
> This is pointless.  If you do not have definition of struct inode
> already available, you'll get breakage on access to ->i_uid.

Yes, I realized that later :-) That was just a (dumb) sketch typed
into the mailer ... I'll send out the conversion patch after compiling
+ booting, of course.

Thanks,
Satyam

Re: [PATCH] utime(s): Honour CAP_FOWNER when times==NULL

[Satyam Sharma]

[ Ok, last overview of this thing. ]


On Tue, 17 Jul 2007, Satyam Sharma wrote:
> On Mon, 16 Jul 2007, Al Viro wrote:
> > On Tue, Jul 17, 2007 at 01:00:42AM +0530, Satyam Sharma wrote:
> > > > 	if ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER))
> > > > 
> > > > test is a rather common test, and in fact, arguably, every time you see 
> > > > one part of it, you should probably see the other. Would it make sense to 
> > > > make a helper inline function to do this, and replace all users? Doing a
> > > > 
> > > > 	git grep 'fsuid.*\<i_uid\>'
> > > > 
> > > > seems to show quite a few cases of this pattern..
> > > 
> > > Yes, I thought of writing a helper function for this myself. The semantics
> > > of CAP_FOWNER sort of justify that, but probably better to get Al's views
> > > on this first.
> > 
> > Helper makes sense (and most of these places will become its call), but...
> > E.g. IIRC the change of UID requires CAP_CHOWN; CAP_FOWNER is not enough.
> > Ditto for change of GID.


Al, you were was *spot* on here. In fact CAP_FOWNER has no role to
play in chown(2), I think.

I did try force-fitting CAP_FOWNER to this thing, and even produced
a patch, but note that bullet one and (especially) two of DESCRIPTION in
(http://www.opengroup.org/onlinepubs/009695399/functions/chown.html):

"Changing the group ID is permitted to a process with an effective user
ID equal to the user ID of the file, but without appropriate privileges,
if and only if owner is equal to the file's user ID or ( uid_t)-1 and
group is equal either to the calling process' effective group ID or to
one of its supplementary group IDs."

pretty much rules out any role for CAP_FOWNER. CAP_CHOWN is clearly
the "appropriate privileges" in question here, and force-fitting
CAP_FOWNER to "with an effective user ID equal to the user ID of the
file, but *without* appropriate privileges" sounds almost sinful.

And, the part about "group is equal either to the *calling* process'
effective group ID or to one of its supplementary group IDs" is the
last straw, as follows:

This lead to the following behaviour when I tested with that patch
(to force-fit CAP_FOWNER into this) applied: a process carrying
CAP_FOWNER (say with fsuid == userA) could change the i_gid of a file
(say owned by userB) to a supplementary group of userA (calling process)
such that userB (the original owner) may not even necessarily be a
member of that supplementary group (the new i_gid).

This looked like undesirable behaviour to me, so I decided to discard
that patch, and stick to our current behaviour which seems correct.


> > setlease() is using CAP_LEASE and that appears
> > to be intentional (no idea what relevant standards say here)...


I agree here as well, in principle. There is surprisingly little
open documentation available about CAP_LEASE, so someone might need
to check that up, but I would classify this similar to the CAP_CHOWN
case and continue to keep CAP_FOWNER out of it, as it presently is.


> > I'd suggest converting the obvious cases with new helper and taking the
> > rest one-by-one after that.  Some of those might want CAP_FOWNER added,
> > some not...
> 
> There aren't too many negative results, here's a little audit:
> 
> fs/attr.c:32:
> fs/attr.c:38:
> 
> -> Both are from inode_change_ok(). [ for chown(2) and chgrp(2) ]
> -> CAP_FOWNER is not checked for either case, I think it should be.
> -> CAP_CHOWN is anyway checked for explicitly later in that condition.


So this one was not converted.


> fs/namei.c:186: if (current->fsuid == inode->i_uid)
> 
> -> generic_permission().
> -> I wonder if CAP_FOWNER processes should ever even be calling into
>    this function in the first place (?)
> -> So best to keep CAP_FOWNER out of this condition (?)
> 
> fs/namei.c:438: if (current->fsuid == inode->i_uid)
> 
> -> exec_permission_lite().
> -> This is a clone function of the previous one, so again CAP_FOWNER
>    out of this (?)


Neither these. Seeing from capabilities(7) man page:

CAP_FOWNER
	Bypass permission checks on operations that normally require
	the file system UID of the process to match the UID of the file
	(e.g., chmod(2), utime(2)), excluding those operations covered
	by the CAP_DAC_OVERRIDE and CAP_DAC_READ_SEARCH; [...]

I think generic_permission() and exec_permission_lite() are precisely
the "operations covered by CAP_DAC_OVERRIDE and CAP_DAC_READ_SEARCH"
being mentioned here. So no modification here either.


> fs/reiserfs/ioctl.c:54:
> fs/xattr.c:63:
> -> False positives, CAP_FOWNER checked on line below.
> -> Helper would help for both cases.


Another thing: I relaxed the grep regexp there a bit and caught a couple
more users in XFS. After some trying-to-audit the 5000-line files
containing 1000-line functions using non-standard inode structures and
weird macros, I can vouch that the the ownership / permission checks
there are just fine :-)

So, to summarize: the do_utimes() change was the only problematic point
in the kernel. All other points that _exclude_ a capable(CAP_FOWNER) check
do so for good reasons.


> Anwyay, so I'm thinking of adding:
> 
> struct inode;
> 
> int is_not_owner(struct inode *)
> {
> 	return ((current->fsuid != inode->i_uid) && !capable(CAP_FOWNER));
> }
> 
> to linux/capability.h inside the __KERNEL__ #ifdef, asm/current.h is
> included in there already.

On second thoughts, I decided it was much more tasteful to keep this
function in include/linux/fs.h. This also meant we have a macro now,
and not an inline function (sadly) because of header file issues that
caused build breakage. I could've avoided the breakage by #include'ing
sched.h from fs.h, but decided against it. (Probably explains why
get_fs_excl() and friends are also macros and not inline functions in
that file.)

Also, named it "is_owner_or_cap(inode)". Patch follows next.

Satyam