[PATCH] passwd size reduction take 6

Rob Landley rob at landley.net
Sat Jan 28 22:01:14 UTC 2006


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?

> 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...)

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:

The httpd.c calls will always start with "$1$".  (One checks for that string 
before calling, the other supplies it as a constant.)  So that might as well 
be calling crypt() directly.

The sulogin.c use is passing in data from bb_askpass(), which is a read() from 
stdin.  The only way you get $1$ or $2$ there is if the user typed it, which 
seems unlikely...

Ah, I see, the salt field is grabbed from the existing password file, and is 
in fact the entire old (encrypted) password.  And _that_ starts with $1$ or 
$2$ to determine encryption type.  The salt field is what determines the 
encryption type.  Hmmm...

Ah, I see.  The resulting password field (for md5) is actually three fields:

$type$salt$encrypted_password

Each field starts with $ (or alternately you can veiw it as three $ separated 
fields with a leading $ indicating it's not an old DES style password, which 
is the fallback).

The "type" is 1 for MD5 and 2 for SHA1.  The SHA1 one doesn't seem to use 
salt, or at least our implementation doesn't.  I suppose we could append the 
salt to the password by hand, if we wanted to.  (Do sha1 passwords need 
salting?  What's compatible with existing practice?)

The salt is random bytes also used along with the cleartext password to 
produce the encrypted_password.  (You can xor, you can append, however you do 
it you perturb the cleartext password in a known way.  This way not every 
cleartext password produces the same encrypted password.  The salt is 
revealed to you so you can reproduce what was done to it, but you can't 
control what the salt is.  This is presumably so people can't pregenerate 
dictionaries of billions of pre-encrypted potential passwords to test against 
quickly.  You'd need a different dictionary for each possible salt value, so 
they have to do actually the math for each possible password they want to try 
at the time they try to break it unless they know the particular salt value 
in advance.  It also means that a hundred users who each have the password 
"password" would all have different encrypted values, so you can't even 
recognize common ones without doing the cryptographic math.)

This _really_ needs to be documented somewhere...  Right, I've updated 
programming.html to include this.

> 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.

> 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.

> Thanks in advance  to all for your help,
> As before patch and whole file for auditing are attached.
> Patch is against svn revision 13608.
>
> Please apply if you think it is ok.
>
> Ciao,
> Tito

Ok, it's time to sit down and review this sucker all the way through.  Find a 
coffee shop (still a bit of a challenge in Pittsburgh), grab a hot chocolate, 
find a nice quiet (but not too quiet) table, and open the code:

I twiddled copy_passwd_file() a bit.  It would leak out_fp if the chmod() or 
chown() failed, it was recalculating strlen() repeatedly, etc.

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.)

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.)

In new_password, why are ret and retry static?

>   /* 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?)

Is something explicitly checking for "Incorrect password." so that if we said 
"Wrong password" instead (a slightly shorter message, although not as short 
as "Nope") it would break?  This is aside from bb_error_msg_and_die() being 
smaller here: it sounds like brittle undocumented usage...

Darn it, I need to take a break.  It's 5 pm and the coffee shop is closing.  
(I've gotten about as much work done on busybox since moving to pittsburgh as 
I used to get done in a day or two in Austin.  I hope this is just fallout 
from moving...)

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
-- 
Steve Ballmer: Innovation!  Inigo Montoya: You keep using that word.
I do not think it means what you think it means.



More information about the busybox mailing list