pkgtools/libnbcompat: incorrect sha256 hashes with -fstrict-aliasing

[cyphar]

This issue was found in the Linux port of libnbcompat (see archiecobbs/libnbcompat#4 and the fix in archiecobbs/libnbcompat#7) but since the code in question is basically the same between the two codebases, I though it'd be prudent to open a bug report here as well.

The core issue is that the sha2.c code does some aliasing that GCC considers undefined behaviour when compiled with -fstrict-aliasing (this option is implied by -O2 which is enabled for most builds on most Linux distributions). However, it is only with GCC >= 11 that the code actually starts producing incorrect output. archiecobbs/libnbcompat#4 shows the difference in output for the standard NIST test vectors as well as my testing methodology, and archiecobbs/libnbcompat#3 is a PR against the port to add test programs to verify that the sha1, sha2 and ripemd160 code produces correct hashes for a set of test vectors. archiecobbs/libnbcompat#7 includes the fix for the port but this will need to be adjusted to match the NetBSD build system.

I'm not sure if any of the code I've written for the port would be welcome in NetBSD, I'm just leaving this comment to let you know that it's possible this might be an issue for NetBSD as well.

[cyphar]

It's possible the mtree in src has the same issue because the sha2.c code in question is largely shared but -fno-strict-aliasing is used in some places around the codebase so I'm not sure if it ends up being set when compiling sha2.c. I'll boot up a NetBSD VM and take a look...

[cyphar]

Seems that NetBSD 10 will use GCC 10.5 and NetBSD 9.3 has GCC 7.5, so the issue is not present in current NetBSD (some quick testing in a NetBSD 9.3 VM shows that mtree produces the correct output), but -fno-strict-aliasing should still be set to make sure it isn't silently missed when a newer GCC version is added.

[bsiegert]

cc @jsonn

[cyphar]

archiecobbs/libnbcompat@864c1cf is a patch in the Linux port which fixes the strict aliasing issue.

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);

[cyphar]

I'm pretty sure this was closed in error.

[jperkin]

Stupid GitHub auto-close.

[dhgutteridge]

This looks akin to a fix already committed to our digest package:
b631703
In other words, quite reasonable for us to integrate.

[dhgutteridge]

@jperkin could you please close this? (I don't have admin rights to do so.) I see you applied this in def71d1.