Date: Mon, 3 Feb 1997 04:47:00 +1100 From: Bruce Evans <bde@zeta.org.au> To: davidn@unique.usn.blaze.net.au, freebsd-bugs@FreeBSD.ORG, freebsd-current@FreeBSD.ORG Subject: Re: sys/param.h: MAXLOGNAME Message-ID: <199702021747.EAA27813@godzilla.zeta.org.au>
next in thread | raw e-mail | index | archive | help
>After some testing with a username with exactly 16 characters
>in length, I hit a problem:
>
>Feb 3 01:54:27 labs login: setlogin 'abcdef0123456789': Invalid argument
>Feb 3 01:54:27 labs login: setusercontext() failed - exiting
There are many more off by 1 and worse bugs near here. setlogin() claims
to fail to for names of length 15, but it actually succeeds. For names of
length >= 16, it claims to fail, but it actually writes a truncated name.
>The error is generated by this code in sys/kern/kern_prot.c:
>
>int
>setlogin(p, uap, retval)
> struct proc *p;
> struct setlogin_args *uap;
> int *retval;
>{
> int error;
>
> if ((error = suser(p->p_ucred, &p->p_acflag)))
> return (error);
> error = copyinstr((caddr_t) uap->namebuf,
> (caddr_t) p->p_pgrp->pg_session->s_login,
> sizeof (p->p_pgrp->pg_session->s_login) - 1, (u_int *)0);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This has an off by 1 bug and corrupts the old name when it fails.
The `len' arg to copyinstr() includes the nul terminator, so 1 should
not be subtracted from the size here. The new name should be copied in
to a temporary buffer in case the copy fails. I think nul termination
is guaranteed although copyinstr() only does it if it returns 0 -
nothing here touches the last byte in s_login, and this byte is
statically initialized to 0 in session0 and cloned by bcopying the
session in enterpgrp(). If `len' were correct here, then copyinstr()
would corrupt the nul terminator, so these bugs must be fixed together.
The man page is unclear. It says that the `len' arg to copystr() and
copyinstr() gives the maximum length of the string. I think it should
say that the length includes the nul terminator. The POSIX.1-1990 spec
is similarly unclear about PATH_MAX. This is reported to be fixed in
P1003.1a. PATH_MAX includes the terminator.
The man page also says `NULL' when it means ASCII `nul'.
In BSD4.4, it is clear from the use of MAXPATHLEN in calls to copyinstr()
for vfs that `len' includes the terminator.
`len' is also wrong in:
lkmioctl(): (MAXLKMNAME-1 and MAXLKMNAME - 2)
ext2_mountroot(): (MNAMELEN - 1, no error check)
ext2_mount(): (sizeof(...) - 1, no error check)
ext2_mount(): (MNAMELEN - 1, no error check)
cd9660, portal, fdesc, kernfs, nullfs, umapfs, union, devfs, msdosfs, nfs,
ffs, lfs, mfs: similar to ext2fs :-(
vfs_mountroot(): (MNAMELEN - 1, no error check)
>and inspecting src/sys/sys/proc.h shows:
>
>struct session {
>...
> char s_login[MAXLOGNAME]; /* Setlogin() name. */
> ^^^^^^^^^^
>};
>
>.. which means that in reality, MAXLOGNAME-1 is the maximum
>number of characters in a username, which is currently 15,
>not 16 as everything else assumes.
>The fix for -current is obvious, although it seems that
>MAXLOGNAME should be at least as large as a valid user
>name. MAXLOGNAME in 2.2 was 12, so it won't be affected.
Its not so obvious. I think MAXLOGNAME includes the nul
terminator.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?199702021747.EAA27813>