From owner-freebsd-current Thu Feb 15 21:29:21 2001 Delivered-To: freebsd-current@freebsd.org Received: from mailman.zeta.org.au (mailman.zeta.org.au [203.26.10.16]) by hub.freebsd.org (Postfix) with ESMTP id 67D0637B4EC; Thu, 15 Feb 2001 21:29:17 -0800 (PST) Received: from bde.zeta.org.au (bde.zeta.org.au [203.2.228.102]) by mailman.zeta.org.au (8.9.3/8.8.7) with ESMTP id QAA20071; Fri, 16 Feb 2001 16:29:12 +1100 Date: Fri, 16 Feb 2001 16:28:44 +1100 (EST) From: Bruce Evans X-Sender: bde@besplex.bde.org To: Robert Watson Cc: Martin Blapp , adrian@FreeBSD.ORG, current@FreeBSD.ORG Subject: Re: Fix for mountpath lenght In-Reply-To: Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: owner-freebsd-current@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.ORG On Thu, 15 Feb 2001, Robert Watson wrote: > As has been pointed out, this is simply incorrect due to an attempt to use > kernel string operators on a string in the kernel address space. Generally > speaking, it's a bad idea to explicitly perform string activities on > userland strings, instead, to rely on the bounds checking in copyinstr() > and related calls. The patch also seems to have a fatal off-by-2 error. It would only have a fatal off-by-1 error, but most filesystems seem to have a benign off-by-1 error. E.g., ffs uses copyinstr() but defeats copyinstr()'s "right" handling of the terminating NUL by subtracting one from the array size. copyinstr(9) has the usual unclearness about NUL terminators for this. The NUL terminator is included in the strings "long"ness for both the input and the output args. This is only documented explicitly for the output arg. > The namei has all appropriate bounds checks it needs > for normal nul-termianted string reading from userland. If you need to > place an artificially low bound on the string length for a path name that > is to be read in, and reject based on the pathname length, then namei() is > probably not the right call to pull in the string in the first place. For > many reasons, including that if shared memory is in use, then there's a > race condition under SMP that can let malicious processes update the path > between the two checks as they are non-atomic. The individual file systems can eaily do this check when the copy in the string. All they have to do is actually check for copyinstr() returning an error, and clean up a bit when it returns an error. Not checking is a bug even if ENAMETOOLONG is treated as a non-error. EFAULT should be treated as an error (but perhaps namei()'s success means that this error can't happen). If there is a race, then it is old and not restricted to SMP (just larger for SMP) -- copyinstr() may block. This shouldn't be a problem for mount(), since mount() requires privilege. > What is it you're trying to accomplish here, exactly? Is it prevent paths > >MNAMELEN to be used as targets of mounting operations? Or is it to > truncate strings reported via statfs to some arbitrary bound? If it's the It is to not permit mount() operations whose mount point can't be recorded in the kernel because its name is too long. There is a similar problem for the "from" name. At least the following non-interactive operations now depend on the names being recorded properly: fsck (for hotroot stuff) and `umount -A'. > > --- vfs_syscalls.c Sun Nov 26 03:30:05 2000 > > +++ vfs_syscalls.c.new Thu Feb 15 18:22:13 2001 > > @@ -140,6 +140,8 @@ > > /* > > * Get vnode to be covered > > */ > > + if (strlen(SCARG(uap, path)) > MNAMELEN) ^ should be >= for no off-by-1 error > > + return (ENAMETOOLONG); > > NDINIT(&nd, LOOKUP, FOLLOW | LOCKLEAF, UIO_USERSPACE, > > SCARG(uap, path), p); > > if ((error = namei(&nd)) != 0) + From: + * $FreeBSD: src/sys/ufs/ffs/ffs_vfsops.c,v 1.138 2001/02/09 06:11:33 bmilekic Exp $ +... + copyinstr(path, mp->mnt_stat.f_mntonname, MNAMELEN - 1, &size); ^^^^ off-by-1 + bzero( mp->mnt_stat.f_mntonname + size, MNAMELEN - size); The bzero() gives a second NUL terminator, but this doesn't require extra code because the rest of the array must be zeroed. Bruce To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-current" in the body of the message