From owner-freebsd-bugs  Sun Feb  2 09:50:52 1997
Return-Path: <owner-bugs>
Received: (from root@localhost)
          by freefall.freebsd.org (8.8.5/8.8.5) id JAA26968
          for bugs-outgoing; Sun, 2 Feb 1997 09:50:52 -0800 (PST)
Received: from godzilla.zeta.org.au (godzilla.zeta.org.au [203.2.228.19])
          by freefall.freebsd.org (8.8.5/8.8.5) with ESMTP id JAA26947;
          Sun, 2 Feb 1997 09:50:47 -0800 (PST)
Received: (from bde@localhost) by godzilla.zeta.org.au (8.8.3/8.6.9) id EAA27813; Mon, 3 Feb 1997 04:47:00 +1100
Date: Mon, 3 Feb 1997 04:47:00 +1100
From: Bruce Evans <bde@zeta.org.au>
Message-Id: <199702021747.EAA27813@godzilla.zeta.org.au>
To: davidn@unique.usn.blaze.net.au, freebsd-bugs@FreeBSD.ORG,
        freebsd-current@FreeBSD.ORG
Subject: Re: sys/param.h: MAXLOGNAME
Sender: owner-bugs@FreeBSD.ORG
X-Loop: FreeBSD.org
Precedence: bulk

>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