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.
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);
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.
>> 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@xxxxxxxxxxxxxxxxxxx/
