Re: SHA1 collision in production repo?! (probably not)

On Fri, Mar 31, 2017 at 10:35:06AM -0700, Junio C Hamano wrote:

Lars Schneider [off-list ref] writes:

quoted

Hi,

I just got a report with the following output after a "git fetch" operation
using Git 2.11.0.windows.3 [1]:

remote: Counting objects: 5922, done.
remote: Compressing objects: 100% (14/14), done.
error: inflate: data stream error (unknown compression method)
error: unable to unpack 6acd8f279a8b20311665f41134579b7380970446 header
fatal: SHA1 COLLISION FOUND WITH 6acd8f279a8b20311665f41134579b7380970446 !
fatal: index-pack failed

I would be really surprised if we discovered a SHA1 collision in a production
repo. My guess is that this is somehow triggered by a network issue (see data
stream error). Any tips how to debug this?

Perhaps the first thing to do is to tweak the messages in builtin/index-pack.c
to help you identify which one of identical 5 messages is firing.

My guess would be that the code saw an object that came over the
wire, hashed it to learn that its object name is
6acd8f279a8b20311665f41134579b7380970446, noticed that it locally
already has the object with the same name, and tried to make sure
they have identical contents (otherwise, what came over the wire is
a successful second preimage attack).  But your loose object on disk
you already had was corrupt and didn't inflate correctly when
builtin/index-pack.c::compare_objects() or check_collision() tried
to.  The code saw no data, or truncated data, or
whatever---something different from what the other data that hashed
down to 6acd8..., and reported a collision when there is no
collision.

My guess is the one in compare_objects(). The "unable to unpack" error
comes from sha1_loose_object_info(). We'd normally then follow up with
read_sha1_file(), which would generate its own set of errors.

But if open_istream() got a bogus value for the object size (but didn't
correctly report an error), then it would probably quietly return 0
bytes from read_istream() later.

I suspect this may improve things, but I haven't dug deeper to see if
there are unwanted side effects, or if there are other spots that need
similar treatment.

diff --git a/sha1_file.c b/sha1_file.c
index 43990dec7..38411f90b 100644
--- a/sha1_file.c
+++ b/sha1_file.c

@@ -2952,7 +2952,7 @@ static int sha1_loose_object_info(const unsigned char *sha1,
 	if (status && oi->typep)
 		*oi->typep = status;
 	strbuf_release(&hdrbuf);
-	return 0;
+	return status;
 }
 
 int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi, unsigned flags)

-Peff