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 [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message
