[Bug libc/15813] New: Multiple issues in __gen_tempname

[Bug libc/15813] New: Multiple issues in __gen_tempname

[bugdal at aerifal dot cx]

http://sourceware.org/bugzilla/show_bug.cgi?id=15813

            Bug ID: 15813
           Summary: Multiple issues in __gen_tempname
           Product: glibc
           Version: unspecified
            Status: NEW
          Severity: normal
          Priority: P2
         Component: libc
          Assignee: unassigned at sourceware dot org
          Reporter: bugdal at aerifal dot cx
                CC: drepper.fsp at gmail dot com

(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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15813] Multiple issues in __gen_tempname

[neleai at seznam dot cz]

https://sourceware.org/bugzilla/show_bug.cgi?id=15813

Ondrej Bilka <neleai at seznam dot cz> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |neleai at seznam dot cz
           Severity|normal                      |enhancement

--- Comment #1 from Ondrej Bilka <neleai at seznam dot cz> ---
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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15813] Multiple issues in __gen_tempname

[bugdal at aerifal dot cx]

https://sourceware.org/bugzilla/show_bug.cgi?id=15813

--- Comment #2 from Rich Felker <bugdal at aerifal dot cx> ---
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).

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15813] Multiple issues in __gen_tempname

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=15813

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |fweimer at redhat dot com

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15813] Multiple issues in __gen_tempname

[jakub at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=15813

Jakub Jelinek <jakub at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
         Resolution|FIXED                       |---
                 CC|                            |jakub at redhat dot com

--- Comment #6 from Jakub Jelinek <jakub at redhat dot com> ---
(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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15813] Multiple issues in __gen_tempname

[jakub at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=15813

--- Comment #7 from Jakub Jelinek <jakub at redhat dot com> ---
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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15813] Multiple issues in __gen_tempname

[adhemerval.zanella at linaro dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=15813

--- Comment #8 from Adhemerval Zanella <adhemerval.zanella at linaro dot org> ---
(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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15813] Multiple issues in __gen_tempname

[kdudka at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=15813

Kamil Dudka <kdudka at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |kdudka at redhat dot com

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15813] Multiple issues in __gen_tempname

[jakub at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=15813

--- Comment #9 from Jakub Jelinek <jakub at redhat dot com> ---
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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15813] Multiple issues in __gen_tempname

[adhemerval.zanella at linaro dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=15813

--- Comment #10 from Adhemerval Zanella <adhemerval.zanella at linaro dot org> ---
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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15813] Multiple issues in __gen_tempname

[carlos at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=15813

--- Comment #11 from Carlos O'Donell <carlos at redhat dot com> ---
(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.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15813] Multiple issues in __gen_tempname

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=15813

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://sourceware.org/bugz
                   |                            |illa/show_bug.cgi?id=26648

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/15813] Multiple issues in __gen_tempname

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=15813

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #12 from Florian Weimer <fweimer at redhat dot com> ---
I filed bug 26648 for the regression.

-- 
You are receiving this mail because:
You are on the CC list for the bug.