Re: [PATCH] utime(s): Honour CAP_FOWNER when times==NULL
[ 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.*\' > > > > > > > > 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
Re: [PATCH] utime(s): Honour CAP_FOWNER when times==NULL
[ 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.
Re: [PATCH] utime(s): Honour CAP_FOWNER when times==NULL
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] utime(s): Honour CAP_FOWNER when times==NULL
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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] utime(s): Honour CAP_FOWNER when times==NULL
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
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] utime(s): Honour CAP_FOWNER when times==NULL
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.*\'
> > >
> > > 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] utime(s): Honour CAP_FOWNER when times==NULL
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.*\' > > > > 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... - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] utime(s): Honour CAP_FOWNER when times==NULL
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.*\' > > 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] utime(s): Honour CAP_FOWNER when times==NULL
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.*\' seems to show quite a few cases of this pattern.. Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] utime(s): Honour CAP_FOWNER when times==NULL
[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 <[EMAIL PROTECTED]>
---
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;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] utime(s): Honour CAP_FOWNER when times==NULL
[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 [EMAIL PROTECTED]
---
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;
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] utime(s): Honour CAP_FOWNER when times==NULL
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] utime(s): Honour CAP_FOWNER when times==NULL
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] utime(s): Honour CAP_FOWNER when times==NULL
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... - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] utime(s): Honour CAP_FOWNER when times==NULL
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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] utime(s): Honour CAP_FOWNER when times==NULL
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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] utime(s): Honour CAP_FOWNER when times==NULL
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.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] utime(s): Honour CAP_FOWNER when times==NULL
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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
