Re: [PATCH v3 2/2] wrapper: use a CSPRNG to generate random file names
- From
René Scharfe <l.s.r@web.de>
- Date
- Jan 18, 2022, 14:42 UTC
- Message-ID
- <3a2b17ac-6ce2-cc29-6993-a9718df61741@web.de>
- In-Reply-To
- <220118.86zgntpegy.gmgdl@evledraar.gmail.com>
Am 18.01.22 um 10:24 schrieb Ævar Arnfjörð Bjarmason:
Show 46 quoted lines
>
> 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?
Show 25 quoted lines
> 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/