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

From: Jeff King <peff@peff.net>
Date: Nov 16, 2021, 21:06 UTC
Message-ID: <YZQdRC9nAA/CbMyM@coredump.intra.peff.net>
In-Reply-To: <211116.864k8bq0xm.gmgdl@evledraar.gmail.com>

On Tue, Nov 16, 2021 at 09:35:59PM +0100, Ævar Arnfjörð Bjarmason wrote:

Show 32 quoted lines

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

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

-Peff