On Tue, Nov 16 2021, Jeff King wrote:
> On Tue, Nov 16, 2021 at 09:35:59PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> I tried testing this codepath real quick now with:
>>
>> diff --git a/wrapper.c b/wrapper.c
>> index 36e12119d76..2f3755886fb 100644
>> --- a/wrapper.c
>> +++ b/wrapper.c
>> @@ -497,6 +497,7 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
>> v /= num_letters;
>> }
>>
>> + BUG("%s", pattern);
>> fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
>> if (fd >= 0)
>> return fd;
>>
>> And then doing:
>>
>> grep BUG test-results/*.out
>>
>> And the resulting output is all of the form:
>>
>> .git/objects/9f/tmp_obj_FOzEcZ
>> .git/objects/pack/tmp_pack_fJC0RI
>>
>> And a couple of:
>>
>> .git/info/refs_Lctaew
>>
>> I.e. these are all cases where we're creating in-repo tempfiles, we're
>> not racing someone in /tmp/ for these, except perhaps in some cases I've
>> missed (but you allude to) where we presumably should just move those
>> into .git/tmp/, at least by default.
>
> Your patch is way too aggressive. By bailing via BUG(), most commands
> will fail, so we never get to the interesting ones (e.g., we would not
> ever get to the point of writing out a tag signature for gpg to verify,
> because we'd barf when trying to create the tag in the first place).
>
> Try:
>
> diff --git a/wrapper.c b/wrapper.c
> index 36e12119d7..5218a4b3bd 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -497,6 +497,10 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)
> v /= num_letters;
> }
>
> + {
> + static struct trace_key t = TRACE_KEY_INIT(TEMPFILE);
> + trace_printf_key(&t, "%s", pattern);
> + }
> fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);
> if (fd >= 0)
> return fd;
>
> And then:
>
> GIT_TRACE_TEMPFILE=/tmp/foo make test
> grep ^/tmp /tmp/foo | wc -l
>
> turns up hundreds of hits.
Thanks, there's a long tail of these, but I came up with this crappy
one-liner one regex at a time while looking at it:
cat /tmp/git_mkstemps_mode.trace | perl -pe 's[/[0-9a-f]{2}/][/HH/]; s[/incoming-\K[^/]+][XXX]; s[/tmp/\K[^_]+][XXX]; s/tmp_(idx|obj|pack)_\K[a-zA-Z0-9]+$/XXX/; s[/objects/\
K../][$1??/]g; s[^/run/user.*/objects/][<systemd run/user>/objects/]; s[(vtag_tmp|pack_|refs_)\K.*][XXX]; '|sort|uniq -c|sort -nr|less
Which gives us:
893 .git/objects/pack/tmp_pack_XXX
836 ./objects/??/tmp_obj_XXX
722 .git/objects/pack/tmp_idx_XXX
401 <systemd run/user>/objects/incoming-XXX/HH/tmp_obj_XXX
366 /run/user/1001/tmp/XXX_pack_XXX
289 <systemd run/user>/objects/??/tmp_obj_XXX
261 .git/info/refs_XXX
258 /tmp/XXX_vtag_tmpXXX
185 clone.git/objects/??/tmp_obj_XXX
77 /tmp/XXX_file
72 marks-test/.git/objects/??/tmp_obj_XXX
71 <systemd run/user>/objects/pack/tmp_pack_XXX
69 <systemd run/user>/objects/pack/tmp_idx_XXX
34 objects/pack/tmp_pack_XXX
34 objects/pack/tmp_idx_XXX
25 /run/user/1001/tmp/XXX.git/objects/??/tmp_obj_XXX
20 info/refs_XXX
12 /tmp/XXX_text
12 foo.git/objects/??/tmp_obj_XXX
I.e. this is stuff that's either already in .git, or a small handful of
special-cases such as "git verify-tag".
>> If there are cases where we actually need this hardening because we're
>> writing in a shared /tmp/ and not .git/, then surely we're better having
>> those API users call a differently named function, or to move those
>> users to using a .git/tmp/ unless they configure things otherwise?
>
> Assuming you can write to .git/tmp means that conceptually read-only
> operations (like verifying tags) require write access to the repository.
That leaves the "differently named function" which I think we should
really do in either case.
I.e. if I'm verifying lots of tags then I'm better off on a modern
systemd system using /run/user/`id -u`, as opposed to /tmp/ which is
often disk-backed. So being aware of $XDG_RUNTIME_DIR seems like a
sensible thing in either case.
And on those systems the DoS aspect of this becomes a non-issue, that
directory is only writable by one (non-super)user.
I think there's a big advantage to having any tricky CSPRNG-implementing
code in its own corner like that.
It means that e.g. if gpg learns some mode to do this that doesn't
require tempfiles, and we're confident we don't create things in /tmp
otherwise that we could drop it, or users who don't want git shipping a
CSPRNG can compile it out.
But I really don't see why it isn't an acceptable solution for git to
just die here if we fail to create the Nth tempfile in a row.
Or something simpler like having the "git verify-tag" code fall back to
writing in say $HOME/.cache/git, which is another simple way to avoid
the issue entirely in most cases.
