On Fri, Dec 14 2018, Ævar Arnfjörð Bjarmason wrote:
> On Fri, Dec 14 2018, Jeff King wrote:
>
>> On Thu, Dec 13, 2018 at 05:08:43PM +0100, Ævar Arnfjörð Bjarmason wrote:
>>
>>> Now that we have this maybe we should discuss why these tests show
>>> different things:
>>>
>>> > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
>>> > index 086f2c40f6..8b1217ea26 100755
>>> > --- a/t/t5500-fetch-pack.sh
>>> > +++ b/t/t5500-fetch-pack.sh
>>> > @@ -628,7 +628,10 @@ test_expect_success 'fetch-pack cannot fetch a raw sha1 that is not advertised a
>>> > test_commit -C server 6 &&
>>> >
>>> > git init client &&
>>> > - test_must_fail git -C client fetch-pack ../server \
>>> > +
>>> > + # Other protocol versions (e.g. 2) allow fetching an unadvertised
>>> > + # object, so run this test with the default protocol version (0).
>>> > + test_must_fail env GIT_TEST_PROTOCOL_VERSION= git -C client fetch-pack ../server \
>>> > $(git -C server rev-parse refs/heads/master^) 2>err &&
>>>
>>> What? So the equivalent of uploadpack.allowAnySHA1InWant=true is on for
>>> v2 all the time?
>>
>> Yeah, I actually didn't realize it until working on the earlier series,
>> but this is the documented behavior:
>>
>> $ git grep -hC3 'want <oid>' Documentation/technical/protocol-v2.txt
>>
>> A `fetch` request can take the following arguments:
>>
>> want <oid>
>> Indicates to the server an object which the client wants to
>> retrieve. Wants can be anything and are not limited to
>> advertised objects.
>>
>> An interesting implication of this at GitHub (and possibly other
>> hosters) is that it exposes objects from shared storage via unexpected
>> repos. If I fork torvalds/linux to peff/linux and push up object X, a v0
>> fetch will (by default) refuse to serve it to me. But v2 will happily
>> hand it over, which may confuse people into thinking that the object is
>> (or at least at some point was) in Linus's repository.
>
> More importantly this bypasses the security guarantee we've had with the
> default of uploadpack.allowAnySHA1InWant=false.
>
> If I push a id_rsa to a topic branch on $githost and immediately realize
> my mistake and delete it, someone with access to a log of SHA-1s
> (e.g. an IRC bot spewing out from/to commits) won't be able to pull it
> down. This is why we have that as a setting in the first place
> (f8edeaa05d ("upload-pack: optionally allow fetching any sha1",
> 2016-11-11)).
>
> Of course this is:
>
> a) Subject to the "SECURITY" section of git-fetch, i.e. maybe this
> doesn't even work in the face of a determined attacker.
>
> b) Hosting sites like GitHub, GitLab, Gitweb etc. aren't doing
> reachability checks when you view /commit/<id>. If I delete my topic
> branch it'll be viewable until the next GC kicks in (which would
> also be the case with uploadpack.allowAnySHA1InWant=true).
>
> So I wonder how much we need to care about this in practice, but for
> some configurations protocol v2 definitely breaks a security promise
> we've been making, now whether that promise was always BS due to "a)"
> above is another matter.
>
> 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.
>
> Otherwise what's th point of these settings given what we're talking
> about in that "SECURITY" section? It seems to just lure users into a
> false sense of security. Better to not make promises we're not confident
> we can keep, and say that if you push your id_rsa you need to run a
> gc/prune on the server.
I sent that too fast. What I meant is that there's 3
settings. uploadpack.{allowTipSHA1InWant,allowReachableSHA1InWant,allowAnySHA1InWant}. Those
are, respectively, cheap/expensive/cheap to serve up.
We could make them cheap/cheap/cheap by just making the first two a
synonym for the 3rd (allowAnySHA1InWant), because as noted above
Also, parts of the v2 code, e.g. this in 685fbd3291 ("fetch-pack:
perform a fetch using v2", 2018-03-15):
/* v2 supports these by default */
allow_unadvertised_object_request |= ALLOW_REACHABLE_SHA1;
Seem to have intended to turn on allowReachableSHA1InWant, not
allowAnySHA1InWant, but apparently that's what we're doing?
In any case, regardless of what v2 intended there anything except having
allowAnySHA1InWant on by default seems irresponsible given x) us
documenting in "SECURITY" that it doesn't really work y) even if it did,
there being a *tiny* minority of people who'd in some way care about
this feature who don't also run some sort of web UI on the git server
that'll be bypassing this setting no matter if it worked as intended for
the wire protocol.
