sha2: fix incorrect sha256 hash issue

[cyphar]

As mentioned in #4, with newer GCC versions, our sha256 digest methods would
return incorrect hashes. This is due to undefined behaviour in sha2.c when
compiling with -fstrict-aliasing (which is implied by -O2, the default
compilation mode under autoconf and by most distributions).

The solution is to force autoconf to set -fno-strict-aliasing. In addition,
add a Github Actions workflow to run make check to make sure we can catch
this is if it ever happens in the future.

Fixes #4
Signed-off-by: Aleksa Sarai [email protected]

[cyphar]

You can verify that the new Github Action would've caught the issue by taking a look at this run in my test repo, where the -fno-strict-aliasing fix wasn't applied.

[cyphar]

An alternative patch is to fix the aliasing issue (the actual problem is only in SHA256_Final and SHA512_Last -- it's because of the fact that context->buffer is being used in a textbook example of what -fstrict-aliasing considers undefined behaviour):

diff --git a/sha2.c b/sha2.c
index bdcbd317bdd4..4c9436f98275 100644
--- a/sha2.c
+++ b/sha2.c
@@ -567,7 +567,7 @@ void SHA256_Final(sha2_byte digest[SHA256_DIGEST_LENGTH], SHA256_CTX* context) {
                        *context->buffer = 0x80;
                }
                /* Set the bit count: */
-               *(sha2_word64*)(void *)&context->buffer[SHA256_SHORT_BLOCK_LENGTH] = context->bitcount;
+               memcpy(&context->buffer[SHA256_SHORT_BLOCK_LENGTH], &context->bitcount, sizeof(context->bitcount));

                /* Final transform: */
                SHA256_Transform(context, (sha2_word32*)(void *)context->buffer);
@@ -870,8 +870,8 @@ static void SHA512_Last(SHA512_CTX* context) {
                *context->buffer = 0x80;
        }
        /* Store the length of input data (in bits): */
-       *(sha2_word64*)(void *)&context->buffer[SHA512_SHORT_BLOCK_LENGTH] = context->bitcount[1];
-       *(sha2_word64*)(void *)&context->buffer[SHA512_SHORT_BLOCK_LENGTH+8] = context->bitcount[0];
+       memcpy(&context->buffer[SHA512_SHORT_BLOCK_LENGTH], &context->bitcount[1], sizeof(*context->bitcount));
+       memcpy(&context->buffer[SHA512_SHORT_BLOCK_LENGTH+8], &context->bitcount[0], sizeof(*context->bitcount));

        /* Final transform: */
        SHA512_Transform(context, (sha2_word64*)(void *)context->buffer);

Let me know which you prefer.

[archiecobbs]

Hah, sorry - just saw you already figured out the same patch as me in 864c1cf. Thanks.

[cyphar]

This also adds CI in the form of Github Actions to check the tests won't fail -- is that something you'd be interested in?

[archiecobbs]

Oh, didn't see that - sorry.

Yes looks good, added in d2f836b.

Thanks!