When suid is set and the non-owner user has write permission,
any writing into this file should be allowed and suid should be
removed after that.
However, current kernel only allows writing without truncations,
when we do truncations on that file, we get EPERM. This is a bug.
Steps to reproduce this bug:
% ls -l rootdir/file1
-rwsrwsrwx 1 root root 3 Jun 25 15:42 rootdir/file1
% echo h > rootdir/file1
zsh: operation not permitted: rootdir/file1
% ls -l rootdir/file1
-rwsrwsrwx 1 root root 3 Jun 25 15:42 rootdir/file1
% echo h >> rootdir/file1
% ls -l rootdir/file1
-rwxrwxrwx 1 root root 5 Jun 25 16:34 rootdir/file1
This patch fixes it.
(Need to Cc stable?)
Signed-off-by: WANG Cong <[email protected]>
Cc: Eric Sandeen <[email protected]>
Cc: Eugene Teo <[email protected]>
Cc: Al Viro <[email protected]>
---
diff --git a/fs/open.c b/fs/open.c
index dd98e80..ebf6d8d 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -199,7 +199,7 @@ out:
int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
struct file *filp)
{
- int err;
+ int ret;
struct iattr newattrs;
/* Not pretty: "inode->i_size" shouldn't really be signed. But it is. */
@@ -214,12 +214,15 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
}
/* Remove suid/sgid on truncate too */
- newattrs.ia_valid |= should_remove_suid(dentry);
+ ret = should_remove_suid(dentry);
+ newattrs.ia_valid |= ret;
+ if (ret)
+ newattrs.ia_valid |= ATTR_FORCE;
mutex_lock(&dentry->d_inode->i_mutex);
- err = notify_change(dentry, &newattrs);
+ ret = notify_change(dentry, &newattrs);
mutex_unlock(&dentry->d_inode->i_mutex);
- return err;
+ return ret;
}
static long do_sys_truncate(const char __user *pathname, loff_t length)
Amerigo Wang wrote:
> When suid is set and the non-owner user has write permission,
> any writing into this file should be allowed and suid should be
> removed after that.
>
> However, current kernel only allows writing without truncations,
> when we do truncations on that file, we get EPERM. This is a bug.
Thanks, I'd noticed this once too but never put together a patch... :(
> Steps to reproduce this bug:
>
> % ls -l rootdir/file1
> -rwsrwsrwx 1 root root 3 Jun 25 15:42 rootdir/file1
> % echo h > rootdir/file1
> zsh: operation not permitted: rootdir/file1
> % ls -l rootdir/file1
> -rwsrwsrwx 1 root root 3 Jun 25 15:42 rootdir/file1
> % echo h >> rootdir/file1
> % ls -l rootdir/file1
> -rwxrwxrwx 1 root root 5 Jun 25 16:34 rootdir/file1
>
> This patch fixes it.
>
> (Need to Cc stable?)
probably a good idea.
> Signed-off-by: WANG Cong <[email protected]>
> Cc: Eric Sandeen <[email protected]>
> Cc: Eugene Teo <[email protected]>
> Cc: Al Viro <[email protected]>
>
> ---
> diff --git a/fs/open.c b/fs/open.c
> index dd98e80..ebf6d8d 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -199,7 +199,7 @@ out:
> int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
> struct file *filp)
> {
> - int err;
> + int ret;
> struct iattr newattrs;
>
> /* Not pretty: "inode->i_size" shouldn't really be signed. But it is. */
> @@ -214,12 +214,15 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
> }
>
> /* Remove suid/sgid on truncate too */
> - newattrs.ia_valid |= should_remove_suid(dentry);
> + ret = should_remove_suid(dentry);
> + newattrs.ia_valid |= ret;
> + if (ret)
> + newattrs.ia_valid |= ATTR_FORCE;
So I think the main problem here is simply that we didn't set
ATTR_FORCE, right...
Seems a little odd to |= with ret, -then- check if it's non-0. Maybe:
/* Remove suid/sgid on truncate too */
- newattrs.ia_valid |= should_remove_suid(dentry);
+ ret = should_remove_suid(dentry);
+ if (ret)
+ newattrs.ia_valid |= (ret | ATTR_FORCE);
?
Otherwise this much looks right to me, though I don't claim to be the
world's foremost expert at this. :)
Thanks,
-Eric
> mutex_lock(&dentry->d_inode->i_mutex);
> - err = notify_change(dentry, &newattrs);
> + ret = notify_change(dentry, &newattrs);
> mutex_unlock(&dentry->d_inode->i_mutex);
> - return err;
> + return ret;
> }
>
> static long do_sys_truncate(const char __user *pathname, loff_t length)
Eric Sandeen wrote:
> Amerigo Wang wrote:
>> When suid is set and the non-owner user has write permission,
>> any writing into this file should be allowed and suid should be
>> removed after that.
>>
>> However, current kernel only allows writing without truncations,
>> when we do truncations on that file, we get EPERM. This is a bug.
...
> So I think the main problem here is simply that we didn't set
> ATTR_FORCE, right...
>
> Seems a little odd to |= with ret, -then- check if it's non-0. Maybe:
>
> /* Remove suid/sgid on truncate too */
> - newattrs.ia_valid |= should_remove_suid(dentry);
> + ret = should_remove_suid(dentry);
> + if (ret)
> + newattrs.ia_valid |= (ret | ATTR_FORCE);
>
On second thought, and after talking w/ eparis, I think this probably
needs a security_inode_killpriv() too... it seems like it might be best
to change file_remove_suid(*file) to dentry_remove_suid(*dentry) and
just call that from do_truncate()?
-Eric
On Wed, Jul 1, 2009 at 3:27 PM, Eric Sandeen<[email protected]> wrote:
> Eric Sandeen wrote:
>> Amerigo Wang wrote:
>>> When suid is set and the non-owner user has write permission,
>>> any writing into this file should be allowed and suid should be
>>> removed after that.
>>>
>>> However, current kernel only allows writing without truncations,
>>> when we do truncations on that file, we get EPERM. This is a bug.
>
> ...
>
>> So I think the main problem here is simply that we didn't set
>> ATTR_FORCE, right...
>>
>> Seems a little odd to |= with ret, -then- check if it's non-0. ?Maybe:
>>
>> ? ? ? /* Remove suid/sgid on truncate too */
>> - ? ? newattrs.ia_valid |= should_remove_suid(dentry);
>> + ? ? ret = should_remove_suid(dentry);
>> + ? ? if (ret)
>> + ? ? ? ? ? ? newattrs.ia_valid |= (ret | ATTR_FORCE);
>>
>
> On second thought, and after talking w/ eparis, I think this probably
> needs a security_inode_killpriv() too... it seems like it might be best
> to change file_remove_suid(*file) to dentry_remove_suid(*dentry) and
> just call that from do_truncate()?
>
> -Eric
All of this stuff seems horribly complex.... I'm trying to wrap my
head around everything going on here as well....
-Eric (Paris)
On Wed, 2009-07-01 at 15:29 -0400, Eric Paris wrote:
> On Wed, Jul 1, 2009 at 3:27 PM, Eric Sandeen<[email protected]> wrote:
> > Eric Sandeen wrote:
> >> Amerigo Wang wrote:
> >>> When suid is set and the non-owner user has write permission,
> >>> any writing into this file should be allowed and suid should be
> >>> removed after that.
> >>>
> >>> However, current kernel only allows writing without truncations,
> >>> when we do truncations on that file, we get EPERM. This is a bug.
> >
> > ...
> >
> >> So I think the main problem here is simply that we didn't set
> >> ATTR_FORCE, right...
> >>
> >> Seems a little odd to |= with ret, -then- check if it's non-0. Maybe:
> >>
> >> /* Remove suid/sgid on truncate too */
> >> - newattrs.ia_valid |= should_remove_suid(dentry);
> >> + ret = should_remove_suid(dentry);
> >> + if (ret)
> >> + newattrs.ia_valid |= (ret | ATTR_FORCE);
> >>
> >
> > On second thought, and after talking w/ eparis, I think this probably
> > needs a security_inode_killpriv() too... it seems like it might be best
> > to change file_remove_suid(*file) to dentry_remove_suid(*dentry) and
> > just call that from do_truncate()?
> >
> > -Eric
>
> All of this stuff seems horribly complex.... I'm trying to wrap my
> head around everything going on here as well....
So NAK on both Amerigo's and Eric's patch. ATTR_FORCE is wrong.
ATTR_FORCE here is going to force the security system to accept ALL of
the attr changes, not just the ATTR_KILL_SUID and/or ATTR_KILL_SGID you
|= in from should_remove_suid.
You need to follow esandeen's recommendation, change file_remove_suid()
to dentry_remove_suid() and then use dentry_remove_suid() in do_truncate
instead of what you are currently trying to do (and I think that's
supposed to be done under the i_mutex right?)
-Eric
-Eric
Eric Paris wrote:
> So NAK on both Amerigo's and Eric's patch. ATTR_FORCE is wrong.
> ATTR_FORCE here is going to force the security system to accept ALL of
> the attr changes, not just the ATTR_KILL_SUID and/or ATTR_KILL_SGID you
> |= in from should_remove_suid.
> You need to follow esandeen's recommendation, change file_remove_suid()
> to dentry_remove_suid() and then use dentry_remove_suid() in do_truncate
> instead of what you are currently trying to do (and I think that's
> supposed to be done under the i_mutex right?)
>
But file_remove_suid() actually adds ATTR_FORCE too, in __remove_suid()...
On Thu, 2009-07-02 at 18:14 +0800, Amerigo Wang wrote:
> Eric Paris wrote:
> > So NAK on both Amerigo's and Eric's patch. ATTR_FORCE is wrong.
> > ATTR_FORCE here is going to force the security system to accept ALL of
> > the attr changes, not just the ATTR_KILL_SUID and/or ATTR_KILL_SGID you
> > |= in from should_remove_suid.
> > You need to follow esandeen's recommendation, change file_remove_suid()
> > to dentry_remove_suid() and then use dentry_remove_suid() in do_truncate
> > instead of what you are currently trying to do (and I think that's
> > supposed to be done under the i_mutex right?)
> >
> But file_remove_suid() actually adds ATTR_FORCE too, in __remove_suid()...
The difference being that it adds it to a private ia_valid that ONLY
contains the SUID/SGID bits that we need to force the removal of. Not
to the ia_valid that contains ATTR_SIZE and ATTR_FILE.
-Eric
Eric Paris wrote:
> On Thu, 2009-07-02 at 18:14 +0800, Amerigo Wang wrote:
>
>> Eric Paris wrote:
>>
>>> So NAK on both Amerigo's and Eric's patch. ATTR_FORCE is wrong.
>>> ATTR_FORCE here is going to force the security system to accept ALL of
>>> the attr changes, not just the ATTR_KILL_SUID and/or ATTR_KILL_SGID you
>>> |= in from should_remove_suid.
>>> You need to follow esandeen's recommendation, change file_remove_suid()
>>> to dentry_remove_suid() and then use dentry_remove_suid() in do_truncate
>>> instead of what you are currently trying to do (and I think that's
>>> supposed to be done under the i_mutex right?)
>>>
>>>
>> But file_remove_suid() actually adds ATTR_FORCE too, in __remove_suid()...
>>
>
> The difference being that it adds it to a private ia_valid that ONLY
> contains the SUID/SGID bits that we need to force the removal of. Not
> to the ia_valid that contains ATTR_SIZE and ATTR_FILE.
>
Yeah! Got it. I will prepare a new patch...
Thank you!