[PATCH] nfsd4: permit read opens of executable-only files

[PATCH] nfsd4: permit read opens of executable-only files

[J. Bruce Fields]

From: J. Bruce Fields <bfields@redhat.com>

A client that wants to execute a file must be able to read it.  Read
opens over nfs are therefore implicitly allowed for executable files
even when those files are not readable.

NFSv2/v3 get this right by using a passed-in NFSD_MAY_OWNER_OVERRIDE on
read requests, but NFSv4 has gotten this wrong ever since
dc730e173785e29b297aa605786c94adaffe2544 "nfsd4: fix owner-override on
open", when we realized that the file owner shouldn't override
permissions on non-reclaim NFSv4 opens.

So we can't use NFSD_MAY_OWNER_OVERRIDE to tell nfsd_permission to allow
reads of executable files.

So, do the same thing we do whenever we encounter another weird NFS
permission nit: define yet another NFSD_MAY_* flag.

The industry's future standardization on 128-bit processors will be
motivated primarily by the need for integers with enough bits for all
the NFSD_MAY_* flags.

Reported-by: Leonardo Borda <leonardoborda@gmail.com>
Cc: stable@kernel.org
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/nfs4proc.c |    2 ++
 fs/nfsd/vfs.c      |    3 ++-
 fs/nfsd/vfs.h      |    1 +
 3 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index d30253a..3fb07ac 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -156,6 +156,8 @@ do_open_permission(struct svc_rqst *rqstp, struct svc_fh *current_fh, struct nfs
 		!(open->op_share_access & NFS4_SHARE_ACCESS_WRITE))
 		return nfserr_inval;
 
+	accmode |= NFSD_MAY_READ_IF_EXEC;
+
 	if (open->op_share_access & NFS4_SHARE_ACCESS_READ)
 		accmode |= NFSD_MAY_READ;
 	if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE)
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 4c22870..75c35fa 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -2116,7 +2116,8 @@ nfsd_permission(struct svc_rqst *rqstp, struct svc_export *exp,
 
 	/* Allow read access to binaries even when mode 111 */
 	if (err == -EACCES && S_ISREG(inode->i_mode) &&
-	    acc == (NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE))
+	     (acc == (NFSD_MAY_READ | NFSD_MAY_OWNER_OVERRIDE) ||
+	      acc == (NFSD_MAY_READ | NFSD_MAY_READ_IF_EXEC)))
 		err = inode_permission(inode, MAY_EXEC);
 
 	return err? nfserrno(err) : 0;
diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index e0bbac0..a22e40e 100644
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -25,6 +25,7 @@
 #define NFSD_MAY_BYPASS_GSS_ON_ROOT 256
 #define NFSD_MAY_NOT_BREAK_LEASE 512
 #define NFSD_MAY_BYPASS_GSS	1024
+#define NFSD_MAY_READ_IF_EXEC	2048
 
 #define NFSD_MAY_CREATE		(NFSD_MAY_EXEC|NFSD_MAY_WRITE)
 #define NFSD_MAY_REMOVE		(NFSD_MAY_EXEC|NFSD_MAY_WRITE|NFSD_MAY_TRUNC)
-- 
1.7.4.1

Re: [PATCH] nfsd4: permit read opens of executable-only files

[Jim Rees]

J. Bruce Fields wrote:

   #define NFSD_MAY_BYPASS_GSS_ON_ROOT 256
   #define NFSD_MAY_NOT_BREAK_LEASE 512
   #define NFSD_MAY_BYPASS_GSS	1024
  +#define NFSD_MAY_READ_IF_EXEC	2048

Not the fault of your patch, but it seems odd, unreadable, and error-prone
that these flag bits are defined in terms of base-10 numbers.

Re: [PATCH] nfsd4: permit read opens of executable-only files

[J. Bruce Fields]

On Thu, Aug 25, 2011 at 01:31:47PM -0400, Jim Rees wrote:
> J. Bruce Fields wrote:
> 
>    #define NFSD_MAY_BYPASS_GSS_ON_ROOT 256
>    #define NFSD_MAY_NOT_BREAK_LEASE 512
>    #define NFSD_MAY_BYPASS_GSS	1024
>   +#define NFSD_MAY_READ_IF_EXEC	2048
> 
> Not the fault of your patch, but it seems odd, unreadable, and error-prone
> that these flag bits are defined in terms of base-10 numbers.

Odd is the one thing it isn't.

I guess 1 << n would be easier but hex makes the mask line up nicer.
So there.  ?

--b.

commit a9fd873383e3affa5ba7017713fa46949147d418
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Thu Aug 25 14:23:39 2011 -0400

    nfsd: prettify NFSD_MAY_* flag definitions
    
    Cc: Jim Rees <rees@umich.edu>
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
index a22e40e..503f3bf 100644
--- a/fs/nfsd/vfs.h
+++ b/fs/nfsd/vfs.h
@@ -10,22 +10,22 @@
 /*
  * Flags for nfsd_permission
  */
-#define NFSD_MAY_NOP		0
-#define NFSD_MAY_EXEC		1 /* == MAY_EXEC */
-#define NFSD_MAY_WRITE		2 /* == MAY_WRITE */
-#define NFSD_MAY_READ		4 /* == MAY_READ */
-#define NFSD_MAY_SATTR		8
-#define NFSD_MAY_TRUNC		16
-#define NFSD_MAY_LOCK		32
-#define NFSD_MAY_MASK		63
+#define NFSD_MAY_NOP			0
+#define NFSD_MAY_EXEC			0x001 /* == MAY_EXEC */
+#define NFSD_MAY_WRITE			0x002 /* == MAY_WRITE */
+#define NFSD_MAY_READ			0x004 /* == MAY_READ */
+#define NFSD_MAY_SATTR			0x008
+#define NFSD_MAY_TRUNC			0x010
+#define NFSD_MAY_LOCK			0x020
+#define NFSD_MAY_MASK			0x03f
 
 /* extra hints to permission and open routines: */
-#define NFSD_MAY_OWNER_OVERRIDE	64
-#define NFSD_MAY_LOCAL_ACCESS	128 /* IRIX doing local access check on device special file*/
-#define NFSD_MAY_BYPASS_GSS_ON_ROOT 256
-#define NFSD_MAY_NOT_BREAK_LEASE 512
-#define NFSD_MAY_BYPASS_GSS	1024
-#define NFSD_MAY_READ_IF_EXEC	2048
+#define NFSD_MAY_OWNER_OVERRIDE		0x040
+#define NFSD_MAY_LOCAL_ACCESS		0x080 /* for device special files */
+#define NFSD_MAY_BYPASS_GSS_ON_ROOT	0x100
+#define NFSD_MAY_NOT_BREAK_LEASE	0x200
+#define NFSD_MAY_BYPASS_GSS		0x400
+#define NFSD_MAY_READ_IF_EXEC		0x800
 
 #define NFSD_MAY_CREATE		(NFSD_MAY_EXEC|NFSD_MAY_WRITE)
 #define NFSD_MAY_REMOVE		(NFSD_MAY_EXEC|NFSD_MAY_WRITE|NFSD_MAY_TRUNC)

Re: [PATCH] nfsd4: permit read opens of executable-only files

[Jim Rees]

J. Bruce Fields wrote:

  Odd is the one thing it isn't.
  
  I guess 1 << n would be easier but hex makes the mask line up nicer.
  So there.  ?

I glanced through include/linux and there doesn't seem to be any one
standard.  In fs.h you can find hex, decimal, and 1 << n all mixed together.
The fs-independent mount flags start out using decimal then switch to 1 << n
at 32768.

Re: [PATCH] nfsd4: permit read opens of executable-only files

[Jim Rees]

J. Bruce Fields wrote:

  commit a9fd873383e3affa5ba7017713fa46949147d418
  Author: J. Bruce Fields <bfields@redhat.com>
  Date:   Thu Aug 25 14:23:39 2011 -0400
  
      nfsd: prettify NFSD_MAY_* flag definitions
      
      Cc: Jim Rees <rees@umich.edu>
      Signed-off-by: J. Bruce Fields <bfields@redhat.com>

Acked-By: Jim Rees <rees@umich.edu>

Re: [PATCH] nfsd4: permit read opens of executable-only files

[Chris J Arges]

J. Bruce Fields <bfields@...> writes:

> 
> From: J. Bruce Fields <bfields@...>
> 
> A client that wants to execute a file must be able to read it.  Read
> opens over nfs are therefore implicitly allowed for executable files
> even when those files are not readable.
> 
> NFSv2/v3 get this right by using a passed-in NFSD_MAY_OWNER_OVERRIDE on
> read requests, but NFSv4 has gotten this wrong ever since
> dc730e173785e29b297aa605786c94adaffe2544 "nfsd4: fix owner-override on
> open", when we realized that the file owner shouldn't override
> permissions on non-reclaim NFSv4 opens.
> 
> So we can't use NFSD_MAY_OWNER_OVERRIDE to tell nfsd_permission to allow
> reads of executable files.
> 
> So, do the same thing we do whenever we encounter another weird NFS
> permission nit: define yet another NFSD_MAY_* flag.
> 
> The industry's future standardization on 128-bit processors will be
> motivated primarily by the need for integers with enough bits for all
> the NFSD_MAY_* flags.
> 
> Reported-by: Leonardo Borda <leonardoborda@...>
> Cc: stable@...
> Signed-off-by: J. Bruce Fields <bfields@...>
> ---
<snip>


Bruce,

I've tested this patch against linux-3.0 and it doesn't allow me to execute 
binaries with permissions of 111.
Here is the BugLink for the Ubuntu bug: 
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/833300
My method of reproducing the bug is documented here: 
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/833300/comments/22
This is copied and pasted below:
I can reproduce this failure on natty/oneiric just fine.

I then applied the upstream patch (a043226bc140a2c1dde162246d68a67e5043e6b2), 
that was identified in the public bug and upstream bug.

After doing this I installed the tools on my laptop:
  apt-get install nfs-common nfs-kernel-server

Then I setup of /etc/exports as described in the bug to a directory on my laptop 
(ext4).
  /path/to/dir *(rw,sync,fsid=0,no_subtree_check)

I then had to insert the module and restart the server:
  sudo modprobe nfs
  sudo service nfs-kernel-server restart

I mounted the directory:
  sudo mount -t nfs4 -o proto=tcp,port=2049 localhost:/ /mnt

I created a similar c program and compiled. I put this into the shared folder.
I could run it with normal permissions.
I then did 'chmod 111 ./a.out' and then I could not run the program (Permission 
denied) if I was doing it via the /mnt directory.

arges@arges-virtual-machine:/mnt$ ls -la
total 24
drwxr-xr-x 2 4294967294 4294967294 4096 2011-11-17 10:12 .
drwxr-xr-x 22 root root 4096 2011-11-17 09:57 ..
---x--x--x 1 4294967294 4294967294 8425 2011-11-17 10:12 a.out
-rw-r--r-- 1 4294967294 4294967294 73 2011-11-17 10:12 hello.c
arges@arges-virtual-machine:/mnt$ ./a.out
-bash: ./a.out: Permission denied
arges@arges-virtual-machine:/mnt$ mount -v
/dev/vda1 on / type ext4 (rw,errors=remount-ro,commit=0)
<snip>
nfsd on /proc/fs/nfsd type nfsd (rw)
localhost:/ on /mnt type nfs (rw,vers=4,addr=127.0.0.1,clientaddr=127.0.0.1)

I could run it if I wasn't running it over the nfsv4 share (sanity check):

arges@arges-virtual-machine:/mnt$ cd ~/Test/
arges@arges-virtual-machine:~/Test$ ls -la
total 24
drwxr-xr-x 2 arges arges 4096 2011-11-17 10:12 .
drwxr-xr-x 22 arges arges 4096 2011-11-17 10:10 ..
---x--x--x 1 arges arges 8425 2011-11-17 10:12 a.out
-rw-r--r-- 1 arges arges 73 2011-11-17 10:12 hello.c
arges@arges-virtual-machine:~/Test$ ./a.out
hello binary



Thanks,
--chris j arges

Re: [PATCH] nfsd4: permit read opens of executable-only files

[J. Bruce Fields]

On Wed, Dec 07, 2011 at 10:42:32PM +0000, Chris J Arges wrote:
> J. Bruce Fields <bfields@...> writes:
> 
> > 
> > From: J. Bruce Fields <bfields@...>
> > 
> > A client that wants to execute a file must be able to read it.  Read
> > opens over nfs are therefore implicitly allowed for executable files
> > even when those files are not readable.
> > 
> > NFSv2/v3 get this right by using a passed-in NFSD_MAY_OWNER_OVERRIDE on
> > read requests, but NFSv4 has gotten this wrong ever since
> > dc730e173785e29b297aa605786c94adaffe2544 "nfsd4: fix owner-override on
> > open", when we realized that the file owner shouldn't override
> > permissions on non-reclaim NFSv4 opens.
> > 
> > So we can't use NFSD_MAY_OWNER_OVERRIDE to tell nfsd_permission to allow
> > reads of executable files.
> > 
> > So, do the same thing we do whenever we encounter another weird NFS
> > permission nit: define yet another NFSD_MAY_* flag.
> > 
> > The industry's future standardization on 128-bit processors will be
> > motivated primarily by the need for integers with enough bits for all
> > the NFSD_MAY_* flags.
> > 
> > Reported-by: Leonardo Borda <leonardoborda@...>
> > Cc: stable@...
> > Signed-off-by: J. Bruce Fields <bfields@...>
> > ---
> <snip>
> 
> 
> Bruce,
> 
> I've tested this patch against linux-3.0 and it doesn't allow me to execute 
> binaries with permissions of 111.

Hm, I see the same permissions error.  However, looking at the
client-server traffic with wireshark, I see no permissions failures from
the server: the read-open of cat succeeds.  (Could you check if the same
is true in your case?)

So my first inclination is to blame the client.... Does this work with
an older client?

--b.

> Here is the BugLink for the Ubuntu bug: 
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/833300
> My method of reproducing the bug is documented here: 
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/833300/comments/22
> This is copied and pasted below:
> I can reproduce this failure on natty/oneiric just fine.
> 
> I then applied the upstream patch (a043226bc140a2c1dde162246d68a67e5043e6b2), 
> that was identified in the public bug and upstream bug.
> 
> After doing this I installed the tools on my laptop:
>   apt-get install nfs-common nfs-kernel-server
> 
> Then I setup of /etc/exports as described in the bug to a directory on my laptop 
> (ext4).
>   /path/to/dir *(rw,sync,fsid=0,no_subtree_check)
> 
> I then had to insert the module and restart the server:
>   sudo modprobe nfs
>   sudo service nfs-kernel-server restart
> 
> I mounted the directory:
>   sudo mount -t nfs4 -o proto=tcp,port=2049 localhost:/ /mnt
> 
> I created a similar c program and compiled. I put this into the shared folder.
> I could run it with normal permissions.
> I then did 'chmod 111 ./a.out' and then I could not run the program (Permission 
> denied) if I was doing it via the /mnt directory.
> 
> arges@arges-virtual-machine:/mnt$ ls -la
> total 24
> drwxr-xr-x 2 4294967294 4294967294 4096 2011-11-17 10:12 .
> drwxr-xr-x 22 root root 4096 2011-11-17 09:57 ..
> ---x--x--x 1 4294967294 4294967294 8425 2011-11-17 10:12 a.out
> -rw-r--r-- 1 4294967294 4294967294 73 2011-11-17 10:12 hello.c
> arges@arges-virtual-machine:/mnt$ ./a.out
> -bash: ./a.out: Permission denied
> arges@arges-virtual-machine:/mnt$ mount -v
> /dev/vda1 on / type ext4 (rw,errors=remount-ro,commit=0)
> <snip>
> nfsd on /proc/fs/nfsd type nfsd (rw)
> localhost:/ on /mnt type nfs (rw,vers=4,addr=127.0.0.1,clientaddr=127.0.0.1)
> 
> I could run it if I wasn't running it over the nfsv4 share (sanity check):
> 
> arges@arges-virtual-machine:/mnt$ cd ~/Test/
> arges@arges-virtual-machine:~/Test$ ls -la
> total 24
> drwxr-xr-x 2 arges arges 4096 2011-11-17 10:12 .
> drwxr-xr-x 22 arges arges 4096 2011-11-17 10:10 ..
> ---x--x--x 1 arges arges 8425 2011-11-17 10:12 a.out
> -rw-r--r-- 1 arges arges 73 2011-11-17 10:12 hello.c
> arges@arges-virtual-machine:~/Test$ ./a.out
> hello binary
> 
> 
> 
> Thanks,
> --chris j arges
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] nfsd4: permit read opens of executable-only files

[Chris J Arges]

<snip>
> > 
> > 
> > Bruce,
> > 
> > I've tested this patch against linux-3.0 and it doesn't allow me to execute 
> > binaries with permissions of 111.
> 
> Hm, I see the same permissions error.  However, looking at the
> client-server traffic with wireshark, I see no permissions failures from
> the server: the read-open of cat succeeds.  (Could you check if the same
> is true in your case?)
> 
> So my first inclination is to blame the client.... Does this work with
> an older client?
> 
> --b.

Bruce,

Using the above test setup, and trying various clients I see a mismatch:

Using a newer nfs clients (nfs-common 1:1.2.2-4/1:1.2.4-1), I can read a file 
with 111 permissions, but cannot execute it.
With an older nfs client (nfs-common 1:1.2.0-4 / ubuntu lucid), I can read and 
execute a file with 111 permissions.

--chris j arges

<snip>

Re: [PATCH] nfsd4: permit read opens of executable-only files

[J. Bruce Fields]

Could you leave me on the cc: ?

Also, Trond: what did we end up deciding to do about permissions
checking on execute?  Was there a bugfix on the client side?

On Tue, Dec 13, 2011 at 05:38:54PM +0000, Chris J Arges wrote:
> <snip>
> > > 
> > > 
> > > Bruce,
> > > 
> > > I've tested this patch against linux-3.0 and it doesn't allow me to execute 
> > > binaries with permissions of 111.
> > 
> > Hm, I see the same permissions error.  However, looking at the
> > client-server traffic with wireshark, I see no permissions failures from
> > the server: the read-open of cat succeeds.  (Could you check if the same
> > is true in your case?)
> > 
> > So my first inclination is to blame the client.... Does this work with
> > an older client?
> > 
> > --b.
> 
> Bruce,
> 
> Using the above test setup, and trying various clients I see a mismatch:
> 
> Using a newer nfs clients (nfs-common 1:1.2.2-4/1:1.2.4-1), I can read a file 
> with 111 permissions, but cannot execute it.
> With an older nfs client (nfs-common 1:1.2.0-4 / ubuntu lucid), I can read and 
> execute a file with 111 permissions.

It certainly sounds like a client-side error....  (Though if you could
take a look at the traffic in wireshark as suggested above, that would
help--it doesn't require much special expertise, just look for an OPEN
call that mentions the file in question, and see if the server replies
with an error or not.)

Note it's the kernel on the client that matters, not the nfs-utils
version.  And most useful for people on this list may be testing with
the latest upstream kernel.  (We aren't necessarily familiar with Ubuntu
kernel versions.)

--b.