From owner-freebsd-current@FreeBSD.ORG Tue Nov 4 13:40:31 2003 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 2FE4B16A4CE for ; Tue, 4 Nov 2003 13:40:31 -0800 (PST) Received: from sweeper.openet-telecom.com (mail.openet-telecom.com [62.17.151.60]) by mx1.FreeBSD.org (Postfix) with ESMTP id C493B43FDF for ; Tue, 4 Nov 2003 13:40:28 -0800 (PST) (envelope-from peter.edwards@openet-telecom.com) Received: from mail.openet-telecom.com (unverified) by sweeper.openet-telecom.com for ; Tue, 4 Nov 2003 21:41:31 +0000 Received: from openet-telecom.com (10.0.0.40) by mail.openet-telecom.com (NPlex 6.5.027) (authenticated as peter.edwards@openet-telecom.com) id 3FA62F9800001FAC for current@freebsd.org; Tue, 4 Nov 2003 21:35:54 +0000 Message-ID: <3FA81CCB.8010803@openet-telecom.com> Date: Tue, 04 Nov 2003 21:40:27 +0000 From: Peter Edwards User-Agent: Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.5) Gecko/20031104 X-Accept-Language: en-us, en MIME-Version: 1.0 To: current@freebsd.org Content-Type: multipart/mixed; boundary="------------050304030407030706040004" Subject: Bug: nmount(2) lacks parameter checking. X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 04 Nov 2003 21:40:31 -0000 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--