Am 18.01.22 um 15:51 schrieb Ævar Arnfjörð Bjarmason:

On Tue, Jan 18 2022, René Scharfe wrote:

quoted

Am 18.01.22 um 10:24 schrieb Ævar Arnfjörð Bjarmason:

quoted

On Mon, Jan 17 2022, brian m. carlson wrote:

quoted

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...

quoted

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. ;)

quoted

quoted

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/ (local)