SHA-512/256 real implementation

[Karlson2k]

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.

[bagder]

Awesome! Just some nits from me...

[bagder]

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

[Karlson2k]

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]

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]

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]

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]

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]

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]

@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]

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.

[Karlson2k]

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

curl/configure.ac

Line 500 in 07e5b3e

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]

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

[Karlson2k]

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 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]

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]

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]

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]

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

[Karlson2k]

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]

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]

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.

[Karlson2k]

OK, macros are merged and simplified.

[bagder]

Thanks!