Re: [PATCH 0/4] hash.h: support choosing a separate SHA-1 for non-cryptographic uses

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Sep 4, 2024, 14:01 UTC
Message-ID: <ZthoII9swrVNLIqZ@tapette.crustytoothpaste.net>
In-Reply-To: <Ztdn25zfi8WHO+GS@nand.local>

On 2024-09-03 at 19:47:39, Taylor Blau wrote:

Show 12 quoted lines

> We still run any packs through index-pack before landing them in
> $GIT_DIR/objects/pack, and index-pack still uses the collision-detecting
> SHA-1 implementation (if the repository uses SHA-1 and Git was compiled
> with it).
> 
> So if I were a malicious attacker trying to compromise data on a forge,
> I would have to first (a) know the name of some pack that I was trying
> to collide, then (b) create a pack which collides with that one before
> actually pushing it. (b) seems difficult to impossible to execute
> (certainly today, maybe ever) because the attacker only controls the
> object contents within the pack, but can't adjust the pack header,
> object headers, compression, etc.

Packing single-threaded is deterministic in my tests, so it would seem that this is possible, even if inconvenient or difficult to execute. It's not very hard to get access to the configuration a forge is using either because it's open source or open core, or just from getting the on-premises version's configuration, so we have to assume that the attacker knows the configuration, and we also can determine what packs are on the server side if we've pushed all of the objects ourselves.

Show 5 quoted lines

> But even if the attacker could do all of that, the remote still needs to
> index that pack, and while checksumming the pack, it would notice the
> collision (or SHA-1 mismatch) and reject the pack by die()-ing either
> way. (AFAICT, this all happens in
> builtin/index-pack.c::parse_pack_objects()).

If we're certain that we'll always index the pack, then I agree we would detect this at that point, and so it would probably be safe. As you and I discussed elsewhere, I'm not the expert on the pack code, so I'll defer to your analysis here.

-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA