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:
signature.asc
Description: PGP signature
