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

[Rob Landley]

On 10/16/2013 11:39:35 PM, Ashwini Sharma wrote:
> On Thu, Oct 17, 2013 at 6:46 AM, Rob Landley <rob at landley.net> wrote:
> 
> > 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...)
> >
> 
> 20 is broken into 16 bytes for salt, 3 for $#$, 1 for _nul_  
> termination of
> string.

Indeed. It's functionally correct but the macro name is misleading.

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

Yeah, my thoughts exactly.

> > 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...
> >
> 
> Algorithm to be used can be given at command line,  which is verified  
> in
> get_salt(). That's why an error return is there at end.

I'd prefer parsing that into the $#$ number to be in a different  
function, so this part just had to care about the number.

> > 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...
> >
> >  +  // Grab 6 bit chunks and convert to characters in ./0-9a-zA-Z  
> +  for (i=0; i<len; i++) { +    int bitpos = i*6, bits = bitpos/8; +  
> +    bits = ((buf[i]+(buf[i+1]<<8)) >> (bitpos&7)) & 0x3f; +    bits  
> += 46; +    if (bits > 57) bits += 8; +    if (bits > 90) bits += 7;  
> + +    salt[i] = bits; +  }
> 
> In this snippet, bits is added with 8 or 7 when it is >57 or >90,
> doing this makes it jump to 66 or 98 respectively.
> 
> which leaves _A_ or _a_, instead this can be
> 
> +    if (bits > 57) bits += 7;  To jump to _A_+    if (bits > 90) bits
> += 6;  To jump to _a_

Oops. Corrected.

Thanks,

Rob