enforce maxlength in usernames

Matthias Andree mandree at FreeBSD.org
Thu Jul 28 20:29:01 UTC 2011


Am 27.07.2011 23:12, schrieb Tito:
> On Wednesday 27 July 2011 22:33:09 Matthias Andree wrote:
>> Am 27.07.2011 21:56, schrieb Tito:
>>
>>> Saying that it does not belong there is not enough, please tell me also
>>> where it should be. Looked like a good place to me. In the same 
>>> function we check for illegal chars in usernames. You should also take
>>> into account that busybox does not support conf files for the adduser
>>> applet. Eventually the value could be made a config option (so that it could be
>>> changed) but it looks like bloat to me. Another way could be to add a define
>>> to libbb.h
>>>
>>> #define MAX_USERNAME_LENGTH 32
>>
>> Alright, IEEE Std. 1003.1-2008 aka Single UNIX™ Specification v4 aka The
>> Open Group Base Specifications Issue 7, already has corresponding
>> definitions.
>>
>> It's available for online reading free of charge after registration at
>> http://pubs.opengroup.org/onlinepubs/9699919799/
>>
>> Basically this standard has headers define LOGIN_NAME_MAX and
>> _POSIX_LOGIN_NAME_MAX, in <limits.h> and <unistd.h>, respectively.
>> These could be used, instead of inventing [y]our own.  Be sure to read
>> up on getlogin(), unistd.h, limits.h, sysconf thereabouts in the
>> standards before implementing; the latter _POSIX_ variant is the minimum
>> acceptable length for LOGIN_NAME, including the \0 byte, and currently 9.
>>
>> Inconsistencies will cause arbitrary malfunction, non-portability,
>> maintenance headaches and possibly even in-system incompatibilities.
>> Non-NUL terminated C strings are the least of your worries in that case.
> 
> Hi,
> Could  this be more acceptable. Could be improved by removing
> the double strlen also the error message could be better.
> Just to see if I overlooked something obvious.

No. The standard demands that, statically, the system defines
LOGIN_NAME_MAX to be >= sysconf(_SC_LOGIN_NAME_MAX).  You don't validate
that in applications though.  Check the getlogin() documentation in
SUSv4 for details on getlogin_r() use if it matters to use.

The validation of proper conformance, however, belongs in a separate
POSIX conformance test suite---but not into applications.

Also note that any half-witted global optimizer will detect and optimize
the common subexpression (strlen(name) + 1) and eliminate the 2nd
expression.

> void FAST_FUNC die_if_bad_username(const char *name)
> {
> 	/* Enforce length limits on usernames. 
> 	 * LOGIN_NAME_MAX: Maximum length of a login name,
> 	 * including the terminating null byte.
> 	 * Must not be less than _POSIX_LOGIN_NAME_MAX (9).
> 	 */
> 	if (!name 
> 	 || strlen(name) + 1 > sysconf(_SC_LOGIN_NAME_MAX)
> 	 || strlen(name) + 1 < _POSIX_LOGIN_NAME_MAX
> 	)

This is bogus. You want to allow shorter names (2nd strlen) as pointed
out by Lauri, and you don't want to validate a minimum of the maximum.
The standards conformance test for the OS doesn't belong into run-time
data validation checks.  I thing you want instead:

(a) a separate check against NULL with separate error message (providing
that die_if_bad_username() can get called with NULL argument at all)

(b) if (strlen(name) + 1 > LOGIN_NAME_MAX) { ... }

(c) omit the sysconf call and thereabouts. Note that sysconf() can
legally return "-1" for a "no limit" semantic, as written previously.

(d) if you meant to test for "empty string" you need !*name or !name[0]
or name[0] == '\0' instead of !name. The compiler should emit the same
code for everything. Be very careful whether you test the pointer or the
content it's pointing at.

> 		bb_error_msg_and_die("illegal name length");
> 	/* 1st char being dash or dot isn't valid: */
> 	goto skip;

Not pretty.  goto is not designed to jump *INTO* loop constructs.

Also note that the character set validation belongs into a function of
its own.

> 	/* For example, name like ".." can make adduser
> 	 * chown "/home/.." recursively - NOT GOOD
> 	 */
> 
> 	do {
> 		if (*name == '-' || *name == '.')
> 			continue;
>  skip:
> 		if (isalnum(*name)

This is bogus and can lead to segfaults through out-of-bounds array
subscripts on systems with signed chars.  This needs to be
isalnum((unsigned char)*name).  This is true for all toupper/tolower and
is*() functions from <ctype.h> where the argument is as wide as char.

Guys, please read the specs and docs!


More information about the busybox mailing list