NetBSD Problem Report #45700

From campbell@mumble.net  Fri Dec  9 08:22:42 2011
Return-Path: <campbell@mumble.net>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by www.NetBSD.org (Postfix) with ESMTP id 451AB63BB6F
	for <gnats-bugs@gnats.NetBSD.org>; Fri,  9 Dec 2011 08:22:42 +0000 (UTC)
Message-Id: <20111209082241.C335D982AB@pluto.mumble.net>
Date: Fri,  9 Dec 2011 08:22:41 +0000 (UTC)
From: Taylor R Campbell <campbell+netbsd@mumble.net>
Reply-To: Taylor R Campbell <campbell+netbsd@mumble.net>
To: gnats-bugs@gnats.NetBSD.org
Subject: /chroot/proc/mounts exposes out-of-chroot pathnames
X-Send-Pr-Version: 3.95

>Number:         45700
>Category:       kern
>Synopsis:       /chroot/proc/mounts exposes out-of-chroot pathnames
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Dec 09 08:25:00 +0000 2011
>Closed-Date:    Sun Apr 18 20:22:31 +0000 2021
>Last-Modified:  Sun Apr 18 20:22:31 +0000 2021
>Originator:     Taylor R Campbell <campbell+netbsd@mumble.net>
>Release:        NetBSD 5.99.56
>Organization:
>Environment:
System: NetBSD oberon.local 5.99.56 NetBSD 5.99.56 (RIAMONOHACK) #0: Sun Oct 16 07:50:03 UTC 2011 root@oberon.local:/home/riastradh/netbsd/current/obj/sys/arch/i386/compile/RIAMONOHACK i386
Architecture: i386
Machine: i386
>Description:

	If I'm chrooted in /chroot, and I mount procfs on /proc (in the
	chroot), then /proc/mounts exposes pathnames from outside the
	chroot.

>How-To-Repeat:

	# chroot /chroot
	# mount -t procfs procfs /proc
	# cat /proc/mounts

>Fix:

	Yes, please!  It's not clear what the right behaviour is, but
	perhaps a hack similar to ptyfs's would be appropriate here.

>Release-Note:

>Audit-Trail:
From: matthew green <mrg@eterna.com.au>
To: gnats-bugs@NetBSD.org
Cc: kern-bug-people@netbsd.org, gnats-admin@netbsd.org,
    netbsd-bugs@netbsd.org
Subject: re: kern/45700: /chroot/proc/mounts exposes out-of-chroot pathnames
Date: Fri, 09 Dec 2011 20:00:18 +1100

 > Machine: i386
 > >Description:
 > 
 > 	If I'm chrooted in /chroot, and I mount procfs on /proc (in the
 > 	chroot), then /proc/mounts exposes pathnames from outside the
 > 	chroot.
 > 
 > >How-To-Repeat:
 > 
 > 	# chroot /chroot
 > 	# mount -t procfs procfs /proc
 > 	# cat /proc/mounts
 > 
 > >Fix:
 > 
 > 	Yes, please!  It's not clear what the right behaviour is, but
 > 	perhaps a hack similar to ptyfs's would be appropriate here.

 df(1) gets this right.  hopefully we can use what ever it does
 to fix this one...

From: Taylor R Campbell <campbell@mumble.net>
To: matthew green <mrg@eterna.com.au>
Cc: gnats-bugs@NetBSD.org, kern-bug-people@netbsd.org, gnats-admin@netbsd.org,
	netbsd-bugs@netbsd.org, Taylor R Campbell <campbell+netbsd@mumble.net>
Subject: Re: kern/45700: /chroot/proc/mounts exposes out-of-chroot pathnames
Date: Fri, 9 Dec 2011 09:54:36 +0000

    Date: Fri,  9 Dec 2011 09:05:04 +0000 (UTC)
    From: matthew green <mrg@eterna.com.au>

    df(1) gets this right.  hopefully we can use what ever it does
    to fix this one...

 Might work just to make procfs_domounts use dostatvfs rather than
 reading from mp->mnt_stat directly.  I'll look into this at some point
 if nobody beats me to it.

From: Matthew Mondor <mm_lists@pulsar-zone.net>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/45700: /chroot/proc/mounts exposes out-of-chroot pathnames
Date: Fri, 9 Dec 2011 04:58:10 -0500

 >  > 	If I'm chrooted in /chroot, and I mount procfs on /proc (in the
 >  > 	chroot), then /proc/mounts exposes pathnames from outside the
 >  > 	chroot.

 Argh...


 On Fri,  9 Dec 2011 09:05:04 +0000 (UTC)
 matthew green <mrg@eterna.com.au> wrote:

 >  df(1) gets this right.  hopefully we can use what ever it does
 >  to fix this one...

 I seems that df(1) uses getmntinfo(3) which itself uses getvfsstat(2),
 calling do_getvfsstat()->dostatvfs() in sys/kern/vfs_syscalls.c:

                 /*
                  * for mount points that are below our root, we can see
                  * them, so we fix up the pathname and return them. The
                  * rest we cannot see, so we don't allow viewing the
                  * data.
                  */

 Perhaps that this check could be moved into a function shared by both
 dostatvfs() and the procfs code (possibly also other such redundant
 checks elsewhere?); It also would be worth checking if procfs
 Linux-compatibility nodes also have another leak...
 -- 
 Matt

From: Taylor R Campbell <campbell+netbsd@mumble.net>
To: matthew green <mrg@eterna.com.au>
Cc: gnats-bugs@NetBSD.org, kern-bug-people@netbsd.org, gnats-admin@netbsd.org,
	netbsd-bugs@netbsd.org
Subject: Re: kern/45700: /chroot/proc/mounts exposes out-of-chroot pathnames
Date: Sun, 11 Dec 2011 00:13:28 +0000

    Date: Fri, 9 Dec 2011 09:54:36 +0000
    From: Taylor R Campbell <campbell@mumble.net>

    Might work just to make procfs_domounts use dostatvfs rather than
    reading from mp->mnt_stat directly.  I'll look into this at some point
    if nobody beats me to it.

 The following patch seems to almost work.  It doesn't fake an entry
 for the root file system if appropriate.  Not sure why not, but I
 don't want to reboot again at the moment to futz with it; I'll peer
 closer at it later.

 Index: procfs_linux.c
 ===================================================================
 RCS file: /cvsroot/src/sys/miscfs/procfs/procfs_linux.c,v
 retrieving revision 1.61
 diff -p -u -r1.61 procfs_linux.c
 --- procfs_linux.c	4 Sep 2011 17:32:10 -0000	1.61
 +++ procfs_linux.c	11 Dec 2011 00:07:23 -0000
 @@ -54,6 +54,7 @@ __KERNEL_RCSID(0, "$NetBSD: procfs_linux
  #include <sys/mount.h>
  #include <sys/conf.h>
  #include <sys/sysctl.h>
 +#include <sys/filedesc.h>

  #include <miscfs/procfs/procfs.h>

 @@ -556,53 +557,81 @@ out:
  	return error;
  }

 +
 +/*
 + * Append a formatted mount entry for the mount point mp to *mtab
 + * starting at *mtabsz, growing it and adding the length of the entry
 + * to *mtabsz.  sfs is mp's vfs statistics structure; bf is a temporary
 + * buffer of size LBFSZ.
 + */
 +static void
 +procfs_format_mount(struct mount *mp, struct statvfs *sfs, char *bf,
 +    char **mtab, size_t *mtabsz)
 +{
 +	const char *fsname;
 +	size_t len;
 +
 +	/* Linux uses different names for some filesystems.  */
 +	fsname = sfs->f_fstypename;
 +	if (strcmp(fsname, "procfs") == 0)
 +		fsname = "proc";
 +	else if (strcmp(fsname, "ext2fs") == 0)
 +		fsname = "ext2";
 +
 +	len = snprintf(bf, LBFSZ, "%s %s %s %s%s%s%s%s%s 0 0\n",
 +	    sfs->f_mntfromname,
 +	    sfs->f_mntonname,
 +	    fsname,
 +	    (mp->mnt_flag & MNT_RDONLY) ? "ro" : "rw",
 +	    (mp->mnt_flag & MNT_NOSUID) ? ",nosuid" : "",
 +	    (mp->mnt_flag & MNT_NOEXEC) ? ",noexec" : "",
 +	    (mp->mnt_flag & MNT_NODEV) ? ",nodev" : "",
 +	    (mp->mnt_flag & MNT_SYNCHRONOUS) ? ",sync" : "",
 +	    (mp->mnt_flag & MNT_NOATIME) ? ",noatime" : "");
 +
 +	*mtab = realloc(*mtab, *mtabsz + len, M_TEMP, M_WAITOK);
 +	memcpy(*mtab + *mtabsz, bf, len);
 +	*mtabsz += len;
 +}
 +
  int
  procfs_domounts(struct lwp *curl, struct proc *p,
      struct pfsnode *pfs, struct uio *uio)
  {
  	char *bf, *mtab = NULL;
 -	const char *fsname;
 -	size_t len, mtabsz = 0;
 +	size_t mtabsz = 0;
  	struct mount *mp, *nmp;
 -	struct statvfs *sfs;
 +	struct statvfs sfs;
  	int error = 0;
 +	bool root = false;

  	bf = malloc(LBFSZ, M_TEMP, M_WAITOK);
 +
  	mutex_enter(&mountlist_lock);
  	for (mp = CIRCLEQ_FIRST(&mountlist); mp != (void *)&mountlist;
  	     mp = nmp) {
 -		if (vfs_busy(mp, &nmp)) {
 +		if (vfs_busy(mp, &nmp))
  			continue;
 -		}
 -
 -		sfs = &mp->mnt_stat;

 -		/* Linux uses different names for some filesystems */
 -		fsname = sfs->f_fstypename;
 -		if (strcmp(fsname, "procfs") == 0)
 -			fsname = "proc";
 -		else if (strcmp(fsname, "ext2fs") == 0)
 -			fsname = "ext2";
 -
 -		len = snprintf(bf, LBFSZ, "%s %s %s %s%s%s%s%s%s 0 0\n",
 -			sfs->f_mntfromname,
 -			sfs->f_mntonname,
 -			fsname,
 -			(mp->mnt_flag & MNT_RDONLY) ? "ro" : "rw",
 -			(mp->mnt_flag & MNT_NOSUID) ? ",nosuid" : "",
 -			(mp->mnt_flag & MNT_NOEXEC) ? ",noexec" : "",
 -			(mp->mnt_flag & MNT_NODEV) ? ",nodev" : "",
 -			(mp->mnt_flag & MNT_SYNCHRONOUS) ? ",sync" : "",
 -			(mp->mnt_flag & MNT_NOATIME) ? ",noatime" : ""
 -			);
 -
 -		mtab = realloc(mtab, mtabsz + len, M_TEMP, M_WAITOK);
 -		memcpy(mtab + mtabsz, bf, len);
 -		mtabsz += len;
 +		if (dostatvfs(mp, &sfs, curl, MNT_NOWAIT, 0) == 0) {
 +			procfs_format_mount(mp, &sfs, bf, &mtab, &mtabsz);
 +			root |= (strcmp(sfs.f_mntonname, "/") == 0);
 +		}

  		vfs_unbusy(mp, false, &nmp);
  	}
  	mutex_exit(&mountlist_lock);
 +
 +	/*
 +	 * If we are inside a chroot that is not itself a mount point,
 +	 * fake a root entry.
 +	 */
 +	if (!root && p->p_cwdi->cwdi_rdir) {
 +		mp = p->p_cwdi->cwdi_rdir->v_mount;
 +		if (dostatvfs(mp, &sfs, curl, MNT_NOWAIT, 1) == 0)
 +			procfs_format_mount(mp, &sfs, bf, &mtab, &mtabsz);
 +	}
 +
  	free(bf, M_TEMP);

  	if (mtabsz > 0) {

From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/45700 CVS commit: src/sys/miscfs/procfs
Date: Thu, 15 Dec 2011 15:55:03 -0500

 Module Name:	src
 Committed By:	christos
 Date:		Thu Dec 15 20:55:02 UTC 2011

 Modified Files:
 	src/sys/miscfs/procfs: procfs_linux.c

 Log Message:
 PR/45700: use dostatvfs instead of grabbing the latest cached copy of
 struct statvfs from the mount point, so that chroot is handled properly.


 To generate a diff of this commit:
 cvs rdiff -u -r1.61 -r1.62 src/sys/miscfs/procfs/procfs_linux.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->feedback
State-Changed-By: wiz@NetBSD.org
State-Changed-When: Thu, 15 Dec 2011 21:17:33 +0000
State-Changed-Why:
Is christos commit sufficient to fix this?


From: Taylor R Campbell <campbell+netbsd@mumble.net>
To: wiz@NetBSD.org
Cc: kern-bug-people@netbsd.org, netbsd-bugs@netbsd.org,
	gnats-admin@netbsd.org, gnats-bugs@NetBSD.org
Subject: Re: kern/45700 (/chroot/proc/mounts exposes out-of-chroot pathnames)
Date: Fri, 16 Dec 2011 04:57:20 +0000

    Date: Thu, 15 Dec 2011 21:17:34 +0000 (UTC)
    From: wiz@NetBSD.org

    Is christos commit sufficient to fix this?

 It would seem appropriate to me for /proc/mounts to match the
 behaviour of getvfsstat, which christos's change doesn't quite
 implement -- getvfsstat doesn't show the full path of any mount point
 even to the superuser, and it fakes an entry for a root file system if
 the chroot is not itself the mount point of any real file system.

 That's what I tried to do in the patch I suggested, although I botched
 something about the root file system fabrication.

State-Changed-From-To: feedback->analyzed
State-Changed-By: wiz@NetBSD.org
State-Changed-When: Fri, 16 Dec 2011 09:28:38 +0000
State-Changed-Why:
Feedback supplied, perhaps more to do.


State-Changed-From-To: analyzed->closed
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Sun, 18 Apr 2021 20:22:31 +0000
State-Changed-Why:
fixed in procfs_linux.c 1.63


>Unformatted:

Home	PR Database Search