Discussion:
[PATCH] mktemp: yet more tests, and yet more fixes.
(too old to reply)
Rob Landley
2018-12-06 17:53:35 UTC
Permalink
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.)
+ 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.

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...)
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
Rob Landley
2018-12-07 15:41:06 UTC
Permalink
We can't reuse the password.c code for random ASCII salts because that
allows '/' (plus it seems to generate sequences of trailing '.'s for
some reason). Do the simplest thing that could possibly work instead.
I need a week sometime to properly put the user account management stuff into
mkroot and get it all promoted.

Part of the reason it's further down my todo list is it's no use to Android
because PIDs mean something else there and login data would go in your version
of the registry instead of being unix-style text files anyway...
---
tests/mktemp.test | 6 ++++++
toys/lsb/mktemp.c | 34 ++++++++++++++++++++++++++++------
2 files changed, 34 insertions(+), 6 deletions(-)
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" "" ""
The one on ubuntu 14.04 required three XXX, so that's what I checked for...
+# 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..55ab1aff 100644
--- a/toys/lsb/mktemp.c
+++ b/toys/lsb/mktemp.c
@@ -37,6 +37,8 @@ void mktemp_main(void)
int template_dir = template && !!strchr(template, '/');
int flags_dir = (toys.optflags & (FLAG_p|FLAG_t));
int use_dir = flags_dir && !template_dir;
+ char *s, *e;
+ int len;
if (template_dir && flags_dir) error_exit("conflicting directories given");
@@ -61,12 +63,32 @@ void mktemp_main(void)
// TODO: coreutils cleans paths, so -p /t/// would result in /t/xxx...
template = use_dir ? xmprintf("%s/%s", TT.p, template) : xstrdup(template);
- if (toys.optflags & FLAG_u) {
- template = mktemp(template);
mkstemp(template) == -1) {
- if (toys.optflags & FLAG_q) toys.exitval = 1;
- else perror_exit("Failed to create %s %s/%s",
- toys.optflags & FLAG_d ? "directory" : "file", TT.p, template);
+ // Point `s` and `e` to the start and end of the last region of XXXXXXes.
+ s = e = strrchr(template, 'X');
+ if (!e || e == template || *(e-1) != 'X') error_exit("need XX in template");
+ while (s >= template && *(s-1) == 'X') --s;
+ len = (e-s+1);
+
+ while (1) {
+ struct stat sb;
+ int i;
+
+ xgetrandom(toybuf, len, 0);
+ for (i = 0; i < len; ++i) {
+ // mktemp randomness is only from "A-Za-z0-9".
+ s[i] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+ "abcdefghijklmnopqrstuvwxyz"
+ "0123456789"[toybuf[i] % (26*2+10)];
+ }
I wanted to avoid a "long long" division on 32 bit systems pulling in the
function unnecessarily, and you only need 2 more chars for 64, and the "posix
portable file character set" thing has 3 more (- . and _).
+ if ((FLAG(u) && lstat(template, &sb) == -1 && errno == ENOENT) ||
+ (FLAG(d) && mkdir(template, 0700) != -1) ||
+ (open(template, O_CREAT|O_CLOEXEC, 0500) != -1)) break;
+ if (errno == EEXIST) continue;
+ if (FLAG(q)) {
+ toys.exitval = 1;
+ return;
+ } else perror_exit("%s", template);
I didn't see this until just now (see "list mass unsubscribe again"), but I'll
try to take a proper look this weekend and see what I missed.

Rob
Rob Landley
2018-12-08 01:12:51 UTC
Permalink
Post by Rob Landley
I didn't see this until just now (see "list mass unsubscribe again"), but I'll
try to take a proper look this weekend and see what I missed.
i think you missed an entire change? toybox ToT doesn't currently build. mktemp
assumes that xgetrandom returns bool and has a new WARN_ONLY flag, but
xgetrandom is void and doesn't have a special case for both getrandom and /dev
not being available...
Oops. Fixed.

Sorry about that.

Rob

Loading...