Occasionally we experience EACCES errors which are caused by the exportfs
reconnect_path() function called by the NFS server (NFSv3). The following
reproduces it reliably on the client.
Compile cd-droppriv.c and make it setuid root:
--------
#include <stdio.h>
#include <stdarg.h>
#include <unistd.h>
#include <string.h>
#include <stdlib.h>
#include <errno.h>
#include <dirent.h>
void die(const char *fmt, ...) __attribute__((format(printf, 1, 2), noreturn));
void die(const char *fmt, ...)
{
va_list ap;
va_start(ap, fmt);
fprintf(stderr, "cd-droppriv: ");
vfprintf(stderr, fmt, ap);
va_end(ap);
exit(1);
}
int main(int argc, char **argv)
{
if (chdir(argv[1]) == -1)
die("chdir: %s\n", strerror(errno));
if (setuid(getuid()) == -1)
die("setuid: %s\n", strerror(errno));
printf("Restart the NFS server then press <enter>.\n");
getchar();
if (opendir(".") == NULL)
die("opendir: %s\n", strerror(errno));
return 0;
}
--------
As root, create a directory tree on the client. The export options on
the server for /mnt include no_root_squash and no_subtree_check:
mkdir -p /mnt/a/b
chmod 700 /mnt/a
chmod 777 /mnt/a/b
Run the program as non-root on the client:
cd-droppriv /mnt/a/b
and press <enter>. When the server is restarted before pressing <enter>
opendir() fails with EACCES:
cd-droppriv: opendir: Permission denied
This happens too when dentries are dropped on the server due to memory
pressure. The following seems to fix the problem (2.6.24.4):
--- ./fs/exportfs/expfs.c.orig 2008-02-04 14:24:21.000000000 +0100
+++ ./fs/exportfs/expfs.c 2008-04-03 18:00:20.000000000 +0200
@@ -170,7 +170,7 @@ reconnect_path(struct vfsmount *mnt, str
}
dprintk("%s: found name: %s\n", __FUNCTION__, nbuf);
mutex_lock(&ppd->d_inode->i_mutex);
- npd = lookup_one_len(nbuf, ppd, strlen(nbuf));
+ npd = lookup_one_noperm(nbuf, ppd);
mutex_unlock(&ppd->d_inode->i_mutex);
if (IS_ERR(npd)) {
err = PTR_ERR(npd);
@@ -447,8 +447,7 @@ struct dentry *exportfs_decode_fh(struct
err = exportfs_get_name(mnt, target_dir, nbuf, result);
if (!err) {
mutex_lock(&target_dir->d_inode->i_mutex);
- nresult = lookup_one_len(nbuf, target_dir,
- strlen(nbuf));
+ nresult = lookup_one_noperm(nbuf, target_dir);
mutex_unlock(&target_dir->d_inode->i_mutex);
if (!IS_ERR(nresult)) {
if (nresult->d_inode) {
--
Frank
On Mon, Apr 07, 2008 at 02:43:46PM -0400, J. Bruce Fields wrote:
> Anyone who depends on the "x" bit to control access to objects in an
> nfs-exported filesystem is already in trouble. We could do so for
> directories (at the expense of non-posix-like behavior such as what
> you've seen), but we probably can't for files. So I'm inclined to think
> this is the right thing to do.
>
> The "DON'T USE THIS FUNCTION EVER, thanks." suggests we should at least
> consult the person who added that comment (cc'd) before adding a call to
> lookup_one_noperm(). (And if we decide to do this, we should make a
> note of this in that comment.)
That function really shouldn't be used and we should obey the x bit.
And yes, due to NFSs staleless file handles this will lead to non-posix
behaviour which is expected. The same will happen with other nfs
servers aswell.
On Wed, Apr 09, 2008 at 03:36:39PM +0200, Christoph Hellwig wrote:
> On Mon, Apr 07, 2008 at 02:43:46PM -0400, J. Bruce Fields wrote:
> > Anyone who depends on the "x" bit to control access to objects in an
> > nfs-exported filesystem is already in trouble. We could do so for
> > directories (at the expense of non-posix-like behavior such as what
> > you've seen), but we probably can't for files. So I'm inclined to think
> > this is the right thing to do.
> >
> > The "DON'T USE THIS FUNCTION EVER, thanks." suggests we should at least
> > consult the person who added that comment (cc'd) before adding a call to
> > lookup_one_noperm(). (And if we decide to do this, we should make a
> > note of this in that comment.)
>
> That function really shouldn't be used and we should obey the x bit.
> And yes, due to NFSs staleless file handles this will lead to non-posix
> behaviour which is expected. The same will happen with other nfs
> servers aswell.
The server exhibits non-posix behavior only occasionally and particularly
this "randomness" is bad. Random failure once every few days is hard to
figure out.
Whether we want to be posix compliant with directory handles is a separate
issue but I think we should. And we (mostly) are.
The proposed patch fixes the linux NFS server behavior to be consistent
over reboots and high memory pressure. It's not a security issue so I
don't see a downside.
--
Frank
On Wed, Apr 09, 2008 at 03:36:39PM +0200, Christoph Hellwig wrote:
> On Mon, Apr 07, 2008 at 02:43:46PM -0400, J. Bruce Fields wrote:
> > Anyone who depends on the "x" bit to control access to objects in an
> > nfs-exported filesystem is already in trouble. We could do so for
> > directories (at the expense of non-posix-like behavior such as what
> > you've seen), but we probably can't for files. So I'm inclined to think
> > this is the right thing to do.
> >
> > The "DON'T USE THIS FUNCTION EVER, thanks." suggests we should at least
> > consult the person who added that comment (cc'd) before adding a call to
> > lookup_one_noperm(). (And if we decide to do this, we should make a
> > note of this in that comment.)
>
> That function really shouldn't be used and we should obey the x bit.
> And yes, due to NFSs staleless file handles this will lead to non-posix
> behaviour which is expected. The same will happen with other nfs
> servers aswell.
Any references for that behavior? Well, we can do some tests, I guess.
--b.
Thanks for the patch and the nice test case....
On Fri, Apr 04, 2008 at 12:24:49PM +0200, Frank van Maarseveen wrote:
> Occasionally we experience EACCES errors which are caused by the exportfs
> reconnect_path() function called by the NFS server (NFSv3). The following
> reproduces it reliably on the client.
OK, so you're trying to use a directory that you previously descended
into, but that you no longer have the right to look up?
>
> Compile cd-droppriv.c and make it setuid root:
> --------
> #include <stdio.h>
> #include <stdarg.h>
> #include <unistd.h>
> #include <string.h>
> #include <stdlib.h>
> #include <errno.h>
> #include <dirent.h>
>
> void die(const char *fmt, ...) __attribute__((format(printf, 1, 2), noreturn));
> void die(const char *fmt, ...)
> {
> va_list ap;
>
> va_start(ap, fmt);
> fprintf(stderr, "cd-droppriv: ");
> vfprintf(stderr, fmt, ap);
> va_end(ap);
> exit(1);
> }
>
> int main(int argc, char **argv)
> {
> if (chdir(argv[1]) == -1)
> die("chdir: %s\n", strerror(errno));
> if (setuid(getuid()) == -1)
> die("setuid: %s\n", strerror(errno));
> printf("Restart the NFS server then press <enter>.\n");
> getchar();
> if (opendir(".") == NULL)
> die("opendir: %s\n", strerror(errno));
> return 0;
> }
> --------
>
> As root, create a directory tree on the client. The export options on
> the server for /mnt include no_root_squash and no_subtree_check:
>
> mkdir -p /mnt/a/b
> chmod 700 /mnt/a
> chmod 777 /mnt/a/b
>
> Run the program as non-root on the client:
>
> cd-droppriv /mnt/a/b
>
> and press <enter>. When the server is restarted before pressing <enter>
> opendir() fails with EACCES:
>
> cd-droppriv: opendir: Permission denied
>
> This happens too when dentries are dropped on the server due to memory
> pressure. The following seems to fix the problem (2.6.24.4):
>
> --- ./fs/exportfs/expfs.c.orig 2008-02-04 14:24:21.000000000 +0100
> +++ ./fs/exportfs/expfs.c 2008-04-03 18:00:20.000000000 +0200
> @@ -170,7 +170,7 @@ reconnect_path(struct vfsmount *mnt, str
> }
> dprintk("%s: found name: %s\n", __FUNCTION__, nbuf);
> mutex_lock(&ppd->d_inode->i_mutex);
> - npd = lookup_one_len(nbuf, ppd, strlen(nbuf));
> + npd = lookup_one_noperm(nbuf, ppd);
Anyone who depends on the "x" bit to control access to objects in an
nfs-exported filesystem is already in trouble. We could do so for
directories (at the expense of non-posix-like behavior such as what
you've seen), but we probably can't for files. So I'm inclined to think
this is the right thing to do.
The "DON'T USE THIS FUNCTION EVER, thanks." suggests we should at least
consult the person who added that comment (cc'd) before adding a call to
lookup_one_noperm(). (And if we decide to do this, we should make a
note of this in that comment.)
Also, I'm curious: could you explain how you hit this In Real Life,
before you made this test case?
--b.
> mutex_unlock(&ppd->d_inode->i_mutex);
> if (IS_ERR(npd)) {
> err = PTR_ERR(npd);
> @@ -447,8 +447,7 @@ struct dentry *exportfs_decode_fh(struct
> err = exportfs_get_name(mnt, target_dir, nbuf, result);
> if (!err) {
> mutex_lock(&target_dir->d_inode->i_mutex);
> - nresult = lookup_one_len(nbuf, target_dir,
> - strlen(nbuf));
> + nresult = lookup_one_noperm(nbuf, target_dir);
> mutex_unlock(&target_dir->d_inode->i_mutex);
> if (!IS_ERR(nresult)) {
> if (nresult->d_inode) {
>
> --
> Frank
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 07, 2008 at 02:43:46PM -0400, J. Bruce Fields wrote:
> Thanks for the patch and the nice test case....
>
> On Fri, Apr 04, 2008 at 12:24:49PM +0200, Frank van Maarseveen wrote:
> > Occasionally we experience EACCES errors which are caused by the exportfs
> > reconnect_path() function called by the NFS server (NFSv3). The following
> > reproduces it reliably on the client.
>
> OK, so you're trying to use a directory that you previously descended
> into, but that you no longer have the right to look up?
Correct. Based on identity and permission bits the process should have
access but it no longer has.
>
> >
> > Compile cd-droppriv.c and make it setuid root:
> > --------
> > #include <stdio.h>
> > #include <stdarg.h>
> > #include <unistd.h>
> > #include <string.h>
> > #include <stdlib.h>
> > #include <errno.h>
> > #include <dirent.h>
> >
> > void die(const char *fmt, ...) __attribute__((format(printf, 1, 2), noreturn));
> > void die(const char *fmt, ...)
> > {
> > va_list ap;
> >
> > va_start(ap, fmt);
> > fprintf(stderr, "cd-droppriv: ");
> > vfprintf(stderr, fmt, ap);
> > va_end(ap);
> > exit(1);
> > }
> >
> > int main(int argc, char **argv)
> > {
> > if (chdir(argv[1]) == -1)
> > die("chdir: %s\n", strerror(errno));
> > if (setuid(getuid()) == -1)
> > die("setuid: %s\n", strerror(errno));
> > printf("Restart the NFS server then press <enter>.\n");
> > getchar();
> > if (opendir(".") == NULL)
> > die("opendir: %s\n", strerror(errno));
> > return 0;
> > }
> > --------
> >
> > As root, create a directory tree on the client. The export options on
> > the server for /mnt include no_root_squash and no_subtree_check:
> >
> > mkdir -p /mnt/a/b
> > chmod 700 /mnt/a
> > chmod 777 /mnt/a/b
> >
> > Run the program as non-root on the client:
> >
> > cd-droppriv /mnt/a/b
> >
> > and press <enter>. When the server is restarted before pressing <enter>
> > opendir() fails with EACCES:
> >
> > cd-droppriv: opendir: Permission denied
> >
> > This happens too when dentries are dropped on the server due to memory
> > pressure. The following seems to fix the problem (2.6.24.4):
> >
> > --- ./fs/exportfs/expfs.c.orig 2008-02-04 14:24:21.000000000 +0100
> > +++ ./fs/exportfs/expfs.c 2008-04-03 18:00:20.000000000 +0200
> > @@ -170,7 +170,7 @@ reconnect_path(struct vfsmount *mnt, str
> > }
> > dprintk("%s: found name: %s\n", __FUNCTION__, nbuf);
> > mutex_lock(&ppd->d_inode->i_mutex);
> > - npd = lookup_one_len(nbuf, ppd, strlen(nbuf));
> > + npd = lookup_one_noperm(nbuf, ppd);
>
> Anyone who depends on the "x" bit to control access to objects in an
> nfs-exported filesystem is already in trouble. We could do so for
> directories (at the expense of non-posix-like behavior such as what
> you've seen), but we probably can't for files. So I'm inclined to think
> this is the right thing to do.
Several file access issues have been fixed in the past. Redirecting
output of setuid-root programs (X server) when squash_root is in effect
and paging in execute-only files over NFS comes to my mind.
>
> The "DON'T USE THIS FUNCTION EVER, thanks." suggests we should at least
> consult the person who added that comment (cc'd) before adding a call to
> lookup_one_noperm(). (And if we decide to do this, we should make a
> note of this in that comment.)
yes.
>
> Also, I'm curious: could you explain how you hit this In Real Life,
> before you made this test case?
The NFS patch to deal with >16 groups on NFSv3 with AUTH_SYS I maintain
on http://www.frankvm.com/nfs-ngroups hit this problem first. Interactive shells
became unusable after a nightly run server mirroring script expelled
a lot of dentries from memory. The nfs-ngroups patch makes the client
picky about which groups to use in NFS requests in order to avoid the
16 groups limit.
>
> --b.
>
> > mutex_unlock(&ppd->d_inode->i_mutex);
> > if (IS_ERR(npd)) {
> > err = PTR_ERR(npd);
> > @@ -447,8 +447,7 @@ struct dentry *exportfs_decode_fh(struct
> > err = exportfs_get_name(mnt, target_dir, nbuf, result);
> > if (!err) {
> > mutex_lock(&target_dir->d_inode->i_mutex);
> > - nresult = lookup_one_len(nbuf, target_dir,
> > - strlen(nbuf));
> > + nresult = lookup_one_noperm(nbuf, target_dir);
> > mutex_unlock(&target_dir->d_inode->i_mutex);
> > if (!IS_ERR(nresult)) {
> > if (nresult->d_inode) {
> >
> > --
> > Frank
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Frank
On Tuesday April 29, [email protected] wrote:
>
> Just to make sure I understand--you're not claiming that there's an
> actual threat of corrupting the on-disk filesystem or in-core data
> structures, right?
Correct. I'm not claiming that.
NeilBrown