NetBSD Problem Report #51595

From wiz@yt.nih.at  Thu Nov  3 13:21:37 2016
Return-Path: <wiz@yt.nih.at>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.netbsd.org", Issuer "Postmaster NetBSD.org" (verified OK))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 27F977A10E
	for <gnats-bugs@gnats.NetBSD.org>; Thu,  3 Nov 2016 13:21:37 +0000 (UTC)
Message-Id: <20161103120418.DB91A2AC0E1@yt.nih.at>
Date: Thu,  3 Nov 2016 13:04:18 +0100 (CET)
From: Thomas Klausner <wiz@NetBSD.org>
Reply-To: Thomas Klausner <wiz@NetBSD.org>
To: gnats-bugs@NetBSD.org
Subject: inconsistent behaviour in statvfs
X-Send-Pr-Version: 3.95

>Number:         51595
>Category:       kern
>Synopsis:       inconsistent behaviour in statvfs
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Nov 03 13:25:00 +0000 2016
>Last-Modified:  Mon Nov 21 15:40:00 +0000 2016
>Originator:     Thomas Klausner
>Release:        NetBSD 7.99.42
>Organization:
Curiosity is the very basis of education and if you tell me that 
curiosity killed the cat, I say only that the cat died nobly.
- Arnold Edinborough
>Environment:


Architecture: x86_64
Machine: amd64
>Description:
In PR 42484 I reported a problem that I originally thought was caused by
tmpfs. With debugging help from hannken@ we found out that the issue stems
from the sandbox setup, in particular:

My pbulk sandboxes are below /archive/sandboxes, which has permissions
drwxr-x---  9 root  wheel  512 Aug 28 23:36 /archive/sandboxes

The problems seem to appear when the pbulk user (which is not a
member of wheel) inside a chroot calls statvfs.

kern/vfs_getcwd.c::getcwd_common() has inconsistent behaviour
when called without "flags & GETCWD_CHECK_ACCESS".

In that case, the directory permissions are only checked when
the entry is not in the cache.

hannken suggested a patch:
Index: kern/vfs_getcwd.c
===================================================================
RCS file: /cvsroot/src/sys/kern/vfs_getcwd.c,v
retrieving revision 1.50
diff -p -u -2 -r1.50 vfs_getcwd.c
--- kern/vfs_getcwd.c   7 Feb 2014 15:29:22 -0000       1.50
+++ kern/vfs_getcwd.c   31 Oct 2016 17:04:02 -0000
@@ -128,5 +128,5 @@ getcwd_scandir(struct vnode **lvpp, stru
        cn.cn_nameiop = LOOKUP;
        cn.cn_flags = ISLASTCN | ISDOTDOT | RDONLY;
-       cn.cn_cred = cred;
+       cn.cn_cred = NOCRED;
        cn.cn_nameptr = "..";
        cn.cn_namelen = 2;
Index: miscfs/genfs/genfs_vnops.c
===================================================================
RCS file: /cvsroot/src/sys/miscfs/genfs/genfs_vnops.c,v
retrieving revision 1.192
diff -p -u -2 -r1.192 genfs_vnops.c
--- miscfs/genfs/genfs_vnops.c  24 Mar 2014 13:42:40 -0000      1.192
+++ miscfs/genfs/genfs_vnops.c  31 Oct 2016 17:04:02 -0000
@@ -666,4 +666,8 @@ genfs_can_access(enum vtype type, mode_t
        int error, ismember;

+       /* Short-circuit requests coming from the kernel. */
+       if (cred == NOCRED || cred == FSCRED)
+               return 0;
+
        mask = 0;

but this breaks nfs:

panic: kernel diagnostic assertion "cred != NOCRED" failed: file "/usr/src/sys/kern/kern_auth.c", line 405
cpu7: Begin traceback...
vpanic() at netbsd:vpanic+0x140
cd_play_msf() at netbsd:cd_play_msf
kauth_cred_ngroups() at netbsd:kauth_cred_ngroups+0x83
nfs_request() at netbsd:nfs_request+0x1da
nfs_lookup() at netbsd:nfs_lookup+0x41c
VOP_LOOKUP() at netbsd:VOP_LOOKUP+0xa8
getcwd_scandir() at netbsd:getcwd_scandir+0x9f
getcwd_common() at netbsd:getcwd_common+0x213
sys___getcwd() at netbsd:sys___getcwd+0xaf
syscall() at netbsd:syscall+0x15b
--- syscall (number 296) ---
7dcef1aa755a:
cpu7: End traceback...

since nfs needs a valid user for permissions checking on the server.

>How-To-Repeat:
Have a chroot below a path that is not readable by a user, build
packages as that user, wait for a combination of effects to meet up.
(quite reliably happens for me in a libreoffice and/or firefox bulk
build).
>Fix:
Possibly: save the prefix of the chroot in 'struct cwdinfo'

Workaround: allow the user permissions on all parts of the path
outside the chroot.

>Audit-Trail:
From: christos@zoulas.com (Christos Zoulas)
To: gnats-bugs@NetBSD.org, kern-bug-people@netbsd.org, 
	gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Cc: 
Subject: Re: kern/51595: inconsistent behaviour in statvfs
Date: Thu, 3 Nov 2016 10:05:16 -0400

 On Nov 3,  1:25pm, wiz@NetBSD.org (Thomas Klausner) wrote:
 -- Subject: kern/51595: inconsistent behaviour in statvfs


 Please explain what happens. I.e. what it returns in the error case
 and what it should return.

 christos

From: Thomas Klausner <wiz@NetBSD.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/51595: inconsistent behaviour in statvfs
Date: Thu, 3 Nov 2016 15:17:26 +0100

 On Thu, Nov 03, 2016 at 02:10:00PM +0000, Christos Zoulas wrote:
 >  Please explain what happens. I.e. what it returns in the error case
 >  and what it should return.

 IIUC:

 kern/vfs_getcwd.c in getcwd_scandir calls VOP_LOOKUP for ".." to strip
 off the chroot path from paths returned to programs inside the chroot.
 This fails with EACCES, but should succeed.

 The reason is that in some cases, this is called with the pbulk user
 credentials and fails, because pbulk cannot enter /archive/sandboxes;
 in some cases it was previously called in another way with root
 credentials, and the call originating from the pbulk user uses the
 cached entry and everything works fine.

  Thomas

From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/51595: inconsistent behaviour in statvfs
Date: Mon, 21 Nov 2016 07:26:43 +0000

 On Thu, Nov 03, 2016 at 02:20:01PM +0000, Thomas Klausner wrote:
  > From: Thomas Klausner <wiz@NetBSD.org>
  > To: gnats-bugs@NetBSD.org
  > Cc: 
  > Subject: Re: kern/51595: inconsistent behaviour in statvfs
  > Date: Thu, 3 Nov 2016 15:17:26 +0100
  > 
  >  On Thu, Nov 03, 2016 at 02:10:00PM +0000, Christos Zoulas wrote:
  >  >  Please explain what happens. I.e. what it returns in the error case
  >  >  and what it should return.
  >  
  >  IIUC:
  >  
  >  kern/vfs_getcwd.c in getcwd_scandir calls VOP_LOOKUP for ".." to strip
  >  off the chroot path from paths returned to programs inside the chroot.
  >  This fails with EACCES, but should succeed.
  >  
  >  The reason is that in some cases, this is called with the pbulk user
  >  credentials and fails, because pbulk cannot enter /archive/sandboxes;
  >  in some cases it was previously called in another way with root
  >  credentials, and the call originating from the pbulk user uses the
  >  cached entry and everything works fine.

 getcwd *should* fail if inside a directory where read (or search)
 permission is missing.

 Don't patch it to ignore the credentials; make it check in both
 cases. Having it succeed some of the time is a bug. (Technically a
 security bug, but not one anyone is likely to care that much about...)

 The use of getcwd in statvfs is going to have to take account of this
 better, but probably a good starting point is to just give EACCES. And
 yeah, that means things like "df ." will fail with EACCES, which is
 why it's only a starting point...

 -- 
 David A. Holland
 dholland@netbsd.org

From: "J. Hannken-Illjes" <hannken@eis.cs.tu-bs.de>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/51595: inconsistent behaviour in statvfs
Date: Mon, 21 Nov 2016 11:10:54 +0100

 > On 21 Nov 2016, at 08:30, David Holland <dholland-bugs@netbsd.org> wrote:
 > 
 > The following reply was made to PR kern/51595; it has been noted by GNATS.
 > 
 > From: David Holland <dholland-bugs@netbsd.org>
 > To: gnats-bugs@NetBSD.org
 > Cc: 
 > Subject: Re: kern/51595: inconsistent behaviour in statvfs
 > Date: Mon, 21 Nov 2016 07:26:43 +0000
 > 
 > On Thu, Nov 03, 2016 at 02:20:01PM +0000, Thomas Klausner wrote:
 >> From: Thomas Klausner <wiz@NetBSD.org>
 >> To: gnats-bugs@NetBSD.org
 >> Cc: 
 >> Subject: Re: kern/51595: inconsistent behaviour in statvfs
 >> Date: Thu, 3 Nov 2016 15:17:26 +0100
 >> 
 >> On Thu, Nov 03, 2016 at 02:10:00PM +0000, Christos Zoulas wrote:
 >>> Please explain what happens. I.e. what it returns in the error case
 >>> and what it should return.
 >> 
 >> IIUC:
 >> 
 >> kern/vfs_getcwd.c in getcwd_scandir calls VOP_LOOKUP for ".." to strip
 >> off the chroot path from paths returned to programs inside the chroot.
 >> This fails with EACCES, but should succeed.
 >> 
 >> The reason is that in some cases, this is called with the pbulk user
 >> credentials and fails, because pbulk cannot enter /archive/sandboxes;
 >> in some cases it was previously called in another way with root
 >> credentials, and the call originating from the pbulk user uses the
 >> cached entry and everything works fine.
 > 
 > getcwd *should* fail if inside a directory where read (or search)
 > permission is missing.

 This is the current behaviour.  It calls getcwd_common() with flag
 GETCWD_CHECK_ACCESS.

 > Don't patch it to ignore the credentials; make it check in both
 > cases. Having it succeed some of the time is a bug. (Technically a
 > security bug, but not one anyone is likely to care that much about...)
 > 
 > The use of getcwd in statvfs is going to have to take account of this
 > better, but probably a good starting point is to just give EACCES. And
 > yeah, that means things like "df ." will fail with EACCES, which is
 > why it's only a starting point...

 To make it clear: statvfs uses getcwd_common() to lookup the path
 leading to a chroot so it can strip this path from the mount path
 seen from inside the chroot.  This lookup should always succeed or
 the path leading to the chroot should be saved on chroot setup.

 --
 J. Hannken-Illjes - hannken@eis.cs.tu-bs.de - TU Braunschweig (Germany)

From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/51595: inconsistent behaviour in statvfs
Date: Mon, 21 Nov 2016 15:36:50 +0000

 On Mon, Nov 21, 2016 at 10:15:01AM +0000, J. Hannken-Illjes wrote:
  >  > getcwd *should* fail if inside a directory where read (or search)
  >  > permission is missing.
  >  
  >  This is the current behaviour.  It calls getcwd_common() with flag
  >  GETCWD_CHECK_ACCESS.

 The original patch in the PR bluntly changes the getcwd internals to
 use NOCRED. Are we sure that the GETCWD_CHECK_ACCESS call is
 equivalent to (or greater than) the calls that would circumvent?
 (yes, I missed the existence of that call last night)

 It seems then like the problem is that the logic to force getcwd to
 succeed regardless of permissions is missing some stuff and we need
 a flag for it in lookup (no! no! gah) and/or readdir.

  >  > Don't patch it to ignore the credentials; make it check in both
  >  > cases. Having it succeed some of the time is a bug. (Technically a
  >  > security bug, but not one anyone is likely to care that much about...)
  >  > 
  >  > The use of getcwd in statvfs is going to have to take account of this
  >  > better, but probably a good starting point is to just give EACCES. And
  >  > yeah, that means things like "df ." will fail with EACCES, which is
  >  > why it's only a starting point...
  >  
  >  To make it clear: statvfs uses getcwd_common() to lookup the path
  >  leading to a chroot so it can strip this path from the mount path
  >  seen from inside the chroot.  This lookup should always succeed or
  >  the path leading to the chroot should be saved on chroot setup.

 You can't save the path because it can change. (In theory one could
 instrument rename to update the saved path or something, but in
 practice this is not very feasible.)

 You can't use NOCRED because nfs will blow up; for that matter,
 because of rootsquash you can't even necessarily force the lookup to
 succeed on nfs.

 One could hack nfs to try harder if passed the force flag that doesn't
 currently exist. That would probably cover most of the interesting
 caes (e.g. have it try the current user first, then try with
 manufactured creds using the owner of the directory, then as uid 0)
 but will be quite a pain. And it still won't work all the time.

 I guess the best solution in the short term is to save the path and
 assume that a sysadmin who moves active chroots around deserves the
 (relatively minor) consequences. Then try sometime in the future to
 figure out a better way to manage the high-level properties of the fs
 namespace.

 -- 
 David A. Holland
 dholland@netbsd.org

>Unformatted:

Home	PR Database Search