[PATCH] 2nd try: improve checks on usernames

walter harms wharms at bfs.de
Tue Aug 2 13:37:35 UTC 2011



Am 02.08.2011 15:10, schrieb Tito:
> On Tuesday 02 August 2011 10:39:22 walter harms wrote:
>>
>> Am 01.08.2011 22:01, schrieb Tito:
>>> Hi,
>>> this patch improves the checks performed on usernames 
>>> with the die_if_bad_username() function by adduser and addgroup.
>>> The changes are:
>>> 1) better comments;
>>> 2) use of the portable filename character set plus '@' and '$';
>>> 3) don't use isalnum as it  will allow non-ASCII letters in legacy 8-bit locales as pointed out by Rich Felker;
>>> 3) enforce minimum length of 1 char (or at least 2 chars if '$' is used as last char);
>>> 4) enforce maximum length of LOGIN_NAME_MAX (including null termination);
>>> 5) don't use goto to jump into the loop (requested by Matthias Andree);
>>> 6) don't allow '$', '.', '@' and '-' as first char;
>>> 7) allow '$' only as last char.
>>> 8) don't print the illegal char in error message as if it is a wide char it will be unreadable.
>>>
>>> On Debian the default is more conservative as defined by the regular expression /^[a-z][-a-z0-9]*$/
>>> we  use a more permissive /^[_A-Za-z0-9][-\@_.A-Za-z0-9]*\$?$/  instead.
>>> The function is tested with this test cases:
>>> 	"",  "$",  "a", "aa", "aA", "@a", "a@",  "a at a", ".a", "a.", "a.a", "-a",
>>> 	"a-",  "a$", "$a",  "a$a", "_a",  "a_", "a_a",  "a1",  "1a",  "a?",  "aè",
>>> 	"a€",  "aQzECE2-A_l at s$"
>>> and seems to behave well.
>>> I would like to point out that this check can be simply turned off
>>> by disabling   FEATURE_CHECK_NAMES in config
>>> (Enable sanity check on user/group names in adduser and addgroup)
>>> to avoid mixing of "mechanism (code) and policy" and also that
>>> it mimics what adduser/addgroup do on my debian box (with
>>> a more restrictive policy).
>>> Hints, critics and improvements are welcome.
>>> Special thanks go to Matthias Andree and Rich Felker for the patience
>>> thay had in explaining me my errors.
>>>
>>> Please apply if you like it.
>>>
>>> Ciao,
>>> Tito
>>>
>>>
>>> /* The username to "(...) be portable across systems conforming to IEEE Std
>>>  * 1003.1-2001, (...) is composed of characters from the portable
>>>  * filename character set. The hyphen should not be used as the first character
>>>  * of a portable username" (SUSv3). The Portable Filename Character Set is:
>>>  *  A B C D E F G H I J K L M N O P Q R S T U V W X Y Z
>>>  *  a b c d e f g h i j k l m n o p q r s t u v w x y z
>>>  *  0 1 2 3 4 5 6 7 8 9 . _ -
>>>  * We allow also the '@' and the '$' sign, but to avoid problems, '@','$' and '.'
>>>  * are not permitted at the beginning of the username (for example, a name
>>>  * like ".." can make adduser chown "/home/..).
>>>  * For compatibility with Samba machine accounts '$' is supported
>>>  * (only) at the end of the username. On Debian the default is more conservative as
>>>  * defined by the regular expression /^[a-z][-a-z0-9]*$/.   
>>>  */
>>>
>>> void FAST_FUNC die_if_bad_username(const char *name)
>>> {
>>> 	const char *s = name;
>>>
>>> 	assert(name != NULL);
>>> 	
>>> 	/* The minimum size of the login name is one char or two if
>>> 	 * last char is the '$', this exception is catched later 
>>> 	 * as the dollar sign could not be the first char.
>>> 	 * The maximum size of the login name is LOGIN_NAME_MAX 
>>> 	 * including the terminating null byte. 
>>> 	 */
>>> 	if (!*name || strlen(name) + 1 > LOGIN_NAME_MAX)
>>> 		bb_error_msg_and_die("illegal name length");
>>>
>>
>> 	when there is a "char buf[LOGIN_NAME_MAX]", maybe you need
>>          strlen(name) + 1 >= LOGIN_NAME_MAX
> 
> Don't think so LOGIN_NAME_MAX is the length of the string including
> the terminating null byte, strlen returns the length without the null byte
> so a strlen + 1 /the null byte*/ fits in LOGIN_NAME_MAX it is ok
> if it greater than LOGIN_NAME_MAX it is a error.
> 
> 		strlen(name) + 1 < LOGIN_NAME_MAX = ok
> 		strlen(name) + 1 = LOGIN_NAME_MAX = ok
> 		strlen(name) + 1 > LOGIN_NAME_MAX = error
>> 		
>>> 	do {
>>> 		/* We don't use isalnum  as it will allow locale-specific non-ASCII */
>>> 		/* letters in legacy 8-bit locales. */
>>> 		if (((*name == '-' || *name == '.' || *name == '@') && name != s) /* not as first char */
>>> 		 || (*name == '$' && (!name[1] && name != s)) /* not as first, only as last char */
>>> 		 || *name == '_'
>>> 		 || (*name >= 48 && *name <= 57)  /* 0-9 */
>>> 		 || (*name >= 65 && *name <= 90)  /* A-Z */
>>> 		 || (*name >= 97 && *name <= 122) /* a-z */
>>> 		) {
>>> 			continue;
>>> 		}
>>
>>
>> 	the user should habe a change to understand what he did wrong ...
>> 	perhaps you can do something like that:
>> 	bb_error_msg_and_die("illegal character >%c< in name",*name);
> 
> I tried this but in some cases it prints garbage to terminal e.g.:
> 
> 	printf("'%c'\n", 'é');
> 
> ./test 
> '�'
> 

granted, ntl it would be nice to have a better error that "it does not work".
Perhaps (s-name) = position ? "illegal character at position %d",(s-name) ?


> so i tought that it is not worth the hassle;
> 
>>> 		bb_error_msg_and_die("illegal character in name");
>>> 	} while (*++name);
>>> }
>>
>> How important do you cansider that $ can be "nowhere else that last place" thing ?
>> So far i understand that Samba can life with $, dropping support for that would simplify the thing.
> 
> I cannot say, I've extracted this info from an adduser error message:
> 
> "To avoid problems, the username should consist only of
> letters, digits, underscores, periods, at signs and dashes, and not start with
> a dash (as defined by IEEE Std 1003.1-2001). For compatibility with Samba
> machine accounts $ is also supported at the end of the username"
> 
> To be honest original adduser implements a first regex that honors
> more or less this error message: /^[_.A-Za-z0-9][-\@_.A-Za-z0-9]*\$?$/
> but is later overriden NAME_REGEX setting in /etc/addduser.conf 
> or hardcoded in /usr/share/perl5/Debian/AdduserCommon.pm
> which on my debian box is more restrictive:   /^[a-z][-a-z0-9]*$/ 
> and doesn't support '$' at all. So eventually it could be dropped.
> 

if this regex stuff is standard ... did you try it in place of the while-loop ?

re,
 wh


More information about the busybox mailing list