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
