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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 16, 2021, 20:35 UTC
Message-ID: <211116.864k8bq0xm.gmgdl@evledraar.gmail.com>
In-Reply-To: <20211116033542.3247094-1-sandals@crustytoothpaste.net>

On Tue, Nov 16 2021, brian m. carlson wrote:

Show 19 quoted lines

> Currently, when we generate a temporary file name, we use the seconds,
> microseconds, and the PID to generate a unique value.  The resulting
> value, while changing frequently, is actually predictable and on some
> systems, it may be possible to cause a DoS by creating all potential
> temporary files when the temporary file is being created in TMPDIR.
>
> The solution to this is to use the system CSPRNG to generate the
> temporary file name.  This is the approach taken by FreeBSD, NetBSD, and
> OpenBSD, and glibc also recently switched to this approach from an
> approach that resembled ours in many ways.
>
> Even if this is not practically exploitable on many systems, it seems
> prudent to be at least as careful about temporary file generation as
> libc is.
>    
> This issue was mentioned on the security list and it was decided that
> this was not sensitive enough to warrant a coordinated disclosure, a
> sentiment with which I agree.  This is difficult to exploit on most
> systems, but I think it's still worth fixing.

I skimmed that report on the security list, and having skimmed this patch series I think what's missing is something like this summary of yours there (which I hope you don't mind me quoting):

    Now, in Git's case, I don't think our security model allows untrusted
    users to write directly into the repository, so I don't think this
    constitutes a vulnerability there.  We have a function that uses TMPDIR,
    which appears to be used for prepping temporary blobs in diffs and in
    GnuPG verification, which is definitely more questionable.

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.

Doesn't that entirely solve this security problem going forward? If a hostile actor can write into your .git/ they don't need to screw with you in this way, they can just write executable aliases, or the same in .git/hook/.

Unless that is we do have some use-case for potentially racing others in /tmp/, but then we could make that specifically configurable etc.

I really don't mind us having a better tempfile() function principle, but so far this sort of hardening just seems entirely unnecessary to me.

As seen from your implementation requires us top dip our toes into seeding random data, which I'd think from a security maintenance perspective we'd be much better offloading to the OS going forward if at all possible.

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?