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

["René Scharfe" <l.s.r@web.de>]

Am 18.01.22 um 15:51 schrieb Ævar Arnfjörð Bjarmason:
>
> On Tue, Jan 18 2022, René Scharfe wrote:
>
>> Am 18.01.22 um 10:24 schrieb Ævar Arnfjörð Bjarmason:
>>>
>>> On Mon, Jan 17 2022, 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.  Even though we set TMP_MAX to 16384, this may be achievable
>>>> in some contexts, even if unlikely to occur in practice.
>>>> [...]
>>>
>>> I had a comment on v1[1] of this series which still applies here,
>>> i.e. the "if we're writing into TMPDIR[...]" here elides the fact that
>>> much of the time we're writing a tempfile into .git, so the security
>>> issue ostensibly being solved here won't be a practical issue at all.
>>>
>>> Then for out-of-repo tempfiles some OS's have a per-user $TEMPDIR you
>>> can use (e.g. systemd's /run/user/`id -u`). Finally...
>>>
>>>> Note that the use of a CSPRNG in generating temporary file names is also
>>>> used in many libcs.  glibc recently changed from an approach similar to
>>>> ours to using a CSPRNG, and FreeBSD and OpenBSD also use a CSPRNG in
>>>> this case.  Even if the likelihood of an attack is low, we should still
>>>> be at least as responsible in creating temporary files as libc is.
>>>
>>> ...we already have in-tree users of mkstemp(), which on glibc ostensibly
>>> tries to solve the same security issues you note here, and the
>>> reftable/* user has been added since earlier iterations of this series:
>>> o
>>>     $ git grep -E '\bmkstemp\(' -- '*.[ch]'
>>>     compat/mingw.c:int mkstemp(char *template)
>>>     compat/mingw.h:int mkstemp(char *template);
>>>     entry.c:                return mkstemp(path);
>>>     reftable/stack.c:       tab_fd = mkstemp(temp_tab_file_name.buf);
>>>     reftable/stack.c:       tab_fd = mkstemp(temp_tab->buf);
>>>     reftable/stack_test.c:  int fd = mkstemp(fn);
>>>     wrapper.c:      fd = mkstemp(filename_template);
>>>
>>> This series really feels like it's adding too much complexity and
>>> potential auditing headaches (distributors worrying about us shipping a
>>> CSPRNG, having to audit it) to a low-level codepath that most of the
>>> time won't need this at all.
>>
>> Good point.  Please let me think out loud for a moment.
>>
>> mkstemp() is secure (right?) and used already.  mkstemps() was used as
>> well until b2d593a779 (wrapper.c: remove unused gitmkstemps() function,
>> 2017-02-28).  What's the difference?  The former requires the random
>> characters at the end (e.g. "name-XXXXXX"), while the latter allows a
>> suffix (e.g. "name-XXXXXX.ext"); that's what the added "s" in the name
>> means, I guess.  And apparently mkstemp is everywhere, but mkstemps is
>> a GNU extension.
>>
>> git_mkstemps_mode is used by mks_tempfile_sm, mks_tempfile_tsm and
>> git_mkstemp_mode.  The latter uses no suffix, so could be implemented
>> using mkstemp and fchmod instead.
>>
>> mks_tempfile_sm is called by write_locked_index, mks_tempfile_s,
>> mks_tempfile_m and mks_tempfile.  Only mks_tempfile_s uses a suffix,
>> but is itself unused.  So an implementation based on mkstemp and fchmod
>> would suffice for mks_tempfile_sm users as well.
>>
>> mks_tempfile_tsm is used by mks_tempfile_ts, mks_tempfile_tm and
>> mks_tempfile_t.  Only mks_tempfile_ts uses a suffix.  Its only caller
>> is diff.c::prep_temp_blob, called only by diff.c::prepare_temp_file,
>> called by add_external_diff_name and run_textconv in the same file.
>>
>> So if I didn't take a wrong turn somewhere the only temporary file name
>> templates with suffixes are those for external diffs and textconv
>> filters.  The rest of the git_mkstemps_mode users could be switched to
>> mkstemp+fchmod.
>>
>> Temporary files for external diffs and textconv filters *should* be
>> placed in $TMPDIR.  Do they need suffixes?  I guess for testconv
>> filters it doesn't matter, but graphical diff viewers might do syntax
>> highlighting based on the extension.
>>
>> Can we get extensions without mktemps or git_mkstemps_mode?  Yes, by
>> using mkdtemp to create a temporary directory with a random name and
>> creating the files in it.  There already are mkdtemp users.
>>
>> So AFAICS all use cases of git_mkstemps_mode can be served by
>> mkstemp+fchmod or mkdtemp.  Would that help?
>
> That seems sensible, as a further practical suggestion it seems like
> we'll retry around 16k times to create these files on failure, perhaps
> trying with a custom extension 8k times would be OK, followed by the
> minor UI degradation of doing the final 8k retries with the more-random
> OS-provided extension-less variant.

You can't use the suffix-less mkstemp if a suffix is necessary.

Retries would be handled by mkstemp and mkdtemp IIUC.  To an extent.
E.g. https://cgit.freebsd.org/src/tree/lib/libc/stdio/mktemp.c seems
to just count up, which doesn't help if an attacker guessed the first
name correctly.  So maybe some kind of EEXIST loop is still necessary.

> More generally I'm still not sure if this is still a purely hypothetical
> attack mitigation, or whether there are actually users out there that
> have to deal with this.
>
> Wouldn't something like this also be an acceptable "solution" to this
> class of problem?
>
> 	#define TMP_MAX 1024
> 	[...]
> 	if (count >= TMP_MAX)
> 		die(_("we tried and failed to create a tempfile using the '%s' template after %d tries!\n\n"
>                     "Is someone actively DoS you? Is your sysadmin known to be your mortal enemy?\n"
>                     "are you using Satan's shared hosting services? In any case, we give up!\n\n"
>                     "To \"retry\" set TEMPDIR to some location where other users won't race us to death"),
>                     template, count);

You mean users should be educated to stay away from shared temporary
directories on multi-user systems?  Good advice.  I'm also not sure
how relevant it is these days -- doesn't everybody get their own VM
these days? :)  Anyway, there are probably some who cannot follow it,
e.g. because their $HOME quota is exhausted.

> For a bonus grade we could add a few more lines and try to stat() some
> of the files we failed to create, and report what UID it is that's
> racing you for all of these tempfile creations.

Sounds fun, can enliven the office.  Once the fisticuffs are over --
PLOT TWIST! Turns out the RNG handed out the same "random" numbers to
everyone. ;)

>
>>> So instead of:
>>>
>>>  A. Add CSPRNG with demo test helper
>>>  B. Use it in git_mkstemps_mode()
>>>
>>> I'd think we'd be much better off with:
>>>
>>>  A. Split out callers of tempfile.c and mk.*temp in wrapper.c that create tempfiles in .git
>>>  B. <the rest>
>>>
>>> I honestly haven't looked too much at what <the rest> is, other than
>>> what I wrote in [1], which seems to suggest that most of our codepaths
>>> won't need this.
>>>
>>> I'd also think that given the reference to CSPRNG in e.g. some glibc
>>> versions that instead of the ifdefs in csprng_bytes() we should instead
>>> directly use a secure mkstemp() (or similar) for the not-.git cases that
>>> remain after the "mktemp in a dir we chown" v.s. "mktemp in shared /tmp"
>>> are split up.
>>>
>>> Maybe these are all things you looked at and considered, but from my
>>> recollection (I didn't go back and re-read the whole discussion) you
>>> didn't chime in on this point, so *bump* :)
>>>
>>> 1. https://lore.kernel.org/git/211116.864k8bq0xm.gmgdl@evledraar.gmail.com/
>