[Toybox] [New Toy] pwgen

[Rob Landley]

On 12/5/20 10:23 AM, Moritz Röhrich wrote:
> Dear Mr. Landley,
> 
> I have implemented `pwgen`, a small password generation utility for toybox.
> This utility is useful, not only for passwords but also for generating
> random strings in general.
> 
> Hopefully the code meets quality expectations, but I'd be more than happy to
> take criticism.
> 
> Best regards, Moritz

Hmmm, hadn't heard of this before but:

  https://linux.die.net/man/1/pwgen

Can't argue. Your implementation is small and looks sane. Ok, applied to
pending. As for cleanup, toys.optflags & FLAG(A) is unnecessary: the FLAG(A)
macro expands to (toys.optflags & FLAG_A).

I inlined get_rand_chr() into the one call site so the two calls just become
"while (strchr(illegal, c=blah));" and then I can inline THAT at its one call
site, ala:

  for (i = 0; i<l; i++) while (strchr(illegal, pw[i] = 33+(rand()%93)));

And then inline THAT at its one and only call site...

The rand() function does not spark joy, I have xgetrandom() for a reason.

Why does the optstr have ? to pass through unknown arguments? What does that
accomplish? (It lets you pass negative values in for the two numeric arguments,
neither of which makes sense as a negative number...)

get_illegal_chars() is only called in one place but then the resulting variable
is only USED in one place, so the tests might as well just be in that place?
(This nicely glosses over the way it's allocating 94 chars and then strcat() a
user-supplied string to that buffer with no bounds checking: let's not do that.)

You generate an array of passwords and then output them with two for() loops,
but each is only used exactly once. Why not just generate one at a time in the
for loop and reuse the same buffer? In fact I have a 4k toybuf and if I just cap
the supported password size at 4k I don't have to allocate or free anything.

I installed the other pwgen to see what it does, and yes when run with no
arguments it outputs a zillion passwords. And then when I run that output
through "wc" to see how many columns and lines of output it's generating, it
produces ONE password. (Because why NOT have different behavior depending on
whether or not the output is to a tty? Ah, the man page says it's an anti
shoulder surfing measure. Makes sense. Also, "as secure as completely completely
random" is a duplicated word in the man page. Anyway, I should update our help
text to explain WHY it's doing that...)

It's hardwiring columns to 80 but we have detection plumbing. (Hmmm, the
previous one wasn't going right up to the right edge and the one I just wrote
is. Aesthetically tweaking it to produce one less password per line match the
other one...)

The FLAG(y) string is possibly easier to understand as:

  if ((c>'9' && c<'A') || (c>'Z' && c<'a') || c>'z')

(Although the order the string is stated in doesn't match up with the ascii
table and I had to hunt and peck to prove it.)

Hmmm, this is producing a LOT more capital letters than the other version, which
also falls under "human readable affordance". let's see... Top bit of entropy
per byte isn't really used, so I'll squelch capitals when it's set. (That should
make 1/4 of letters capital.)

The other one doesn't do a page full for "pwgen -1", fixed... Hmmm,

$ toybox pwgen -y
p:Q1$h=C h6W`ieZ< Q`o!b|+) 1apBp}nT er at 7mKgi waAqC[7i v<y\:jzt [#o=Nw7w
tx1^1Uo[ o`B]y84{ wjdsl>%n R=<h[*0" #m*+(z!( qbZf,3h) fs&oc1C0 `?#-sstC
r`mR{ht{ i%g'FA$> ofy=#t}7 rCRWEmlq 7A;/`|}= rvqv|swe wT\z-(sw ,Cr*y6c.
$ pwgen -y
Eegae:B9 pee3Boh{ Hie~j3Lu aew)a3Jo zae'Cho5 quah!Ph5 EJa(X5Ee zui7Aez)
Too2Ed)o kap.ae4L ahj$i8Se Aile-ch4 nah+w3Ea wa"Zo9ea Shu4dae+ tuNg]u7e
giY!oc9o duG5eiz- sahc7eS* ooPi at z0e eX7nei_d iV/ae1se eiQu4om^ Ni>pig1o

That's still a very different character distribution. He's squelching more
capitals than I am, and at least half the punctuation... ah, that's not what
he's doing. When you request punctuation you get ONE instance of punctuation,
and one, or less often two, or almost never three capital letters... this is a
curve isn't it?

$ pwgen 8 1000 | sed 's/[^A-Z]//g' | while read i; do echo $i | wc -c; done |
sort | uniq -c
    754 2
    217 3
     25 4
      4 5

Yes, that is DEFINITELY intentional. Right. And what I've got so far is just doing:

$ ./pwgen 8 1000 | sed 's/[^A-Z]//g' | while read i; do echo $i | wc -c; done |
sort | uniq -c
    188 1
    376 2
    293 3
    107 4
     31 5
      5 6

Which is... eh? Close enough? The other one is guaranteeing that each password
has at least one number, and with -y adds ONE instance of punctuation to each
password. That means every generated passwords can comply with a site policies,
which is not how this is designed but I'm not redesigning it just now...

Rob