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. > > 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
_______________________________________________ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net