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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 26, 2017, 23:16 UTC
Message-ID: <xmqqinmv4ojt.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170326061826.yx6nh3k2ps6uyyz6@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 18 quoted lines

> 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).