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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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.

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/