On 07/05/2012 03:04 AM, Kyungwan Han wrote: > Dear Rob. > > > How are you? :-) > > I send source codes for passwd feature and I ask you to review our codes. > > I'd like to give code to you more frequently, but I spent a more time to > review this, so I cannot do it.
For me the login/passwd stuff has been blocked on more thoroughly investigating android password handling design and deployment options. Specifically, does bionic have getpwent() and friends? (Last I checked, it just had stubs). If it _does_ grow support, would it store password information in /etc or in its sad clone of the Windows registry? (In which case renaming /etc/shadow wouldn't do much.) If this code wouldn't work linked against bionic, would it be better for us to have built-in support for the file format (it's just colon-separated data, easy enough to parse on our own), or should we look at a C library like musl as our recommended android deployment option? > Although I did review for our code, I think it might have something to > improve in the code. It looks reasonably sane at first glance, I just need to study the design issues more closely. (It was fairly far down down my todo list until now.) There are a few cleanups like "asprintf() is stubbornly non-portable but we have xsmprintf() in the library, which is", but I can do those easily enough. One design issue: strength_check() is policy. I'm a little reluctant to hardwire that in. A _proper_ strength check would involve dictionaries and such. Maybe the ability to shell out to something that does an arbitrary strength check could be added later. But if a user sets their password to their username, is it really likely they didn't _know_ that's a bad idea before we tell them? If we're not checking them setting it to "connect", "monkey" or "123456" I'm not sure this check is doing any good at all. (P.S. Because those were #12, 14, and 15 on the list of passwords leaked from linkedin.com. All the others in the top 15 list were less than 6 digits, which means you can brute force them on a smart phone.) http://redtape.msnbc.msn.com/_news/2012/06/07/12107855-a-linkedin-leak-lesson-top-30-dumb-passwords-people-still-use?lite Also, I recently added an "Error messages and internationalization" section to the end of http://landley.net/toybox/design.html which might be relevant here. > If you find it after reviewing, please let me know. > > > Two files are added to toybox: > - toys/passwd.c - passwd_main() > - lib/passwdutils.c - helper library for passwd. > > Two files are modified : > - lib/lib.h - adding declaration for passwd helper functions in line > 170~172. > - scripts/make.sh - adding librt for generating random number seed in > line 117. Um, line 117 of what? lib/passwdutils.c that's a blank line, toys/passwd.c it's "msg = "too short"; Are we not using /dev/urandom? The OS has a better random number generator than we'll ever manage from userspace... > I think passwdutils.c can be reused in adduser, addgroup, etc, so I > extracted it from passwd.c. Agreed. > If you mind this structure, I merge passwd.c and passwdutil.c. > > Please let me know your opinion. I need to look at it more closely. I'll try to allocate some time this evening. Thanks, Rob -- GNU/Linux isn't: Linux=GPLv2, GNU=GPLv3+, they can't share code. Either it's "mere aggregation", or a license violation. Pick one. _______________________________________________ Toybox mailing list [email protected] http://lists.landley.net/listinfo.cgi/toybox-landley.net
