Date: Tue, 04 Nov 2003 21:40:27 +0000 From: Peter Edwards <peter.edwards@openet-telecom.com> To: current@freebsd.org Subject: Bug: nmount(2) lacks parameter checking. Message-ID: <3FA81CCB.8010803@openet-telecom.com>
next in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format.
--------------050304030407030706040004
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Transfer-Encoding: 7bit
Hi,
Looking over the code for nmount(), I think I noticed a few bugs.
(tried send-pr, but the lack of a web-front-end at freebsd.org,
and a decent mail system locally means that's not a runner)
nmount() calls vfs_nmount() pretty much directly after copying in
the io vector from userland.
vfs_nmount() then calls vfs_buildopts() as the first thing it does.
There's a couple of problems here. Firstly, 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 the option's "name" is a null
terminated string, but, it later calls vfs_sanitizeopts(), which
assumes this. By passing in strings just at and just over the
pagesize, a non-root user can cause a crash in vfs_buildopts
reasonably reliably when strcmp to hit an unmappped page. (Program
available on request)
vfs_buildopts also leaks memory if it jumps to "bad": anything in
the current option is lost to the woods.
There's also 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 be up to 64K in size. That's
64M of memory, plus some overhead for option structures, which would
be a lot to start chewing up in the kernel.
The source I based these observations on is from today, while my
kernel is a few weeks old, and I no longer have source for it.
Given the traffic on the list recently, I figure now is Not A Good
Time to install a fresh kernel, so the patch attached is tested to
the point that it compiles, but I think something like it is
required.
--------------050304030407030706040004
Content-Type: text/plain;
name="nmount.txt"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="nmount.txt"
Index: kern/vfs_mount.c
===================================================================
RCS file: /pub/FreeBSD/development/FreeBSD-CVS/src/sys/kern/vfs_mount.c,v
retrieving revision 1.111
diff -u -r1.111 vfs_mount.c
--- kern/vfs_mount.c 26 Sep 2003 09:07:27 -0000 1.111
+++ kern/vfs_mount.c 4 Nov 2003 21:46:44 -0000
@@ -246,6 +246,8 @@
struct vfsopt *opt;
unsigned int i, iovcnt;
int error, namelen, optlen;
+ size_t memTotal = 0;
+ static const size_t maxMemTotal = 1024 * 64;
iovcnt = auio->uio_iovcnt;
opts = malloc(sizeof(struct vfsoptlist), M_MOUNT, M_WAITOK);
@@ -256,6 +258,26 @@
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 > maxMemTotal ||
+ optlen > maxMemTotal ||
+ namelen > maxMemTotal) {
+ error = EINVAL;
+ goto bad;
+ }
+
if (auio->uio_segflg == UIO_SYSSPACE) {
bcopy(auio->uio_iov[i].iov_base, opt->name, namelen);
} else {
@@ -264,7 +286,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) {
@@ -277,7 +303,6 @@
goto bad;
}
}
- TAILQ_INSERT_TAIL(opts, opt, link);
}
vfs_sanitizeopts(opts);
*options = opts;
--------------050304030407030706040004--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?3FA81CCB.8010803>