[Toybox] Sending passwd source code for contribution

[Rob Landley]

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.

 1341496655.0