Re: [PATCH 2/2] wrapper: use a CSPRNG to generate random file names
From: Jeff King <hidden>
Date: 2021-11-16 15:36:53
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.
quoted hunk ↗ jump to hunk
@@ -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