2008-04-16 12:38:51

by Jeff Layton

[permalink] [raw]
Subject: [PATCH] knfsd: clear both setuid and setgid whenever a chown is done

Currently, knfsd only clears the setuid bit if the owner of a file
is changed on a SETATTR call, and only clears the setgid bit if
the group is changed. POSIX says this in the spec for chown():

"If the specified file is a regular file, one or more of the
S_IXUSR, S_IXGRP, or S_IXOTH bits of the file mode are set, and the
process does not have appropriate privileges, the set-user-ID
(S_ISUID) and set-group-ID (S_ISGID) bits of the file mode shall
be cleared upon successful return from chown()."

If I'm reading this correct, then knfsd is doing this wrong. It should
be clearing both the setuid and setgid bit on any SETATTR that changes
the uid or gid. This wasn't really as noticable before, but now that the
ATTR_KILL_S*ID bits are a no-op for the NFS client, it's more evident.

This patch corrects the nfsd_setattr logic so that this occurs. It also
does a bit of cleanup to the function and eliminates an unneeded local
variable.

Signed-off-by: Jeff Layton <[email protected]>
---
fs/nfsd/vfs.c | 30 ++++++++++++++----------------
1 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 17ac51b..7d6cd1a 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -264,7 +264,6 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
struct inode *inode;
int accmode = MAY_SATTR;
int ftype = 0;
- int imode;
__be32 err;
int host_err;
int size_change = 0;
@@ -360,25 +359,24 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
DQUOT_INIT(inode);
}

- imode = inode->i_mode;
if (iap->ia_valid & ATTR_MODE) {
iap->ia_mode &= S_IALLUGO;
- imode = iap->ia_mode |= (imode & ~S_IALLUGO);
- /* if changing uid/gid revoke setuid/setgid in mode */
- if ((iap->ia_valid & ATTR_UID) && iap->ia_uid != inode->i_uid) {
- iap->ia_valid |= ATTR_KILL_PRIV;
+ iap->ia_mode |= (inode->i_mode & ~S_IALLUGO);
+ }
+
+ /* Revoke setuid/setgid on chown */
+ if (((iap->ia_valid & ATTR_UID) && iap->ia_uid != inode->i_uid) ||
+ ((iap->ia_valid & ATTR_GID) && iap->ia_gid != inode->i_gid)) {
+ iap->ia_valid |= ATTR_KILL_PRIV;
+ if (iap->ia_valid & ATTR_MODE) {
+ /* we're setting mode too, just clear the mode bits */
iap->ia_mode &= ~S_ISUID;
+ if (iap->ia_mode & S_IXGRP)
+ iap->ia_mode &= ~S_ISGID;
+ } else {
+ /* set ATTR_KILL_* bits and let VFS handle it */
+ iap->ia_valid |= (ATTR_KILL_SUID | ATTR_KILL_SGID);
}
- if ((iap->ia_valid & ATTR_GID) && iap->ia_gid != inode->i_gid)
- iap->ia_mode &= ~S_ISGID;
- } else {
- /*
- * Revoke setuid/setgid bit on chown/chgrp
- */
- if ((iap->ia_valid & ATTR_UID) && iap->ia_uid != inode->i_uid)
- iap->ia_valid |= ATTR_KILL_SUID | ATTR_KILL_PRIV;
- if ((iap->ia_valid & ATTR_GID) && iap->ia_gid != inode->i_gid)
- iap->ia_valid |= ATTR_KILL_SGID;
}

/* Change the attributes. */
--
1.5.3.6


2008-04-16 18:37:28

by J. Bruce Fields

[permalink] [raw]
Subject: Re: [PATCH] knfsd: clear both setuid and setgid whenever a chown is done

On Wed, Apr 16, 2008 at 08:38:51AM -0400, Jeff Layton wrote:
> Currently, knfsd only clears the setuid bit if the owner of a file
> is changed on a SETATTR call, and only clears the setgid bit if
> the group is changed. POSIX says this in the spec for chown():
>
> "If the specified file is a regular file, one or more of the
> S_IXUSR, S_IXGRP, or S_IXOTH bits of the file mode are set, and the
> process does not have appropriate privileges, the set-user-ID
> (S_ISUID) and set-group-ID (S_ISGID) bits of the file mode shall
> be cleared upon successful return from chown()."
>
> If I'm reading this correct, then knfsd is doing this wrong. It should
> be clearing both the setuid and setgid bit on any SETATTR that changes
> the uid or gid. This wasn't really as noticable before, but now that the
> ATTR_KILL_S*ID bits are a no-op for the NFS client, it's more evident.

OK, thanks!

>
> This patch corrects the nfsd_setattr logic so that this occurs. It also
> does a bit of cleanup to the function and eliminates an unneeded local
> variable.

Might be nice to have the cleanup separately first, if that's easy.
(Should be for the "imode" removal, at least. Wonder how that got
there?)

One other question:

>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> fs/nfsd/vfs.c | 30 ++++++++++++++----------------
> 1 files changed, 14 insertions(+), 16 deletions(-)
>
> diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> index 17ac51b..7d6cd1a 100644
> --- a/fs/nfsd/vfs.c
> +++ b/fs/nfsd/vfs.c
> @@ -264,7 +264,6 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
> struct inode *inode;
> int accmode = MAY_SATTR;
> int ftype = 0;
> - int imode;
> __be32 err;
> int host_err;
> int size_change = 0;
> @@ -360,25 +359,24 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
> DQUOT_INIT(inode);
> }
>
> - imode = inode->i_mode;
> if (iap->ia_valid & ATTR_MODE) {
> iap->ia_mode &= S_IALLUGO;
> - imode = iap->ia_mode |= (imode & ~S_IALLUGO);
> - /* if changing uid/gid revoke setuid/setgid in mode */
> - if ((iap->ia_valid & ATTR_UID) && iap->ia_uid != inode->i_uid) {
> - iap->ia_valid |= ATTR_KILL_PRIV;
> + iap->ia_mode |= (inode->i_mode & ~S_IALLUGO);
> + }
> +
> + /* Revoke setuid/setgid on chown */
> + if (((iap->ia_valid & ATTR_UID) && iap->ia_uid != inode->i_uid) ||
> + ((iap->ia_valid & ATTR_GID) && iap->ia_gid != inode->i_gid)) {
> + iap->ia_valid |= ATTR_KILL_PRIV;
> + if (iap->ia_valid & ATTR_MODE) {
> + /* we're setting mode too, just clear the mode bits */
> iap->ia_mode &= ~S_ISUID;
> + if (iap->ia_mode & S_IXGRP)
> + iap->ia_mode &= ~S_ISGID;

Why the check for S_IXGRP? I notice it's mentioned in the posix text
(though this doesn't implement precisely that suggestion), but the old
code didn't do it that I can see.

--b.

> + } else {
> + /* set ATTR_KILL_* bits and let VFS handle it */
> + iap->ia_valid |= (ATTR_KILL_SUID | ATTR_KILL_SGID);
> }
> - if ((iap->ia_valid & ATTR_GID) && iap->ia_gid != inode->i_gid)
> - iap->ia_mode &= ~S_ISGID;
> - } else {
> - /*
> - * Revoke setuid/setgid bit on chown/chgrp
> - */
> - if ((iap->ia_valid & ATTR_UID) && iap->ia_uid != inode->i_uid)
> - iap->ia_valid |= ATTR_KILL_SUID | ATTR_KILL_PRIV;
> - if ((iap->ia_valid & ATTR_GID) && iap->ia_gid != inode->i_gid)
> - iap->ia_valid |= ATTR_KILL_SGID;
> }
>
> /* Change the attributes. */
> --
> 1.5.3.6
>

2008-04-16 18:46:53

by Jeff Layton

[permalink] [raw]
Subject: Re: [PATCH] knfsd: clear both setuid and setgid whenever a chown is done

On Wed, 16 Apr 2008 14:37:28 -0400
"J. Bruce Fields" <[email protected]> wrote:

> On Wed, Apr 16, 2008 at 08:38:51AM -0400, Jeff Layton wrote:
> > Currently, knfsd only clears the setuid bit if the owner of a file
> > is changed on a SETATTR call, and only clears the setgid bit if
> > the group is changed. POSIX says this in the spec for chown():
> >
> > "If the specified file is a regular file, one or more of the
> > S_IXUSR, S_IXGRP, or S_IXOTH bits of the file mode are set, and the
> > process does not have appropriate privileges, the set-user-ID
> > (S_ISUID) and set-group-ID (S_ISGID) bits of the file mode shall
> > be cleared upon successful return from chown()."
> >
> > If I'm reading this correct, then knfsd is doing this wrong. It should
> > be clearing both the setuid and setgid bit on any SETATTR that changes
> > the uid or gid. This wasn't really as noticable before, but now that the
> > ATTR_KILL_S*ID bits are a no-op for the NFS client, it's more evident.
>
> OK, thanks!
>
> >
> > This patch corrects the nfsd_setattr logic so that this occurs. It also
> > does a bit of cleanup to the function and eliminates an unneeded local
> > variable.
>
> Might be nice to have the cleanup separately first, if that's easy.
> (Should be for the "imode" removal, at least. Wonder how that got
> there?)
>

Yep, that's the main thing I was cleaning up. I'll plan to respin it as
2 or more separate patches.

> One other question:
>
> >
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > fs/nfsd/vfs.c | 30 ++++++++++++++----------------
> > 1 files changed, 14 insertions(+), 16 deletions(-)
> >
> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index 17ac51b..7d6cd1a 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -264,7 +264,6 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
> > struct inode *inode;
> > int accmode = MAY_SATTR;
> > int ftype = 0;
> > - int imode;
> > __be32 err;
> > int host_err;
> > int size_change = 0;
> > @@ -360,25 +359,24 @@ nfsd_setattr(struct svc_rqst *rqstp, struct svc_fh *fhp, struct iattr *iap,
> > DQUOT_INIT(inode);
> > }
> >
> > - imode = inode->i_mode;
> > if (iap->ia_valid & ATTR_MODE) {
> > iap->ia_mode &= S_IALLUGO;
> > - imode = iap->ia_mode |= (imode & ~S_IALLUGO);
> > - /* if changing uid/gid revoke setuid/setgid in mode */
> > - if ((iap->ia_valid & ATTR_UID) && iap->ia_uid != inode->i_uid) {
> > - iap->ia_valid |= ATTR_KILL_PRIV;
> > + iap->ia_mode |= (inode->i_mode & ~S_IALLUGO);
> > + }
> > +
> > + /* Revoke setuid/setgid on chown */
> > + if (((iap->ia_valid & ATTR_UID) && iap->ia_uid != inode->i_uid) ||
> > + ((iap->ia_valid & ATTR_GID) && iap->ia_gid != inode->i_gid)) {
> > + iap->ia_valid |= ATTR_KILL_PRIV;
> > + if (iap->ia_valid & ATTR_MODE) {
> > + /* we're setting mode too, just clear the mode bits */
> > iap->ia_mode &= ~S_ISUID;
> > + if (iap->ia_mode & S_IXGRP)
> > + iap->ia_mode &= ~S_ISGID;
>
> Why the check for S_IXGRP? I notice it's mentioned in the posix text
> (though this doesn't implement precisely that suggestion), but the old
> code didn't do it that I can see.
>
> --b.
>

Yes, that is different. I tried to mirror what notify_change actually
does. It's possible that someone could set the S_ISGID bit without
S_IXGRP, as that signifies mandatory locking. In that case, we
probably don't want to clear the S_ISGID bit.

> > + } else {
> > + /* set ATTR_KILL_* bits and let VFS handle it */
> > + iap->ia_valid |= (ATTR_KILL_SUID | ATTR_KILL_SGID);
> > }
> > - if ((iap->ia_valid & ATTR_GID) && iap->ia_gid != inode->i_gid)
> > - iap->ia_mode &= ~S_ISGID;
> > - } else {
> > - /*
> > - * Revoke setuid/setgid bit on chown/chgrp
> > - */
> > - if ((iap->ia_valid & ATTR_UID) && iap->ia_uid != inode->i_uid)
> > - iap->ia_valid |= ATTR_KILL_SUID | ATTR_KILL_PRIV;
> > - if ((iap->ia_valid & ATTR_GID) && iap->ia_gid != inode->i_gid)
> > - iap->ia_valid |= ATTR_KILL_SGID;
> > }
> >
> > /* Change the attributes. */
> > --
> > 1.5.3.6
> >

Thanks!
--
Jeff Layton <[email protected]>