Re: [PATCH 2/2] wrapper: use a CSPRNG to generate random file names

[Taylor Blau <me@ttaylorr.com>]

On Tue, Nov 16, 2021 at 10:36:51AM -0500, Jeff King wrote:
> On Tue, Nov 16, 2021 at 03:35:42AM +0000, brian m. carlson wrote:
>
> > The current way we generate random file names is by taking the seconds
> > and microseconds, plus the PID, and mixing them together, then encoding
> > them.  If this fails, we increment the value by 7777, and try again up
> > to TMP_MAX times.
> >
> > Unfortunately, this is not the best idea from a security perspective.
> > If we're writing into TMPDIR, an attacker can guess these values easily
> > and prevent us from creating any temporary files at all by creating them
> > all first.  POSIX only requires TMP_MAX to be 25, so this is achievable
> > in some contexts, even if unlikely to occur in practice.
>
> I think we unconditionally define TMP_MAX as 16384. I don't think that
> changes the fundamental issue that somebody could race us and win,
> though.

Yes, we do. Right above the declaration of this function (and so hidden
from the context) we do:

    #undef TMP_MAX
    #define TMP_MAX 16384

I don't think that the value of TMP_MAX makes this substantially less
likely, so I agree that the fundamental issue is the same.

> > @@ -485,12 +483,13 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
> >  	 * Replace pattern's XXXXXX characters with randomness.
> >  	 * Try TMP_MAX different filenames.
> >  	 */
> > -	gettimeofday(&tv, NULL);
> > -	value = ((uint64_t)tv.tv_usec << 16) ^ tv.tv_sec ^ getpid();
> >  	filename_template = &pattern[len - num_x - suffix_len];
> >  	for (count = 0; count < TMP_MAX; ++count) {
> > -		uint64_t v = value;
> >  		int i;
> > +		uint64_t v;
> > +		if (csprng_bytes(&v, sizeof(v)) < 0)
> > +			return -1;
>
> If csprng_bytes() fail, the resulting errno is likely to be confusing.
> E.g., if /dev/urandom doesn't exist we'd get ENOENT. But the caller is
> likely to say something like:
>
>   error: unable to create temporary file: no such file or directory
>
> which is misleading. It's probably worth doing:
>
>   return error_errno("unable to get random bytes for temporary file");
>
> or similar here. That's verbose on top of the error that the caller will
> give, but this is something we don't expect to fail in practice.
>
> I actually wonder if we should simply die() in such a case. That's not
> very friendly from a libification stand-point, but we really can't
> progress on much without being able to generate random bytes.

Alternatively, we could fall back to the existing code paths. This is
somewhat connected to my suggestion to Randall earlier in the thread.
But I would rather see that fallback done at compile-time for platforms
that don't give us an easy-to-use CSPRNG, and avoid masking legitimate
errors caused from trying to use a CSPRNG that should exist.

Thanks,
Taylor