[PATCH 2/7] NFS: if ATTR_KILL_S*ID bits are set, then skip mode change

[PATCH 2/7] NFS: if ATTR_KILL_S*ID bits are set, then skip mode change

[Jeff Layton]

If the ATTR_KILL_S*ID bits are set then any mode change is only for
clearing the setuid/setgid bits. For NFS skip the mode change and
let the server handle it.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfs/inode.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index 45633f9..441bd8b 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -327,6 +327,10 @@ nfs_setattr(struct dentry *dentry, struct iattr *attr)
 
 	nfs_inc_stats(inode, NFSIOS_VFSSETATTR);
 
+	/* skip mode change if it's just for clearing setuid/setgid */
+	if (attr->ia_valid & (ATTR_KILL_SUID | ATTR_KILL_SGID))
+		attr->ia_valid &= ~ATTR_MODE;
+
 	if (attr->ia_valid & ATTR_SIZE) {
 		if (!S_ISREG(inode->i_mode) || attr->ia_size == i_size_read(inode))
 			attr->ia_valid &= ~ATTR_SIZE;
-- 
1.5.2.1

Re: [NFS] [PATCH 2/7] NFS: if ATTR_KILL_S*ID bits are set, then skip mode change

[Greg Banks]

On Tue, Sep 04, 2007 at 10:37:04AM -0400, Jeff Layton wrote:
> If the ATTR_KILL_S*ID bits are set then any mode change is only for
> clearing the setuid/setgid bits. For NFS skip the mode change and
> let the server handle it.

You're assuming the server will remove setuid and setgid bits on WRITE?
I don't see that behaviour specified in the RFC, at least for v3.
The RFC specifies a behaviour for the mtime attribute as a side
effect of WRITE, but says nothing about mode.  This means server
implementations are free to clobber setuid or not.  A quick experiment
shows that at least the Irix server will *NOT* clobber those bits.
So with an Irix server you've now lost this Linux-specific "security
feature".

I'm curious about the reasons behind this change.  You mention
credential issues; how exactly is it that you have the correct creds
to perform a WRITE rpc but not a SETATTR rpc?

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
Apparently, I'm Bedevere.  Which MPHG character are you?
I don't speak for SGI.

Re: [NFS] [PATCH 2/7] NFS: if ATTR_KILL_S*ID bits are set, then skip mode change

[Jeff Layton]

On Fri, 14 Sep 2007 20:25:45 +1000
Greg Banks <gnb@sgi.com> wrote:

> On Tue, Sep 04, 2007 at 10:37:04AM -0400, Jeff Layton wrote:
> > If the ATTR_KILL_S*ID bits are set then any mode change is only for
> > clearing the setuid/setgid bits. For NFS skip the mode change and
> > let the server handle it.
> 
> You're assuming the server will remove setuid and setgid bits on WRITE?
> I don't see that behaviour specified in the RFC, at least for v3.
> The RFC specifies a behaviour for the mtime attribute as a side
> effect of WRITE, but says nothing about mode.  This means server
> implementations are free to clobber setuid or not.  A quick experiment
> shows that at least the Irix server will *NOT* clobber those bits.
> So with an Irix server you've now lost this Linux-specific "security
> feature".
> 
> I'm curious about the reasons behind this change.  You mention
> credential issues; how exactly is it that you have the correct creds
> to perform a WRITE rpc but not a SETATTR rpc?
> 

Consider this case. user1 and user2 are both members of group
"allusers":

user1$ echo foo > foo
user1$ chgrp allusers foo
user1$ chmod 04770 foo
user2$ echo bar >> foo

On most local filesystems, this would work correctly. The end result
would be a file with mode 0770 and the expected contents. On NFS
though, the write by user2 fails. When the write is attempted, the
kernel tries to squash the setuid bit using the credentials of user2,
who's not allowed to change the mode. The write then fails because the
setattr fails.

There are other situations that are similar, but the bottom line is
that the linux-specific security feature doesn't work in all cases, and
in some situations it means that operations fail where they shouldn't.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [NFS] [PATCH 2/7] NFS: if ATTR_KILL_S*ID bits are set, then skip mode change

[Greg Banks]

On Fri, Sep 14, 2007 at 07:02:58AM -0400, Jeff Layton wrote:
> On Fri, 14 Sep 2007 20:25:45 +1000
> Greg Banks <gnb@sgi.com> wrote:
> 
> > I'm curious about the reasons behind this change.  You mention
> > credential issues; how exactly is it that you have the correct creds
> > to perform a WRITE rpc but not a SETATTR rpc?
> > 
> 
> Consider this case. user1 and user2 are both members of group
> "allusers":
> 
> user1$ echo foo > foo
> user1$ chgrp allusers foo
> user1$ chmod 04770 foo
> user2$ echo bar >> foo
> 
> On most local filesystems, this would work correctly. The end result
> would be a file with mode 0770 and the expected contents. On NFS
> though, the write by user2 fails. When the write is attempted, the
> kernel tries to squash the setuid bit using the credentials of user2,
> who's not allowed to change the mode. The write then fails because the
> setattr fails.

Ok, I ran an experiment and I see this failure mode.

So the SETATTR rpc is really a side effect of the client kernel's
behaviour and not an operation directly requested by the user process
on the client.  Is there any reason why that rpc needs to have user2's
creds?  Why not do the rpc with a fake set of creds with uid and gid
set to the uid and gid of the file, in this case user1/allusers ?
That way the rpc will most likely pass the server's permission check.

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
Apparently, I'm Bedevere.  Which MPHG character are you?
I don't speak for SGI.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH 2/7] NFS: if ATTR_KILL_S*ID bits are set, then skip mode change

[Jeff Layton]

On Fri, 14 Sep 2007 23:09:24 +1000
Greg Banks <gnb@sgi.com> wrote:

> On Fri, Sep 14, 2007 at 07:02:58AM -0400, Jeff Layton wrote:
> > On Fri, 14 Sep 2007 20:25:45 +1000
> > Greg Banks <gnb@sgi.com> wrote:
> > 
> > > I'm curious about the reasons behind this change.  You mention
> > > credential issues; how exactly is it that you have the correct creds
> > > to perform a WRITE rpc but not a SETATTR rpc?
> > > 
> > 
> > Consider this case. user1 and user2 are both members of group
> > "allusers":
> > 
> > user1$ echo foo > foo
> > user1$ chgrp allusers foo
> > user1$ chmod 04770 foo
> > user2$ echo bar >> foo
> > 
> > On most local filesystems, this would work correctly. The end result
> > would be a file with mode 0770 and the expected contents. On NFS
> > though, the write by user2 fails. When the write is attempted, the
> > kernel tries to squash the setuid bit using the credentials of user2,
> > who's not allowed to change the mode. The write then fails because the
> > setattr fails.
> 
> Ok, I ran an experiment and I see this failure mode.
> 
> So the SETATTR rpc is really a side effect of the client kernel's
> behaviour and not an operation directly requested by the user process
> on the client.  Is there any reason why that rpc needs to have user2's
> creds?  Why not do the rpc with a fake set of creds with uid and gid
> set to the uid and gid of the file, in this case user1/allusers ?
> That way the rpc will most likely pass the server's permission check.
> 

That might work in some cases, but there are many where it wouldn't...

Suppose user1 here is root and all of the user1 operations are being
done on the server. If the server has root squashing enabled, then
user2's operation would still fail.

Another problem:

Suppose we're using gssapi. There's no guarantee that the client will
have the proper credentials to fake up a call as user1 (you might need
user1 krb5 tickets, etc).

-- 
Jeff Layton <jlayton@redhat.com>

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [NFS] [PATCH 2/7] NFS: if ATTR_KILL_S*ID bits are set, then skip mode change

[Greg Banks]

On Fri, Sep 14, 2007 at 09:38:46AM -0400, Jeff Layton wrote:
> On Fri, 14 Sep 2007 23:09:24 +1000
> Greg Banks <gnb@sgi.com> wrote:
> 
> > On Fri, Sep 14, 2007 at 07:02:58AM -0400, Jeff Layton wrote:
> > > On Fri, 14 Sep 2007 20:25:45 +1000
> > > Greg Banks <gnb@sgi.com> wrote:
> > > 
> > > > I'm curious about the reasons behind this change.  You mention
> > > > credential issues; how exactly is it that you have the correct creds
> > > > to perform a WRITE rpc but not a SETATTR rpc?
> > > > 
> > > 
> > > Consider this case. user1 and user2 are both members of group
> > > "allusers":
> > > 
> > > user1$ echo foo > foo
> > > user1$ chgrp allusers foo
> > > user1$ chmod 04770 foo
> > > user2$ echo bar >> foo
> > > 
> > > On most local filesystems, this would work correctly. The end result
> > > would be a file with mode 0770 and the expected contents. On NFS
> > > though, the write by user2 fails. When the write is attempted, the
> > > kernel tries to squash the setuid bit using the credentials of user2,
> > > who's not allowed to change the mode. The write then fails because the
> > > setattr fails.
> > 
> > Ok, I ran an experiment and I see this failure mode.
> > 
> > So the SETATTR rpc is really a side effect of the client kernel's
> > behaviour and not an operation directly requested by the user process
> > on the client.  Is there any reason why that rpc needs to have user2's
> > creds?  Why not do the rpc with a fake set of creds with uid and gid
> > set to the uid and gid of the file, in this case user1/allusers ?
> > That way the rpc will most likely pass the server's permission check.
> > 
> 
> That might work in some cases, but there are many where it wouldn't...
> 
> Suppose user1 here is root and all of the user1 operations are being
> done on the server. If the server has root squashing enabled, then
> user2's operation would still fail.

In that case, user1's operations would also fail, which is even more
serious a problem.  Also arguably you actually *want* writes by a
nonroot user to a setuid root executable to fail ;-)

> Another problem:
> 
> Suppose we're using gssapi. There's no guarantee that the client will
> have the proper credentials to fake up a call as user1 (you might need
> user1 krb5 tickets, etc).

Yes, good point.  You could use the root creds, except for root squashing.
Ok, you convinced me.

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
Apparently, I'm Bedevere.  Which MPHG character are you?
I don't speak for SGI.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [NFS] [PATCH 2/7] NFS: if ATTR_KILL_S*ID bits are set, then skip mode change

[Jeff Layton]

On Sat, 15 Sep 2007 00:40:33 +1000
Greg Banks <gnb@sgi.com> wrote:

> On Fri, Sep 14, 2007 at 09:38:46AM -0400, Jeff Layton wrote:
> > On Fri, 14 Sep 2007 23:09:24 +1000
> > Greg Banks <gnb@sgi.com> wrote:
> > 
> > > On Fri, Sep 14, 2007 at 07:02:58AM -0400, Jeff Layton wrote:
> > > > On Fri, 14 Sep 2007 20:25:45 +1000
> > > > Greg Banks <gnb@sgi.com> wrote:
> > > > 
> > > > > I'm curious about the reasons behind this change.  You mention
> > > > > credential issues; how exactly is it that you have the correct creds
> > > > > to perform a WRITE rpc but not a SETATTR rpc?
> > > > > 
> > > > 
> > > > Consider this case. user1 and user2 are both members of group
> > > > "allusers":
> > > > 
> > > > user1$ echo foo > foo
> > > > user1$ chgrp allusers foo
> > > > user1$ chmod 04770 foo
> > > > user2$ echo bar >> foo
> > > > 
> > > > On most local filesystems, this would work correctly. The end result
> > > > would be a file with mode 0770 and the expected contents. On NFS
> > > > though, the write by user2 fails. When the write is attempted, the
> > > > kernel tries to squash the setuid bit using the credentials of user2,
> > > > who's not allowed to change the mode. The write then fails because the
> > > > setattr fails.
> > > 
> > > Ok, I ran an experiment and I see this failure mode.
> > > 
> > > So the SETATTR rpc is really a side effect of the client kernel's
> > > behaviour and not an operation directly requested by the user process
> > > on the client.  Is there any reason why that rpc needs to have user2's
> > > creds?  Why not do the rpc with a fake set of creds with uid and gid
> > > set to the uid and gid of the file, in this case user1/allusers ?
> > > That way the rpc will most likely pass the server's permission check.
> > > 
> > 
> > That might work in some cases, but there are many where it wouldn't...
> > 
> > Suppose user1 here is root and all of the user1 operations are being
> > done on the server. If the server has root squashing enabled, then
> > user2's operation would still fail.
> 
> In that case, user1's operations would also fail, which is even more
> serious a problem.  Also arguably you actually *want* writes by a
> nonroot user to a setuid root executable to fail ;-)
> 

Well, user1's operations would fail if done from the client, which is
why I mentioned that they would have to be done on the server.

The second point is a good one, but POSIX says that it should be allowed
if the permissions allow for it. The whole situation is somewhat
contrived anyway, I can't think of a place where this is something
you'd really want to do, but I think we need to try to follow the spec
as best as possible...

> > Another problem:
> > 
> > Suppose we're using gssapi. There's no guarantee that the client will
> > have the proper credentials to fake up a call as user1 (you might need
> > user1 krb5 tickets, etc).
> 
> Yes, good point.  You could use the root creds, except for root squashing.
> Ok, you convinced me.
> 

Right. When I was first looking at this, I considered some similar
approaches, but hit roadblocks with all of them. The only real option
seems to be to leave this to the server, but that does assume that the
server handles this properly.

Servers that don't are broken, IMO. If Irix isn't clearing these bits
on a write then it might be good to see if they can fix that...

-- 
Jeff Layton <jlayton@redhat.com>

Re: [NFS] [PATCH 2/7] NFS: if ATTR_KILL_S*ID bits are set, then skip mode change

[Greg Banks]

On Fri, Sep 14, 2007 at 10:58:38AM -0400, Jeff Layton wrote:
> On Sat, 15 Sep 2007 00:40:33 +1000
> Greg Banks <gnb@sgi.com> wrote:
> 
> 
> > Ok, you convinced me.
> 
> Right. When I was first looking at this, I considered some similar
> approaches, but hit roadblocks with all of them. The only real option
> seems to be to leave this to the server, but that does assume that the
> server handles this properly.
> 
> Servers that don't are broken, IMO.

According to what spec?  A quick trip around the machine room shows
that neither Solaris 10 nor Darwin 7.9.0 clobber setuid on write
either.

> If Irix isn't clearing these bits
> on a write then it might be good to see if they can fix that...

I think first you'd have to mount a serious argument that it's broken,
more serious than "it works differently from Linux".

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
Apparently, I'm Bedevere.  Which MPHG character are you?
I don't speak for SGI.

Re: [NFS] [PATCH 2/7] NFS: if ATTR_KILL_S*ID bits are set, then skip mode change

[Jeff Layton]

On Sat, 15 Sep 2007 01:43:45 +1000
Greg Banks <gnb@sgi.com> wrote:

> On Fri, Sep 14, 2007 at 10:58:38AM -0400, Jeff Layton wrote:
> > On Sat, 15 Sep 2007 00:40:33 +1000
> > Greg Banks <gnb@sgi.com> wrote:
> > 
> > 
> > > Ok, you convinced me.
> > 
> > Right. When I was first looking at this, I considered some similar
> > approaches, but hit roadblocks with all of them. The only real option
> > seems to be to leave this to the server, but that does assume that the
> > server handles this properly.
> > 
> > Servers that don't are broken, IMO.
> 
> According to what spec?  A quick trip around the machine room shows
> that neither Solaris 10 nor Darwin 7.9.0 clobber setuid on write
> either.
> 

Hmm, last time I checked Solaris, I thought it did, but that was
Solaris 11. I'll plan to fire up my solaris qemu image and test
it again...

> > If Irix isn't clearing these bits
> > on a write then it might be good to see if they can fix that...
> 
> I think first you'd have to mount a serious argument that it's broken,
> more serious than "it works differently from Linux".
> 

Good point. POSIX is frustratingly ambiguous on this:

     Upon successful completion, where nbyte is greater than 0, write()
     shall mark for update the st_ctime and st_mtime fields of the file,
     and if the file is a regular file, the S_ISUID and S_ISGID bits of
     the file mode may be cleared.

...the "may" in that last sentence makes it optional, I suppose. Even if
it weren't then I guess there's also an argument that a write that comes
in via a nfs server may not be subject to the same semantics as the
write() syscall.

In any case, "broken" is probably too strong a term :-)

-- 
Jeff Layton <jlayton@redhat.com>

Re: [NFS] [PATCH 2/7] NFS: if ATTR_KILL_S*ID bits are set, then skip mode change

[Trond Myklebust]

On Sat, 2007-09-15 at 01:43 +1000, Greg Banks wrote:
> On Fri, Sep 14, 2007 at 10:58:38AM -0400, Jeff Layton wrote:
> > If Irix isn't clearing these bits
> > on a write then it might be good to see if they can fix that...
> 
> I think first you'd have to mount a serious argument that it's broken,
> more serious than "it works differently from Linux".

How about: "If IRIX isn't clearing these bits then they're leaving their
customers wide open to all sorts of security issues."

Unless you make the chmod/chgrp atomic with the write, then there will
always be a way for a client to inject data while the setuid/setgid bits
are set: basically, it allows said client to rewrite a setuid/setgid
executable.

We're not fixing this in the client because it isn't fixable on the
client.

Trond