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

[Jeff King <peff@peff.net>]

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.

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

-Peff