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

[Ashwini Sharma]

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.


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

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.


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


>  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


Ashwini
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20131017/a765b0c8/attachment-0005.htm>