On 2022-01-18 at 09:24:58, Ævar Arnfjörð Bjarmason wrote:
> 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...
Some OSes do have that, but it is not universal and we can't rely on
environment variables being set. They are stripped out by some
programs, including Homebrew, even on OSes where they're provided.
/run/user is also not a suitable temporary directory on Linux.
Temporary files can be large, and /run is almost always a tmpfs, which
means it sits entirely in memory. Setting anything in /run as TMPDIR
is a mistake.
> ...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.
Every Rust program or Go program includes code to call a CSPRNG because
it's required to avoid hash collision DoS attacks. I do plan to look
into that in the future, because my guess right now is that we are in
fact vulnerable to DoS attacks if someone creates crafted object IDs.[0]
When I do that, I'll need this code.
I also don't think adding code for a CSPRNG is an auditing problem. We
use the system CSPRNG, which is the thing that literally everybody
should be doing if they need good quality random numbers. If we were
shipping a custom CSPRNG, then that would be an auditing problem, but I
am explicitly not doing that because it's not necessary here and I'm
willing to insist that the system provide one somewhere so we don't have
to.
> 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* :)
I did explain it in the cover letter for v2, along with the explanation
in the paragraph above. The situation is that mkstemp doesn't handle
all our use cases, and Randall said in the followups to v1 that mkstemp
is not available on NonStop. I therefore must conclude that we'll need
to implement this somewhere, even if only as a fallback.
If we think mkstemp is going to work here and we don't need this, then
I'll drop this series. However, I am annoyed that I'm getting
conflicting information about what code is portable on different
platforms, which is made especially difficult by trying to support Unix
systems that don't support a 21-year-old standard and which lack
suitable public documentation. If I write and polish a series based on
a set of information I've been given and then it turns out that we
decide to do something completely different based on conflicting
information, that's not a motivator to continue sending patches. This
will not be the first time I've dropped a series after several rounds of
review because we totally decided to change course and do something
different, and I don't want to repeat this again.
[0] This type of attack is extremely trivial because the number of
collisions necessary for this attack is usually on the order of 2^16,
which means the work can be done in a couple seconds on a typical
system. I have a proof of concept of this for PHP online.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
Attachment:
signature.asc
Description: PGP signature
