[Toybox] [PATCH] mktemp: yet more tests, and yet more fixes.

[enh]

On Thu, Dec 6, 2018 at 9:53 AM Rob Landley <rob at landley.net> wrote:
>
> On 12/5/18 7:34 PM, enh via Toybox wrote:
> > In particular this reuses the password.c code for random ASCII bytes.
> > ---
> >  lib/lib.h         |  1 +
> >  lib/password.c    | 35 +++++++++++++++++++++--------------
> >  tests/mktemp.test |  6 ++++++
> >  toys/lsb/mktemp.c | 24 ++++++++++++++++++------
> >  4 files changed, 46 insertions(+), 20 deletions(-)
>
> Sigh. lib/password.c was an external contribution that I've cleaned up a bit but
> that whole pile of commands (passwd, su, sudo, useradd, userdel, groupadd,
> groupdel...) are still kinda on the todo list until I get mkroot to the point it
> can use/test them.
>
> > diff --git a/lib/lib.h b/lib/lib.h
> > index 14bb7cf6..d51bad4a 100644
> > --- a/lib/lib.h
> > +++ b/lib/lib.h
> > @@ -306,6 +306,7 @@ int pollinate(int in1, int in2, int out1, int
> > out2, int timeout, int shutdown_ti
> >  char *ntop(struct sockaddr *sa);
> >
> >  // password.c
> > +void get_random_ascii(char *buf, int buflen);
> >  int get_salt(char *salt, char * algo);
> >
> >  // commas.c
> > diff --git a/lib/password.c b/lib/password.c
> > index b9cc1346..e3598989 100644
> > --- a/lib/password.c
> > +++ b/lib/password.c
> > @@ -8,6 +8,26 @@
> >  #include "toys.h"
> >  #include <time.h>
> >
> > +void get_random_ascii(char *buf, int buflen)
> > +{
> > +  int i;
> > +
> > +  // Read appropriate number of random bytes for salt
> > +  xgetrandom(libbuf, ((buflen*6)+7)/8, 0);
> > +
> > +  // Grab 6 bit chunks and convert to characters in ./0-9a-zA-Z
> > +  for (i=0; i<buflen; i++) {
> > +    int bitpos = i*6, bits = bitpos/8;
> > +
> > +    bits = ((libbuf[i]+(libbuf[i+1]<<8)) >> (bitpos&7)) & 0x3f;
> > +    bits += 46;
> > +    if (bits > 57) bits += 7;
> > +    if (bits > 90) bits += 6;
>
> That can return / (ascii 47) as part of the acceptable character set, which you
> do not want in mktemp's filename. (I was adjusting so all 64 entries were in the
> old "posix portable filename character set", which is "a-z", "A-Z", "0-9", and
> the punctuation ".", "-", and "_". The one I excluded was _ because it's off by
> itself in the ascii table and -. are adjacent.)

yes, i've already sent a replacement for the second patch.

we should still get the first patch in though.

> > +    buf[i] = bits;
> > +  }
> > +}
>
> My concern is that if xgetrandom() fails (called early in the boot, on an older
> kernel before /proc is mounted, etc) mktemp should still return something
> reasonable. (The mktemp out of the library code will, and if mktemp exits with
> no stdout output a calling script doing VAL=$(mktemp -u) won't necessarily catch
> the error.) So I added WARN_ONLY support to xgetrandom() and fallback code
> loosely modeled on what musl was doing in its mktemp().
>
> This is different from what passwd() should do: that's persistent key data that
> needs to be made with strong entropy, and when it can't do that it should exit
> with an error.

this still seems like something that should be fixed in xgetrandom
though. it already takes flags, and a "low quality is fine" flag
sounds like a better answer. (works for uuidgen too, for example.)

> Using libbuf does solve the length problem, although it seems unlikely that
> would ever come up for either use case? (64/6 is 10 characters of entropy, and
> an 11th with reduced range... Still, if it's in lib/ why not use libbuf...)

that's actually a bug in my latest patch: if you have more Xes than
fit in toybuf.

i can send a third patch to xgetrandom in-place in `template`. (or
another amend of the second patch if you prefer.)

but can we at least get the first of the patches in?

> > diff --git a/tests/mktemp.test b/tests/mktemp.test
> > index ee023d6b..0c235469 100755
> > --- a/tests/mktemp.test
> > +++ b/tests/mktemp.test
> > @@ -37,3 +37,9 @@ testing "-p DIR -t TEMPLATE but no TMPDIR" "TMPDIR=
> > mktemp -u -p DIR -t hello.XX
> >
> >  # mktemp -u doesn't need to be able to write to the directory.
> >  testing "-u" "mktemp -u -p /proc | grep -q '^/proc/tmp\...........$'
> > && echo yes" "yes\n" "" ""
> > +
> > +# mktemp needs at least XX in the template.
> > +testing "bad template" "mktemp -u helloX || echo error" "error\n" "" ""
> > +
> > +# mktemp -q shouldn't print the path.
> > +testing "-q" "mktemp -p /proc -q || echo only-failure" "only-failure\n" "" ""
> > diff --git a/toys/lsb/mktemp.c b/toys/lsb/mktemp.c
> > index 57d1d118..b9e144dc 100644
>
> Tests are good. Another patch with a mix of things I have to cherry pick out of.
> Probably over the weekend a this point...
>
> Rob