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?
> 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/
