On 10/16/2013 11:39:35 PM, Ashwini Sharma wrote:
On Thu, Oct 17, 2013 at 6:46 AM, Rob Landley <r...@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
_______________________________________________
Toybox mailing list
Toybox@lists.landley.net
http://lists.landley.net/listinfo.cgi/toybox-landley.net
