On Mon, Dec 17 2018, Jonathan Nieder wrote: > Hi, > > Jeff King wrote: >> On Fri, Dec 14, 2018 at 11:55:30AM +0100, Ævar Arnfjörð Bjarmason wrote: > >>> 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). > > [...] >>> 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. Doesn't Gerrit always use JGit? The discussion upthread is about what we should do about git.git's version of this feature since we document it doesn't do its job from a security / ACL standpoint, but that doesn't preclude other server implementations from having a working version of this. But if gitolite is relying on this to hide refs, and if our security docs are to be believed, then it's just providing security through obscurity. Like you I'm curious about a POC. The patch I submitted in <20181217221625.1523-1-avarab@xxxxxxxxx> takes the "SECURITY" docs at face value. I.e. I think it's dangerous to expose this as-is given the scary non-promise they make, but perhaps we'll find that this actually works well enough in some cases, or there's other non-security uses for this (e.g. simply discouraging users from cloning certain things, e.g. for cache performance purposes). > 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
