Re: [PATCH 0/2] Generate temporary files using a CSPRNG

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

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.