(1) Access to the static object value is unsynchronized, resulting in undefined behavior. Undefined behavior is not desirable entropy. (2) Low-resolution gettimeofday rather than high-resolution clock_gettime is used as an entropy source. (3) Entropy is only gathered once per run; subsequent attempts merely add 7777 to value, so that if an attacker can guess the initial temp name that will be tried, the attacker can also guess all subsequent attempts for the same run. Proposed solutions: (1) Make value automatic. There is no value (pardon the pun) to keeping it between runs. (2) Use clock_gettime, possibly with multiple clocks (e.g. realtime and cputime). (3) Get new entropy on each attempt rather than adding the fixed value 7777.
I do not see how could attacker use __gen_tempname weakness, worst he could do is dos/ cause mkxtemp to fail which should be handled correctly. If you want this fixed write a patch. keeping value is more entropic than calculating anew as entropy of sum of uncorrelated variables is at least maximum of entropies of variables. Without that we would call clock_gettime twice in quick succession which has almost same entropy as calling it once. As __gen_tempname call does disk access we can affort on linux just read 64bits from /dev/urandom. If attacker can guess that we have bigger worries than temporary files.
Issue 1, the undefined behavior, is the most serious. All cases of UB should be fixed; this should simply be a set-in-stone policy. Issue 2 is low-priority, but switching to a higher-quality entropy source would be the easiest way to solve issue 3 and would improve the quality of the simplest solution to issue 1 (removing the static state). Issue 3 is possibly an attack vector, but fairly low priority (DoS only).
(In reply to Rich Felker from comment #0) > (1) Access to the static object value is unsynchronized, resulting in > undefined behavior. Undefined behavior is not desirable entropy. > > (2) Low-resolution gettimeofday rather than high-resolution clock_gettime is > used as an entropy source. > > (3) Entropy is only gathered once per run; subsequent attempts merely add > 7777 to value, so that if an attacker can guess the initial temp name that > will be tried, the attacker can also guess all subsequent attempts for the > same run. > > Proposed solutions: > > (1) Make value automatic. There is no value (pardon the pun) to keeping it > between runs. > > (2) Use clock_gettime, possibly with multiple clocks (e.g. realtime and > cputime). > > (3) Get new entropy on each attempt rather than adding the fixed value 7777. (1) and (2) were addressed by https://sourceware.org/ml/libc-alpha/2019-02/msg00465.html https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=359653aaacad463d916323f03c0ac3c47405aafa#patch5
The master branch has been updated by Adhemerval Zanella <azanella@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=e1df30fbc2e2167a982c0e77a7ebee28f4dd0800 commit e1df30fbc2e2167a982c0e77a7ebee28f4dd0800 Author: Adhemerval Zanella <adhemerval.zanella@linaro.org> Date: Thu Jul 25 11:22:17 2019 -0300 Get new entropy on each attempt __gen_tempname (BZ #15813) This is missing bit for fully fix BZ#15813 (the other two were fixed by 359653aaacad463). Checked on x86_64-linux-gnu. [BZ #15813] sysdeps/posix/tempname.c (__gen_tempname): get entrypy on each attempt.
Fixed on 2.31 (commit id e1df30fbc2e2167a982c0e77a7ebee28f4dd0800). Waiting for the creation of 2.31 milestone to update the bug report.
(In reply to cvs-commit@gcc.gnu.org from comment #4) > The master branch has been updated by Adhemerval Zanella > <azanella@sourceware.org>: > > https://sourceware.org/git/gitweb.cgi?p=glibc.git; > h=e1df30fbc2e2167a982c0e77a7ebee28f4dd0800 > > commit e1df30fbc2e2167a982c0e77a7ebee28f4dd0800 > Author: Adhemerval Zanella <adhemerval.zanella@linaro.org> > Date: Thu Jul 25 11:22:17 2019 -0300 > > Get new entropy on each attempt __gen_tempname (BZ #15813) > > This is missing bit for fully fix BZ#15813 (the other two were fixed > by 359653aaacad463). > > Checked on x86_64-linux-gnu. > > [BZ #15813] > sysdeps/posix/tempname.c (__gen_tempname): get entrypy on each > attempt. This change is completely broken. As random_bits is defined as: static inline uint32_t random_bits (void) { struct __timespec64 tv; __clock_gettime64 (CLOCK_MONOTONIC, &tv); /* Shuffle the lower bits to minimize the clock bias. */ uint32_t ret = tv.tv_nsec ^ tv.tv_sec; ret ^= (ret << 24) | (ret >> 8); return ret; } on a fast machine instead of making sure that in the loop the value is even more random it makes sure that each attempt uses the exact same value, which it knows from the earlier attempt that it exists already. So, either it needs to ensure the entropy source is not imprecise time based, or it needs e.g. add to the initial value in each iteration rather than use the same value.
Perhaps upon the first EEXIST it could getrandom (with GRND_NONBLOCK) and from that decide the addend to be used instead of 7777 and use RANDOM_BITS only before the loop as the base? The code should make sure the addend isn't something like 0 or that would cause only very few possibilities in the TMP_MAX attempts. And/or the addend could change every few hundred attempts.
(In reply to Jakub Jelinek from comment #7) > Perhaps upon the first EEXIST it could getrandom (with GRND_NONBLOCK) and > from that decide the addend to be used instead of 7777 and use RANDOM_BITS > only before the loop as the base? The code should make sure the addend > isn't something like 0 or that would cause only very few possibilities in > the TMP_MAX attempts. And/or the addend could change every few hundred > attempts. I am more inclined to sync with latest gnulib implementation and just remove the RANDOM_BITS part which is used only for _LIBC. This makes the implementation to use getrandom or it fallbacks to a 64-bit linear congruential generator. The fallback will be always used on kernels older than 3.17 (due missing __NR_getrandom support) and I am wondering if it is worth to implement the fallback to read the random device (as Hurd implementation). Also, the gnulib fallback relies on UB (the linear congruential generator uses the unitialized value a stack variable), so I think it would be better to fix it as well. A simple solution might to initialize the variable to its own address.
The non-_LIBC fallback is actually worse (gettimeofday with usec precision). Anyway, I think the usec timer with pid is just fine for the first attempt, so reading from /dev/urandom or using similar syscall is I think a waste of time for the common case, it is just in the unlikely case that the file exists that perhaps something more expensive is needed.
I meant the updated gnulib version from its repository, not the outdated version glibc packs now. The gnulib version currently does not use neither the process pid nor the clock for source of entropy, it uses either getrandom or a simple linear congruential generator if getrandom fails. I think using getentropy where available should make it more robust. These interface contains inherent concurrent issues and applications should either use O_TMPFILE or at leat tmpfile.
(In reply to Adhemerval Zanella from comment #10) > I meant the updated gnulib version from its repository, not the outdated > version glibc packs now. The gnulib version currently does not use neither > the process pid nor the clock for source of entropy, it uses either > getrandom or a simple linear congruential generator if getrandom fails. > > I think using getentropy where available should make it more robust. These > interface contains inherent concurrent issues and applications should either > use O_TMPFILE or at leat tmpfile. We should backport this to the release branch also since this is a regression.
I filed bug 26648 for the regression.