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

SHA-512/256 real implementation #12897

Closed
wants to merge 4 commits into from

Conversation

Karlson2k
Copy link
Contributor

@Karlson2k Karlson2k commented Feb 8, 2024

This PR adds real support for SHA-512/256 hashing algorithm.
Currently curl implementation of Digest Auth fakes SHA-512/256 by using SHA-256 algorithm which is completely wrong.

The new implementation added as an optional and can be disabled if needed (the related tests are automatically disabled). The disabling machinery is here, but the support for disabling in configure and CMake is not implemented and can be added when/if needed.

@github-actions github-actions bot added the tests label Feb 8, 2024
@Karlson2k Karlson2k force-pushed the sha512_256_impl_02 branch 2 times, most recently from 9269faa to 92e01e1 Compare February 8, 2024 13:20
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.

Awesome! Just some nits from me...

lib/curl_sha512_256.c Outdated Show resolved Hide resolved
lib/curl_sha512_256.c Outdated Show resolved Hide resolved
lib/curl_sha512_256.c Outdated Show resolved Hide resolved
lib/curl_sha512_256.c Outdated Show resolved Hide resolved
lib/curl_sha512_256.c Outdated Show resolved Hide resolved
@bagder
Copy link
Member

bagder commented Feb 8, 2024

Is there a particular reason you did not use the TLS library's crypto functions for this? OpenSSL etc provide functions for this.

@Karlson2k
Copy link
Contributor Author

Karlson2k commented Feb 8, 2024

Is there a particular reason you did not use the TLS library's crypto functions for this? OpenSSL etc provide functions for this.

Frankly, I didn't check the latest (read "five years old") OpenSSL versions. Previously OpenSSL supported only SHA-512, which is quite different.
Other TLS backends typically still support only SHA-512 (like GnuTLS).
Anyway, TLS backends support can be easily added, while hash computation algorithm is required in libcurl always, when Digest Auth is not disabled, so I believe it is the most important part.

@Karlson2k
Copy link
Contributor Author

The only failing test is 1165, reported sync CURL_DISABLE not in sync.
Before fixing it, let me know what is preferred:

  • new configure and cmake parameter to disable SHA-512/256
  • remove option to disable SHA-512/256
  • add something to mute test error without actual interface for disabling SHA-512/256

@Karlson2k
Copy link
Contributor Author

Unrelated: regexps in test1165.pl are incorrect.
The are /(CURL_DISABLE_[A-Z_]+)/
So CURL_DISABLE_SHA is detected by this tool instead of CURL_DISABLE_SHA512_256.

@Karlson2k Karlson2k force-pushed the sha512_256_impl_02 branch 2 times, most recently from f710b8f to 1f2c640 Compare February 8, 2024 16:44
@Karlson2k
Copy link
Contributor Author

Unrelated: regexps in test1165.pl are incorrect. The are /(CURL_DISABLE_[A-Z_]+)/ So CURL_DISABLE_SHA is detected by this tool instead of CURL_DISABLE_SHA512_256.

PR #12903 published

@dfandrich
Copy link
Contributor

Are there any crypto libraries used by libcurl that still lack SHA2 functions? SHA-256 support was added to libcurl before OpenSSL supported it, but I don't see a need to carry a local version of this calculation today. There are optimized functions already available that curl could use instead.

@Karlson2k
Copy link
Contributor Author

Karlson2k commented Feb 8, 2024

Are there any crypto libraries used by libcurl that still lack SHA2 functions? SHA-256 support was added to libcurl before OpenSSL supported it, but I don't see a need to carry a local version of this calculation today. There are optimized functions already available that curl could use instead.

I think MD5 and SHA-256 are supported by most of TLS libs. However, I'm not sure that all libs are providing digest calculation API as digest is not the primary function of TLS lib. SHA-256 and MD5 are provided by some C libraries and the kernels.
Not really related to this PR, but I think it not hard to maintain existing implementation and it can be used even without any TLS lib.

@Karlson2k Karlson2k force-pushed the sha512_256_impl_02 branch 3 times, most recently from a935115 to c93e63d Compare February 8, 2024 19:49
@Karlson2k
Copy link
Contributor Author

Karlson2k commented Feb 8, 2024

@bagder Everything was corrected except one comment.
Let me know what to do with it.

Also CURL_DISABLE_xxx macro check was muted in unpolite way. Please give me some hints what to do.

@Karlson2k
Copy link
Contributor Author

Karlson2k commented Feb 8, 2024

VS2010 and VS2008 failures are related.
I don't have any of them to reproduce.
Looks like MHDX_UINT64 gets defined to empty. Quite strange.

lib/curl_sha512_256.c Outdated Show resolved Hide resolved
lib/curl_sha512_256.c Outdated Show resolved Hide resolved
lib/curl_sha512_256.c Outdated Show resolved Hide resolved
@Karlson2k Karlson2k force-pushed the sha512_256_impl_02 branch 3 times, most recently from 86f7fb4 to 5e0d192 Compare February 9, 2024 15:00
@Karlson2k
Copy link
Contributor Author

Karlson2k commented Feb 9, 2024

@bagder I found that inline is not correctly supported by curl build system.
While configure has correct test and substitution for inline keyword (see

AC_C_INLINE
), CMake and others does not support it properly. This leads to build failures on MSVC 2008 and 2010.
I would suggest to add CMake code like

check_c_source_compiles("
  inline int foo_func(void){return 0;}
  int main(void)
  {
    return foo_func();
  }" HAVE_KEYWORD_INLINE)
if(not HAVE_KEYWORD_INLINE)
  check_c_source_compiles("
    __inline int foo_func(void){return 0;}
    int main(void)
    {
      return foo_func();
    }" HAVE_KEYWORD___INLINE)
endif()

The if both are not supported, add -Dinline=, if only the second form is supported then -Dinline=__inline.

Also, for VS projects to work with old compilers, add somewhere in the code (to some header that included globally):

#if !defined(inline) && defined(_MSC_VER)
#  if _MSC_VER < 1900
#    define inline __inline
#  endif
#endif

Currently curl code has inline keyword only for code that never compiled by MSVC.

@Karlson2k Karlson2k force-pushed the sha512_256_impl_02 branch 2 times, most recently from 1dbb750 to 4032cdb Compare February 9, 2024 23:05
@Karlson2k
Copy link
Contributor Author

Rebased and updated as my another PR #12903 has been merged.

@Karlson2k Karlson2k force-pushed the sha512_256_impl_02 branch 2 times, most recently from f1c6626 to 0fec7b2 Compare February 10, 2024 19:19
@Karlson2k
Copy link
Contributor Author

Tests failures seem to be unrelated.
@bagder Updated as you requested.

@vszakats
Copy link
Member

vszakats commented Feb 13, 2024

Regarding inline, unless there are strong reasons to spread this to all build-systems with detection and the rest (= complexity and build perf regression), wouldn't this code (or similar) make it work well-enough for most envs to start:

#ifdef __GNUC__
#undef inline
#define inline __inline__
#elif defined(_MSC_VER)
#undef inline
#define inline __inline
#endif

Taken from the libssh2 source code.

@Karlson2k
Copy link
Contributor Author

Regarding inline, unless there are strong reasons to spread this to all build-systems with detection and the rest (= complexity and build perf regression), wouldn't this code (or similar) make it work well-enough for most envs to start:

#ifdef __GNUC__
#undef inline
#define inline __inline__
#elif defined(_MSC_VER)
#undef inline
#define inline __inline
#endif

Taken from the libssh2 source code.

No need to undef inline since it was detected correctly by configure. It is like breaking thing that was fixed.
Force inline should work better at this function is used too many times.

@vszakats

This comment was marked as outdated.

@vszakats
Copy link
Member

vszakats commented Feb 14, 2024

This could work for both VS and CMake builds (maybe for autotools even):

--- a/lib/curl_setup.h
+++ b/lib/curl_setup.h
@@ -117,6 +117,17 @@
 
 #endif /* HAVE_CONFIG_H */
 
+#ifndef inline
+#  if defined(__GNUC__)
+#    define inline __inline__
+#  elif defined(_MSC_VER) && _MSC_VER < 1900
+#    define inline __inline
+#  elif !defined(__cplusplus) && \
+     !(defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L)
+#    define inline
+#  endif
+#endif
+
 /* ================================================================ */
 /* Definition of preprocessor macros/symbols which modify compiler  */
 /* behavior or generated code characteristics must be done here,   */

@Karlson2k
Copy link
Contributor Author

This could work for both VS and CMake builds (maybe for autotools even):

Yep, for simple inline it would work OK.
I'd add CMake detection, like it's done in autotools.

However, this fix is a general fix for libcurl/curl.

For this particular code force inline is better. It can be also made a part of global header, if needed.
The current version works, tested on all supported compilers and platforms, reliable and future-proof (more future-proof than use of __inline__ which is supported for backward-compatibility).

@vszakats
Copy link
Member

vszakats commented Feb 14, 2024

I'm against adding this to cmake if there is a reasonably good in-source solution for it, because adding each of these detections cripple build performance and spread redundant complexity to the build-tool level. Many targets need an in-source solution anyway.

If performance of this code is so critical to require __forceinline (assuming we know better than the compiler), it may be worth using an existing optimized implemention in the TLS backends. Otherwise letting the compiler do its best based on inline may be enough IMO.

@Karlson2k
Copy link
Contributor Author

Karlson2k commented Feb 18, 2024

It would be nice to use TLS-backend implementation of SHA-512/256, however. as I wrote earlier here and here there are just a few TLS-backends supporting SHA-512/256.

I'd emphasize: SHA-512/256 is rarely implemented by various libraries. SHA-512 is widely supported, but it is quite different from SHA-512/256.

This PR does not introduce any global changes, except curl_uint64_t type as it was requested by @bagder.
This PR does not require CMake changes. The suggested CMake changes are required for correct support of inline which is broken in libcurl globally (I think mainly because of old MS VC compilers) and not used by code in this PR.

Support of "force inline" is implemented carefully, in a safe way. It is used only on specific (whitelisted) compilers and will not break other compilers by falling back to simple static function.

@Karlson2k
Copy link
Contributor Author

Updated with even more careful and correct detection of supported inline keywords.
No changes to CMake, everything in the source code.

@Karlson2k
Copy link
Contributor Author

Karlson2k commented Feb 18, 2024

Note: this code may break some preprocessors

#if defined(SOME_MACRO) && SOME_MACRO >= 3
#define SOMETHING something
#endif

The correct way is

#ifdef SOME_MACRO
#if SOME_MACRO >= 3
#define SOMETHING something
#endif
#endif

Making comparison against C-like constants in preprocessor directives is not always processed correctly.
Incorrect:

#if SOME_VALUE >= 5000000L
#define SOMETHING something
#endif

Correct:

#if SOME_VALUE >= 5000000
#define SOMETHING something
#endif

@bagder
Copy link
Member

bagder commented Feb 18, 2024

Note: this code may break some preprocessors

I would argue that is a broken preprocessor. We use this construct in several places in the code already and so far no one has reported a problem with this.

@Karlson2k
Copy link
Contributor Author

Note: this code may break some preprocessors

I would argue that is a broken preprocessor. We use this construct in several places in the code already and so far no one has reported a problem with this.

Or can be just undetected...

No problem, let me know if you prefer a shorted merged preprocessor constructs and I'll adapt the code.

Also fix the tests.
New implementation tested with GNU libmicrohttpd. The new numbers in
tests are real SHA-512/256 numbers (not just some random ;) numbers ).
@Karlson2k
Copy link
Contributor Author

OK, macros are merged and simplified.

@bagder bagder closed this in cbe41d1 Feb 20, 2024
@bagder
Copy link
Member

bagder commented Feb 20, 2024

Thanks!

@Karlson2k Karlson2k deleted the sha512_256_impl_02 branch February 20, 2024 15:49
bagder pushed a commit that referenced this pull request Mar 7, 2024
This is a follow-up for PR #12897.

Add support for SHA-512/256 digest calculation by TLS backends.
Currently only OpenSSL and GnuTLS (actually, nettle) support
SHA-512/256.

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

Successfully merging this pull request may close these issues.

None yet

4 participants