| Message ID | 20241002232618.GA3442753@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4638250b7b9288c197c16600630cefd4e196b4fc |
| Headers | show |
| Series | [(v2.47,regression)] hash.h: set NEEDS_CLONE_HELPER_UNSAFE in fallback mode | expand |
On 2024-10-02 at 23:26:18, Jeff King wrote: > This is a regression in v2.47.0-rc0. As mentioned above, I kind of doubt > anybody will hit it in practice (I only did because I was trying to do > some timing tests between the fast and dc variants). And it is almost > tempting to leave it as a wake-up call for anybody who is still not > using a collision-detecting sha1. ;) I think this is a fine fix for 2.47. I have a branch on my remote (sha1-dc-only), which I'll send out after it passes CI (probably later this week), that removes support for the everything but SHA-1-DC (except for the unsafe code). I don't think there's a reasonable configuration where people can use Git with other SHA-1 code except in extremely limited circumstances we shouldn't have to maintain code for. The code is open source, so people who really must have maximum performance with all of the vulnerabilities can patch it back in themselves.
On Thu, Oct 03, 2024 at 12:13:47AM +0000, brian m. carlson wrote: > On 2024-10-02 at 23:26:18, Jeff King wrote: > > This is a regression in v2.47.0-rc0. As mentioned above, I kind of doubt > > anybody will hit it in practice (I only did because I was trying to do > > some timing tests between the fast and dc variants). And it is almost > > tempting to leave it as a wake-up call for anybody who is still not > > using a collision-detecting sha1. ;) > > I think this is a fine fix for 2.47. I have a branch on my remote > (sha1-dc-only), which I'll send out after it passes CI (probably later > this week), that removes support for the everything but SHA-1-DC (except > for the unsafe code). > > I don't think there's a reasonable configuration where people can use > Git with other SHA-1 code except in extremely limited circumstances we > shouldn't have to maintain code for. The code is open source, so people > who really must have maximum performance with all of the vulnerabilities > can patch it back in themselves. Yeah, I feel the same way. I only happened to try this because it was the easiest way to speed-compare different implementations using "test-tool sha1". ;) Possibly that helper could grow an option to use the unsafe variant, though even that is probably not a high priority. -Peff
On Wed, Oct 02, 2024 at 07:26:18PM -0400, Jeff King wrote: > diff --git a/hash.h b/hash.h > index f97f858307..756166ce5e 100644 > --- a/hash.h > +++ b/hash.h > @@ -82,6 +82,9 @@ > # ifdef platform_SHA1_Clone > # define platform_SHA1_Clone_unsafe platform_SHA1_Clone > # endif > +# ifdef SHA1_NEEDS_CLONE_HELPER > +# define SHA1_NEEDS_CLONE_HELPER_UNSAFE > +# endif > #endif Gah. I could have sworn I wrote something like this myself, but I must have dropped it or hallucinated writing it in the first place. Thanks for finding and fixing. Thanks, Taylor
On Wed, Oct 02, 2024 at 08:21:40PM -0400, Jeff King wrote: > On Thu, Oct 03, 2024 at 12:13:47AM +0000, brian m. carlson wrote: > > > On 2024-10-02 at 23:26:18, Jeff King wrote: > > > This is a regression in v2.47.0-rc0. As mentioned above, I kind of doubt > > > anybody will hit it in practice (I only did because I was trying to do > > > some timing tests between the fast and dc variants). And it is almost > > > tempting to leave it as a wake-up call for anybody who is still not > > > using a collision-detecting sha1. ;) > > > > I think this is a fine fix for 2.47. I have a branch on my remote > > (sha1-dc-only), which I'll send out after it passes CI (probably later > > this week), that removes support for the everything but SHA-1-DC (except > > for the unsafe code). > > > > I don't think there's a reasonable configuration where people can use > > Git with other SHA-1 code except in extremely limited circumstances we > > shouldn't have to maintain code for. The code is open source, so people > > who really must have maximum performance with all of the vulnerabilities > > can patch it back in themselves. > > Yeah, I feel the same way. I only happened to try this because it was > the easiest way to speed-compare different implementations using > "test-tool sha1". ;) I imagine that you both mean that non-collision detecting variants are unsuitable for the "safe" SHA-1 implementation, and that the "unsafe" variant can still be driven with BLK_SHA1, OpenSSL, etc. And reading the patch at the tip of brian's 'sha1-dc-only' branch, that looks to be the case. So I'm in agreement with the both of you ;-). > Possibly that helper could grow an option to use the unsafe variant, > though even that is probably not a high priority. Yeah, that would be nice. Though I agree it's not a huge priority. Thanks, Taylor
Taylor Blau <me@ttaylorr.com> writes: > On Wed, Oct 02, 2024 at 07:26:18PM -0400, Jeff King wrote: >> diff --git a/hash.h b/hash.h >> index f97f858307..756166ce5e 100644 >> --- a/hash.h >> +++ b/hash.h >> @@ -82,6 +82,9 @@ >> # ifdef platform_SHA1_Clone >> # define platform_SHA1_Clone_unsafe platform_SHA1_Clone >> # endif >> +# ifdef SHA1_NEEDS_CLONE_HELPER >> +# define SHA1_NEEDS_CLONE_HELPER_UNSAFE >> +# endif >> #endif > > Gah. I could have sworn I wrote something like this myself, but I must > have dropped it or hallucinated writing it in the first place. > > Thanks for finding and fixing. Yeah, thanks, all.
diff --git a/hash.h b/hash.h index f97f858307..756166ce5e 100644 --- a/hash.h +++ b/hash.h @@ -82,6 +82,9 @@ # ifdef platform_SHA1_Clone # define platform_SHA1_Clone_unsafe platform_SHA1_Clone # endif +# ifdef SHA1_NEEDS_CLONE_HELPER +# define SHA1_NEEDS_CLONE_HELPER_UNSAFE +# endif #endif #define git_SHA_CTX platform_SHA_CTX
Commit 253ed9ecff (hash.h: scaffolding for _unsafe hashing variants, 2024-09-26) introduced the concept of having two hash algorithms: a safe and an unsafe one. When the Makefile knobs do not explicitly request an unsafe one, we fall back to using the safe algorithm. However, the fallback to do so forgot one case: we should inherit the NEEDS_CLONE_HELPER flag from the safe variant. Failing to do so means that we'll end up defining two clone functions (the algorithm specific one, and the generic one that just calls memcpy). You'll see an error like this: $ make OPENSSL_SHA1=1 [...] sha1/openssl.h:46:29: error: redefinition of ‘openssl_SHA1_Clone’ 46 | #define platform_SHA1_Clone openssl_SHA1_Clone | ^~~~~~~~~~~~~~~~~~ hash.h:83:40: note: in expansion of macro ‘platform_SHA1_Clone’ 83 | # define platform_SHA1_Clone_unsafe platform_SHA1_Clone | ^~~~~~~~~~~~~~~~~~~ hash.h:101:33: note: in expansion of macro ‘platform_SHA1_Clone_unsafe’ 101 | # define git_SHA1_Clone_unsafe platform_SHA1_Clone_unsafe | ^~~~~~~~~~~~~~~~~~~~~~~~~~ hash.h:133:20: note: in expansion of macro ‘git_SHA1_Clone_unsafe’ 133 | static inline void git_SHA1_Clone_unsafe(git_SHA_CTX_unsafe *dst, | ^~~~~~~~~~~~~~~~~~~~~ sha1/openssl.h:37:20: note: previous definition of ‘openssl_SHA1_Clone’ with type ‘void(struct openssl_SHA1_CTX *, const struct openssl_SHA1_CTX *)’ 37 | static inline void openssl_SHA1_Clone(struct openssl_SHA1_CTX *dst, | ^~~~~~~~~~~~~~~~~~ This only matters when compiling with openssl as the "safe" variant, since it's the only algorithm that requires a clone helper (and even then, only if you are using openssl 3.0+). And you should never do that, because it's not safe. But still, the invocation above used to work and should continue to do so until we decide to require a collision-detecting variant for the safe algorithm entirely. Signed-off-by: Jeff King <peff@peff.net> --- This is a regression in v2.47.0-rc0. As mentioned above, I kind of doubt anybody will hit it in practice (I only did because I was trying to do some timing tests between the fast and dc variants). And it is almost tempting to leave it as a wake-up call for anybody who is still not using a collision-detecting sha1. ;) hash.h | 3 +++ 1 file changed, 3 insertions(+)