Re: [PATCH 0/7] PREVIEW: Introduce DC_AND_OPENSSL_SHA1 make flag

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> On Fri, Mar 24, 2017 at 11:37:54PM -0700, Junio C Hamano wrote:
>
>> The hash that names a packfile is constructed by sorting all the
>> names of the objects contained in the packfile and running SHA-1
>> hash over it.  I think this MUST be hashed with collision-attack
>> detection.  A malicious site can feed you a packfile that contains
>> objects the site crafts so that the sorted object names would result
>> in a collision-attack, ending up with one pack that contains a sets
>> of objects different from another pack that happens to have the same
>> packname, causing Git to say "Ah, this new pack must have the same
>> set of objects as the pack we already have" and discard it,
>> resulting in lost objects and a corrupt repository with missing
>> objects.
>
> I don't think this case really matters for collision detection. What's
> important is what Git does when it receives a brand-new packfile that
> would overwrite an existing one. It _should_ keep the old one, under the
> usual "existing data wins" rule.

If a malicious site can craft two packfiles that hash to the same,
then it can first feed one against a fetch request, then feed the
other one when a later fetch request comes, and then the later pack
is discarded by the "existing data wins" rule.  Even though this
later pack may have all the objects necessary to complete the refs
this later fetch request asked for, and an earlier pack that
happened to have the same pack trailer hash doesn't have these
necessary objects, the receiver ends up discarding this necessary
pack.  The end result is that the receiver's repository is now
corrupt, lacking some objects.

It is a different matter if such an "attack" is a useful one to
conduct from attacker's point of view. 

I highlighted this case as notable because the way the trailing hash
is also used as the name of packfile makes this fall into the same
category as object hash, in that the hash is used for identification
and deduplication (because we have a rule that says there can be
only one _thing_ that hashes to one value), for which we do want to
use the collision-attack detecting kind of hashing, even though it
otherwise should fall into the other category (i.e. use of csum-file
API to compute trailer hash), where the hash is used merely for
bit-flip detection (we are perfectly OK if you have multiple index
files with different contents that happen to have the same hash in
the trailer, because the hash is not used for identificaiton and
deduplication).