[Toybox] [New Toys] - groupadd, useradd, mkpasswd and modified passwd, lib/password

[Rob Landley]

On 10/02/2013 03:49:21 AM, Ashwini Sharma wrote:
> Hi list,
> 
> Attached are the implementations for groupadd, useradd and mkpasswd
> commands.

Ok, I've caught up to this...

> Patches are as follows
> 
> 1. __lib.patch__  : this includes changes made to lib/lib.h and
> __lib/password.c__.
> lib/passowrd.c is modified to share the function __update_password()__
> among groupadd, useradd commands.
> This also has the factored out code, common to both, for __passwd__  
> and
> __mkpasswd__.
> Also has the cosmetic cleanup changes.

Alas, lib/password.c is one of the pieces that would be in pending if  
login.c didn't predate it. I never got this properly cleaned up. Still,  
adding this doesn't change that, so... first patch applied, and lemme  
at least look at the new bits.

We add three #defines to the header, which are never used. They should  
be in the patch that actually uses them. (MAX_SALT_LEN of 20 is wrong,  
the largest actual salt is 16, that's the buffer size needed to  
process... something. Right...)

The first function in there, random_number_generator(), just reads an  
int from a filehandle. It's only ever called from one place, and that  
caller is what opens and closes /dev/urandom, so having the read in the  
same place doesn't add any new portability constraints. (I.E. let's  
just inline this function at its only callsite.)

The return value is fed into inttoc(), which is also the only caller of  
that function. inttoc() discards all but 10 bits of it, so we read 32  
bits of entropy, and discard 21 of those bits? Not idea on devices with  
limited entropy.

Um, hang on:

   i &= 0x3f; // masking for using 10 bits only

You can't use 10 bits out of an 8 bit byte. And 0x3f is 00111111 which  
is 6 bits. So the comment is both impossible and wrong.

Let's see, max salt is actually 16, times 6 is 96 bits which is an even  
12 bytes, so we can read in a buffer of that... this logic is largely  
the same as uuencode by the way.

All the users are in get_salt(), which is a strange function. Each  
algorithm has a unique digit indicated in the $#$ signature in the  
hash, but we don't use that digit to indicate the algorithm, instead we  
pass in a string which we check and then _set_ the digit. Why do we do  
that? If it was an int we could just use an array to set the salt  
length. Also, the function has an error return value for matching none  
of the strings, but didn't we pass _in_ that string? Not sure of the  
design goal here, who is currently calling this...

Um, there's a second copy in toys/lsb/passwd.c...? Ah, I see, the  
second patch in the series removes the function. so it's a move spread  
across two patches. Lemme redo that checkin to do both halves...

(Sigh. My bad. If I don't clean up code, people extend the  
non-cleaned-up code...)

I need to set up a test environment for this. Ok, I've inlined  
random_number_generator() and inttoc() and it compiles, but I can't  
test it. I'll check it in anyway and try to come up with a test  
chroot...

> 2. __passwd.c.patch__ : this file is modified due to the common code
> factoring out and cosmetic cleanup changes.

I checked in this and the previous patch together as one logical unit,  
because it's moving code from one file to another and the result  
doesn't make sense without both of them.

Rob
 1381972584.0