Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

curl_sha512_256: add workaround for NetBSD bug #13133

Closed

Conversation

Karlson2k
Copy link
Contributor

@Karlson2k Karlson2k commented Mar 15, 2024

Based on @mkauf analysis and suggestion.

The details are here and here.

The macro EVP_MAX_MD_SIZE is not available (on my machine), so I used SHA512_256_DIGEST_SIZE * 2 as a size of the temporal array which should be enough.

@Karlson2k
Copy link
Contributor Author

@mkauf, please check this PR.

@Karlson2k Karlson2k force-pushed the sha_512_256_netbsd_wkarnd_01 branch from 6ecef83 to 97282d6 Compare March 15, 2024 12:22
@Karlson2k
Copy link
Contributor Author

PR has been updated with fixed code formatting.

@mkauf mkauf self-requested a review March 15, 2024 13:21
Copy link
Contributor

@mkauf mkauf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! The code compiles on NetBSD 9.3 and test 1615 works.

If the NetBSD team fixes the bug, how long should curl keep the workaround?

lib/curl_sha512_256.c Show resolved Hide resolved
lib/curl_sha512_256.c Show resolved Hide resolved
@mkauf mkauf self-requested a review March 15, 2024 13:36
@Karlson2k
Copy link
Contributor Author

Thanks! The code compiles on NetBSD 9.3 and test 1615 works.

If the NetBSD team fixes the bug, how long should curl keep the workaround?

If/When NetBSD fixed the bug, we can identify NetBSD version by __NetBSD_Version__ (defined in <sys/param.h>) and limit the workaround only for earlier versions. If it would be fixed in ported OpenSSL, then fixed version can be detected by OPENSSL_VERSION_NUMBER

@mkauf
Copy link
Contributor

mkauf commented Mar 15, 2024

@Karlson2k : Can you please change the commit message so that it does not mention me? e.g. just remove the "@".

Last time this happened I got a lot of notification mails from GitHub... many projects copy commits and when they push, mentioned users get notifications.

Based on Michael Kaufmann analysis and suggestion
@Karlson2k Karlson2k force-pushed the sha_512_256_netbsd_wkarnd_01 branch from 97282d6 to f1e1719 Compare March 15, 2024 15:03
@Karlson2k
Copy link
Contributor Author

@Karlson2k : Can you please change the commit message so that it does not mention me? e.g. just remove the "@".

Done.
No change in the code, only commit message updated.

@dfandrich
Copy link
Contributor

I'm not sure we need to work around this issue. It's a security issue in NetBSD and (presumably), it will be fixed fairly quickly. Even if we keep it, I wouldn't keep it around past the next curl release.

@Karlson2k
Copy link
Contributor Author

Failed tests 661 and 1541 with VS2010 seem to be unrelated.

@Karlson2k
Copy link
Contributor Author

Karlson2k commented Mar 16, 2024

https://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=58039

This bug was fixed in NetBSD, but so far hasn't been pushed as update, so existing NetBSD installations still have this bug.
I think it worth to merge this PR. It is not large, not intrusive, limited to NetBSD only and ensures that curl and libcurl will not trigger segmentation failures on NetBSD.

Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this works around a crash in existing netbsd I think it is worth merging.

@bagder bagder closed this in b600638 Mar 18, 2024
@Karlson2k Karlson2k deleted the sha_512_256_netbsd_wkarnd_01 branch March 20, 2024 14:01
@riastradh
Copy link

FYI, we fixed this in the NetBSD 10.0 release that went out today, but it still affects all NetBSD 9.x versions.

You may want to explicit_memset to zero the part of tmp_digest that you don't use -- some of the noteworthy security properties of SHA-512/256 rely on keeping the other half of the output secret.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants