Re: [REGRESSION] Can't clone GitHub repos (fetch-pack error) due to avoiding deprecated OpenSSL SHA-1 routines

["brian m. carlson" <sandals@crustytoothpaste.net>]

[-- Attachment #1: Type: text/plain, Size: 2875 bytes --]

On 2023-08-31 at 12:47:19, Bagas Sanjaya wrote:
> Hi,
> 
> I built Git v2.42.0 on Debian testing, linked with OpenSSL (v3.0.10 from
> distribution) with Makefile knob `OPENSSL_SHA1=YesPlease 
> OPENSSL_SHA256=YesPlease`. I tried to shallow clone git.git repository:

I should point out that using OpenSSL's SHA-1 support is insecure
because it doesn't check for collisions.  As a practical matter, no
distro builds that way, and if you distributed that build, it would
probably qualify for a CVE.

However, OPENSSL_SHA256 being set is fine for a local build or a build
where you're not distributing OpenSSL itself.

> ```
> $ git clone https://github.com/git/git --depth=1 git-scm
> ```
> 
> All the necessary objects were fetched but the clone errored instead with:
> 
> ```
> fatal: fetch-pack: invalid index-pack output
> ```
> 
> This issue is a regression since v2.41.0 doesn't have it. Bisecting, the
> culprit is commit bda9c12073e7 (avoid SHA-1 functions deprecated in OpenSSL 3+,
> 2023-08-01). AFAIK, the culprit doesn't touch `fetch-pack.c` as I hoped.

I also see this with that configuration on Debian sid, and it appears to
affect SHA-256 as well.  The testsuite fails in a variety of spectacular
ways.  For example, t0410 is broken in exactly this way.

A simple git index-pack on Git's codebase shows a segfault in
`EVP_DigestUpdate` after calling `flush`.  It's not clear to me why this
would occur, but I did note that the context is a static variable.  I
wonder if there's something about this configuration that results in
breakage if a context is reused, although looking at the code, nothing
jumps out to me.

I did, however, apply this patch, which I think makes the problem really
clear:

----
diff --git a/sha1/openssl.h b/sha1/openssl.h
index 006c1f4ba5..0390ba9da6 100644
--- a/sha1/openssl.h
+++ b/sha1/openssl.h
@@ -32,6 +32,7 @@ static inline void openssl_SHA1_Final(unsigned char *digest,
 {
 	EVP_DigestFinal_ex(ctx->ectx, digest, NULL);
 	EVP_MD_CTX_free(ctx->ectx);
+	ctx->ectx = NULL;
 }
 
 static inline void openssl_SHA1_Clone(struct openssl_SHA1_CTX *dst,
----

Now we see that when the segfault happens, `input_ctx.sha1.ectx` is
NULL.  I'm not sure why that is, or what needs to be fixed, but I think
it's clear that _someone_ isn't calling the `init_fn` method before
re-using the context, and they definitely should be.

Hopefully this gives someone a good push in the right direction on
solving the problem.

If someone wants to pick up the above patch to help make this problem
more obvious in the future (don't forget to do the same for SHA-256),
please do so with my blessing.  I wouldn't say you need my sign-off
since it's so trivial, but feel free to forge it if it makes you feel
better.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]