Re: [PATCH v2 8/8] tests: mark tests broken under GIT_TEST_PROTOCOL_VERSION=2

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Dec 17, 2018, 23:14 UTC
Message-ID: <20181217231452.GA13835@google.com>
In-Reply-To: <20181217195713.GA10673@sigill.intra.peff.net>

Hi,

Jeff King wrote:

> On Fri, Dec 14, 2018 at 11:55:30AM +0100, Ævar Arnfjörð Bjarmason wrote:

Show 6 quoted lines

>> More importantly this bypasses the security guarantee we've had with the
>> default of uploadpack.allowAnySHA1InWant=false.
>
> IMHO those security guarantees there are overrated (due to delta
> guessing attacks, though things are not quite as bad if the attacker
> can't actually push to the repo).

Do you have a proof of concept for delta guessing? My understanding was that without using a broken hash (e.g. uncorrected SHA-1), it is not feasible to carry out.

JGit checks delta bases in received thin packs for reachability as well.

> But I agree that people do assume it's the case. I was certainly
> surprised by the v2 behavior, and I don't remember that aspect being
> discussed.

IMHO it's a plain bug (either in implementation or documentation).

[...]

Show 15 quoted lines

>> I'm inclined to say that in the face of that "SECURITY" section we
>> should just:
>>
>>  * Turn on uploadpack.allowReachableSHA1InWant for v0/v1 by
>>    default. Make saying uploadpack.allowReachableSHA1InWant=false warn
>>    with "this won't work, see SECURITY...".
>>
>>  * The uploadpack.allowTipSHA1InWant setting will also be turned on by
>>    default, and will be much faster, since it'll just degrade to
>>    uploadpack.allowReachableSHA1InWant=true and we won't need any
>>    reachability check. We'll also warn saying that setting it is
>>    useless.
>
> No real argument from me. I have always thought those security
> guarantees were BS.

This would make per-branch ACLs (as implemented both by Gerrit and gitolite) an essentially useless feature, so please no.

I would be all for changing the default, but making turning off allowReachableSHA1InWant an unsupported deprecated thing is a step too far, in my opinion.

Is there somewhere that we can document these kinds of invariants or goals so that we don't have to keep repeating the same discussions?

Thanks, Jonathan