[PATCH] passwd size reduction take 6

Tito farmatito at tiscali.it
Mon Jan 30 13:19:40 UTC 2006


On Saturday 28 January 2006 23:01, Rob Landley wrote:
> On Wednesday 25 January 2006 14:08, Tito wrote:
> > Hi,
> > This is take 6 of passwd size reduction and this time
> > I think I've got it right..
> >
> > The program now acts more like the real passwd on my linux box:
> > 1) allow 3 retries as long as a good (not easy to guess)  new password is
> > typed (for normal user)
> 
> You need to type a good password in order to be allowed to retry?
> 
> I'm confused: passwd changes the password, right?  So if they type a good, not 
> easy to guess password, why is that not simply accepted?

Sorry, just bad English. 
1) allow 3 retries until a not easy to guess new password is typed.

> > 2) override obscure password checking for root 
> > 3) don't ask for old password again if obscure test fails.
> > 4) exits on password retype mismatch.
> > 5) use bb_do_delay in all errors related to asking for the old password in
> > new_password() .
> 
> Ok.
> 
> > 6) rip out -a switch (non standard: on my system it is -a 
> > -all), hope nobody complains.....
> 
> Hmmm, glance at 1.1.0 tarball...  It selects algorithm (des or md5).  Yeah, 
> allowing a user to specify that on an individual password makes very little 
> sense.
> 
> How is this specified now?  If CONFIG_FEATURE_SHA1_PASSWORDS is configured in, 
> shouldn't it _always_ generate SHA1 passwords?  (in which case the salt[] bit 
> in new_password, and hence the whole bb_read_random64_string() stuff, should 
> #ifdef out?  Hmmm...)

Yes i was wondering about this as  CONFIG_FEATURE_SHA1_PASSWORDS 
is not in Config.in,  .config or bb_config.h, so there is no way to select this 
option (maybe only manually!?) and probably It could just be removed.

It is only in:
busybox.13608/include/libbb.h
busybox.13608/include/usage.h
busybox.13608/libbb/pw_encrypt.c

> Looking closer, I'm not quite sure what libbb/pw_encrypt.c is for.  It has 
> three users: httpd.c, sulogin.c, and passwd.c.  We know what passwd.c is 
> doing, so let's look at the other two:

snip

> 
> > 7) default to use salted passwords (more secure?!).
> > 8) generate random salt if /dev/random could be opened
> 
> Coolness, that closes a bug in the bug generator.
> 
> > 9) use /etc/shadow when CONFIG_FEATURE_SHADOWPASSWDS is enabled
> >      only if the /etc/shadow exists else default to /etc/passwd.
> 
> Falling back to /etc/passwd probably isn't a good idea.  If you don't want to 
> create /etc/shadow automatically, I'd prefer to fail noisily if it doesn't 
> exist.

This is what real passwd does, i've tested it.
There are 3 cases:

1) user is not in /etc/passwd or /etc/passwd is missing: exit with unknown user
2) user is in /etc/passwd but not in /etc/shadow: exit with unknown user
3) user is in /etc/passwd and  /etc/shadow is missing: password is changed in /etc/passwd.

Feel free to change it, size will be reduced a lot.

> > 10) fix the license as suggested on the list.
> > 11) code cleanup
> >
> > Size reduction is:
> >
> >    text    data     bss     dec     hex filename
> >    3166       0     160    3326     cfe passwd.o.orig
> >    2009  136     140    2285     8ed passwd.o
> 
> Nice.
> 

snip

> 
> I twiddled copy_passwd_file() a bit.  It would leak out_fp if the chmod() or 
> chown() failed, 

I couldn't imagine why they should fail on a path we was able to open 
(maybe only if path is deleted?)

> it was recalculating strlen() repeatedly, etc. 

I've tried to use a variable to store this value but size increased.

> Using copy_passwd_file() to create a backup copy of the password file is a bit 
> silly.  You already have the old file, and you really shouldn't be modifying 
> its contents (that leads to all sorts of fun race conditions with other 
> processes trying to read /etc/passwd while you diddle it), so why not just 
> hardlink it to the backup using link(), make the new file off to one side, 
> and then do an atomic rename() to put it in the right place (which deletes 
> the file being squashed for you, atomically?)  That way you not only have 
> better multitasking behavior but you get the backup copy for free without 
> having to diddle with stat() and utime() to manually propogate the timestamp 
> to the backup copy...
> 
> I can sort of see doing the file locking stuff so that if two passwd() 
> processes try to update at the same time they don't stomp each other, but 
> wouldn't it be better to use F_SETLKW to wait, rather than failing 
> immediately?  (Is there any way to specify a timeout on this?  Or do we have 
> to use alarm()?  In which case this should _definitely_ be wrapped in libbb 
> somewhere.)

This was the behaviour of the old passwd applet.

> Sorry, old OS/2 habits kicking in when file locking comes up...
> 
> read_random64_string() is discarding entropy from /dev/random.  This generally 
> isn't something you want to do on embedded devices that have limited 
> quantities of real entropy available and will potentially block indefinitely 
> if they run out.
> 
> And reading in a whole file into a char * given a path to the file is 
> something mdev does, and probably other things.  That seems like a candidate 
> for libbb.
> 
> Also, there's already base64 handling code in networking/wget.c function 
> base64enc(), coreutils/uudecode.c function read_base64(), 
> coreutils/uuencode.c table tbl_base64[], networking/httpd.c function 
> decodeBase64(), and probably elsewhere.  That _really_ needs to be in libbb, 
> but I'll leave it alone for now.  (TODO item added.)

Yes, I've seen them also and thought about unifying them, but 
don't understand well this stuff......

> 
> In new_password, why are ret and retry static?

Ops....

> >   /* Do not use bb_error_msg() as the output should not
> >   contain the program name for the sake of scripting utilties.*/
> 
> What scripting utilities?  (I tried to check the relevant standard, but this 
> is yet another utility that the Open Group Base Standards version 6 doesn't 
> even _mention_, which kinda sucks.  Is there a larger standard that 
> incorporates this thing somewhere?  LSB, perhaps?)

Vodz dixit:

The Passwd utility is unlike nonterminal. For hack this many people
using "expect". bb_error_msg("eror_msg") produce "utility_name: error_msg"
and unexpect for "expect"

> 
> Question; this fprintf + expect stuff applies only to this two messages?

Yes.

> I've modified the passwd.c file you sent me, and if I can find another 2 hours 
> to work on it today I'll check it in.
> 
> Rob

OK, thanks and Ciao,
Tito.

BTW: I'll be offline for the next two weeks for vacation, 
so to avoid that you run out of patches to audit  ;-)
I've fixed some minor issues and modified a little the previous
obscure stuff patch and added a config option:

config CONFIG_RELAXED_PASSWD_CHECK
       bool "  Support for relaxed password checking"
       default n
       depends on CONFIG_PASSWD
       help
        Permit passwords that not contain a mix of four different
        types of characters (upper case letters, lower case letters,
        numbers and special characters) but request an increase in
        length for each missing type.

With this option enabled the behaviour is as before 
else non mixed passwords are considered weak.

I also moved the check for root to obscure.c so it now warns about 
weak root passwords but doesn't return an error.

If this patch is checked in, this line in passwd.c new_password()

if (obscure(bb_common_bufsiz1, cp, pw) && getuid()) {

should be changed to; 

if (obscure(bb_common_bufsiz1, cp, pw)) {

Bye.

:-P
-------------- next part --------------
A non-text attachment was scrubbed...
Name: obscure02.patch
Type: text/x-diff
Size: 11900 bytes
Desc: not available
Url : http://lists.busybox.net/pipermail/busybox/attachments/20060130/7096f499/attachment.bin 


More information about the busybox mailing list