Update SPHINCS+ #1420

dstebila · 2023-03-18T15:11:57Z

[Yes] Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
[Yes] Does this PR change the the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in oqs-provider, OQS-OpenSSL, OQS-BoringSSL, and OQS-OpenSSH will also need to be ready for review and merge by the time this is merged.)
TODO: Ensure all references to the removed variants (Haraka) are removed.

.CMake/alg_support.cmake

baentsch

Didn't we decide to drop the "robust" variants as per NIST's recommendation (#1245 (comment) )?

dstebila · 2023-03-19T18:10:23Z

Didn't we decide to drop the "robust" variants as per NIST's recommendation (#1245 (comment) )?

Yes, you're right. I've removed the robust variants as well as the Haraka variants (NIST's email also says they only plan to move forward with SHA2 and SHAKE variants).

baentsch · 2023-03-19T18:34:51Z

removed the robust variants as well as the Haraka variants

Compile runs down from 1900 to 1300 .... The environment says Thank you :)

But the compile errors still confuse me: I locally have missing symbols

/usr/bin/ld: thash_shake_robustx4.c:(.text.PQCLEAN_SPHINCSSHAKE128FROBUST_AVX2_thashx4+0x2a3): undefined reference to `KeccakP1600times4_PermuteAll_24rounds'
/usr/bin/ld: lib/liboqs.a(hash_shakex4.c.o): in function `PQCLEAN_SPHINCSSHAKE128FSIMPLE_AVX2_prf_addrx4':
hash_shakex4.c:(.text.PQCLEAN_SPHINCSSHAKE128FSIMPLE_AVX2_prf_addrx4+0x17e): undefined reference to `KeccakP1600times4_PermuteAll_24rounds'
/usr/bin/ld: lib/liboqs.a(thash_shake_simplex4.c.o): in function `PQCLEAN_SPHINCSSHAKE128FSIMPLE_AVX2_thashx4':
thash_shake_simplex4.c:(.text.PQCLEAN_SPHINCSSHAKE128FSIMPLE_AVX2_thashx4+0x237): undefined reference to `KeccakP1600times4_PermuteAll_24rounds'
/usr/bin/ld: lib/liboqs.a(hash_shakex4.c.o):hash_shakex4.c:(.text.PQCLEAN_SPHINCSSHAKE128SROBUST_AVX2_prf_addrx4+0x17e): more undefined references to `KeccakP1600times4_PermuteAll_24rounds' follow
collect2: error: ld returned 1 exit status

and the CI runs have even weirder problems. Maybe a fresh look tomorrow resolves these....

dstebila · 2023-03-19T18:52:44Z

removed the robust variants as well as the Haraka variants

Compile runs down from 1900 to 1300 .... The environment says Thank you :)

But the compile errors still confuse me: I locally have missing symbols

/usr/bin/ld: thash_shake_robustx4.c:(.text.PQCLEAN_SPHINCSSHAKE128FROBUST_AVX2_thashx4+0x2a3): undefined reference to `KeccakP1600times4_PermuteAll_24rounds'
/usr/bin/ld: lib/liboqs.a(hash_shakex4.c.o): in function `PQCLEAN_SPHINCSSHAKE128FSIMPLE_AVX2_prf_addrx4':
hash_shakex4.c:(.text.PQCLEAN_SPHINCSSHAKE128FSIMPLE_AVX2_prf_addrx4+0x17e): undefined reference to `KeccakP1600times4_PermuteAll_24rounds'
/usr/bin/ld: lib/liboqs.a(thash_shake_simplex4.c.o): in function `PQCLEAN_SPHINCSSHAKE128FSIMPLE_AVX2_thashx4':
thash_shake_simplex4.c:(.text.PQCLEAN_SPHINCSSHAKE128FSIMPLE_AVX2_thashx4+0x237): undefined reference to `KeccakP1600times4_PermuteAll_24rounds'
/usr/bin/ld: lib/liboqs.a(hash_shakex4.c.o):hash_shakex4.c:(.text.PQCLEAN_SPHINCSSHAKE128SROBUST_AVX2_prf_addrx4+0x17e): more undefined references to `KeccakP1600times4_PermuteAll_24rounds' follow
collect2: error: ld returned 1 exit status

and the CI runs have even weirder problems. Maybe a fresh look tomorrow resolves these....

I did a quick pull without looking much into the changes. I recall there was a change involving removal of a Keccak-related file from the PQClean directories, but I didn't look into it. I don't think I'll get a chance to do so over the next few days, unfortunately.

baentsch · 2023-03-20T07:41:51Z

I recall there was a change involving removal of a Keccak-related file from the PQClean directories, but I didn't look into it.

Yup -- before this PR there was a copy_from_upstream patch file for sphincs removing fips202x4.[c|h]. Seems to be gone for some reason (?). Manually doing these removals only unearthed missing includes like for immintrin.h, so some more work required...

I don't think I'll get a chance to do so over the next few days, unfortunately.

NP; so focusing just on functionality (sphincs+ update) for the IETF hackathon I opted to simply remove the AVX2 optimizations from this PR (see #1422). This solves most build problems, but for example https://github.com/open-quantum-safe/liboqs/actions/runs/4465669616/jobs/7842995392, confirms the same results on my local machine and the (optimized) M1, namely incorrect KATs for 4 algorithms:

FAILED tests/test_kat.py::test_sig[SPHINCS+-SHA256-192f-simple] - AssertionError: assert 'fd4e301339b2...2b414f962f44c' == '5302cb39e848...b4f38caa7b99d'
FAILED tests/test_kat.py::test_sig[SPHINCS+-SHA256-256s-simple] - AssertionError: assert '05d15a742539...fa529071b0748' == 'db9b817748aa...652e1c98cdbf7'
FAILED tests/test_kat.py::test_sig[SPHINCS+-SHA256-192s-simple] - AssertionError: assert '0fa07f3f7775...3924ba2713839' == '9ecda7889d92...0226ebb06daf2'
FAILED tests/test_kat.py::test_sig[SPHINCS+-SHA256-256f-simple] - AssertionError: assert 'bd88b4945316...ae1370ed13480' == '38206d5db048...e76d4ce9fca23'

--> It seems the KATs introduced with this PR are not all correct. Thus, I'm afraid we cannot use this Sphincs+ update in the IETF hackathon. Agreed, @dstebila @xvzcf ? Good ideas how to facilitate that regardless very welcome.

xvzcf · 2023-05-01T21:40:04Z

@baentsch @dstebila I think at least part of the problem lies with our OpenSSL SHA2 code. In the mb-remove-avx2-from-sphincs branch, compare the focal-std-openssl job triggered by my commit (the job transcript can be found here) and the job from the previous commit.

baentsch · 2023-05-02T09:40:04Z

@baentsch @dstebila I think at least part of the problem lies with our OpenSSL SHA2 code. In the mb-remove-avx2-from-sphincs branch, compare the focal-std-openssl job triggered by my commit (the job transcript can be found here) and the job from the previous commit.

Interesting, indeed: Just by setting OQS_OPENSSL_OFF the test passes. It looks like our self tests then are also insufficient/not equivalent to the use of the API in liboqs. Not good.

baentsch · 2023-05-09T17:26:15Z

I now started to look a bit into this issue and concluded that the use of the OQS_SHA2_sha512_inc API causes the problem, right? When trying to understand the API, I wonder why these comments are as they are (referencing SHA3 in SHA2 include comments)? Cut-copy-paste error? I didn't find similar ones in the code, but it might be worth while fixing this (@dstebila?) as part of this PR. Now keeping looking further...

xvzcf · 2023-05-09T17:30:08Z

I now started to look a bit into this issue and concluded that the use of the OQS_SHA2_sha512_inc API causes the problem, right?

I've been looking into it as well, and yes it's this API that seems to be changing the KAT values.

Martyrshot · 2023-05-09T19:28:17Z

I wonder why these comments are as they are (referencing SHA3 in SHA2 include comments)?

I believe that was a fat finger on my part. Oops! Sorry!

baentsch · 2023-05-10T12:54:30Z

I wonder why these comments are as they are (referencing SHA3 in SHA2 include comments)?

I believe that was a fat finger on my part. Oops! Sorry!

Good to know :) Will you do a PR fixing this?

Next question: Am I reading correctly that our code is double-freeing MD contexts:

liboqs/src/common/sha2/sha2_ossl.c

Lines 125 to 136 in d704da0

    
           void OQS_SHA2_sha512_inc_finalize(uint8_t *out, OQS_SHA2_sha512_ctx *state, const uint8_t *in, size_t inlen) { 
        
           	unsigned int md_len; 
        
           	if (inlen > 0) { 
        
           		EVP_DigestUpdate((EVP_MD_CTX *) state->ctx, in, inlen); 
        
           	} 
        
           	EVP_DigestFinal_ex((EVP_MD_CTX *) state->ctx, out, &md_len); 
        
           	EVP_MD_CTX_free((EVP_MD_CTX *) state->ctx); 
        
           } 
        
           void OQS_SHA2_sha512_inc_ctx_release(OQS_SHA2_sha512_ctx *state) { 
        
           	EVP_MD_CTX_destroy((EVP_MD_CTX *) state->ctx); 
        
           }

(when OQS_SHA2_sha512_inc_finalize and OQS_SHA2_sha512_inc_ctx_release are called in succession)? Is this intentional? (see https://www.openssl.org/docs/man3.1/man3/EVP_MD_CTX_free.html:

The EVP_MD_CTX_create() and EVP_MD_CTX_destroy() functions were renamed to EVP_MD_CTX_new() and EVP_MD_CTX_free() in OpenSSL 1.1.0, respectively.

baentsch · 2023-05-10T13:25:21Z

The more I read this code, the less happy I am: Many OpenSSL API calls are made without checking their return values. This is not good, considering for example

Ignoring failure returns of EVP_DigestInit_ex(), EVP_DigestInit_ex2(), or EVP_DigestInit() can lead to undefined behavior on subsequent calls updating or finalizing the EVP_MD_CTX such as the EVP_DigestUpdate() or EVP_DigestFinal() functions.

(from https://www.openssl.org/docs/man3.0/man3/EVP_DigestInit_ex.html).

Shall I do a PR for this or does anyone else volunteer?

dstebila · 2023-05-10T13:37:44Z

Next question: Am I reading correctly that our code is double-freeing MD contexts:

Our API expects that you call one or the other of _inc_finalize, or _inc_ctx_release, but not both. Whether that is obeyed is a different question, of course.

The EVP_MD_CTX_create() and EVP_MD_CTX_destroy() functions were renamed to EVP_MD_CTX_new() and EVP_MD_CTX_free() in OpenSSL 1.1.0, respectively.

It is confusing that we sometimes use _destroy and sometimes use _free. It would be be worth simplifying.

dstebila · 2023-05-10T13:39:03Z

Next question: Am I reading correctly that our code is double-freeing MD contexts:

Our API expects that you call one or the other of _inc_finalize, or _inc_ctx_release, but not both. Whether that is obeyed is a different question, of course.

I suppose we could put some null sets/checks (state->ctx = null on release, and if state->ctx == null before release) to mitigate any damage if they are called twice.

dstebila · 2023-05-10T13:39:55Z

The more I read this code, the less happy I am: Many OpenSSL API calls are made without checking their return values. This is not good, considering for example

Ignoring failure returns of EVP_DigestInit_ex(), EVP_DigestInit_ex2(), or EVP_DigestInit() can lead to undefined behavior on subsequent calls updating or finalizing the EVP_MD_CTX such as the EVP_DigestUpdate() or EVP_DigestFinal() functions.

(from https://www.openssl.org/docs/man3.0/man3/EVP_DigestInit_ex.html).

Shall I do a PR for this or does anyone else volunteer?

Since most of our hashing API has no return value (void), would you then be putting in something similar to OQS_EXIT_IF_NULL?

baentsch · 2023-05-10T13:42:48Z

And another logical question: Why is SHA2_BLOCK_SIZE defined as 64 in the OpenSSL wrapper (

liboqs/src/common/sha2/sha2_ossl.c

Line 45 in d704da0

#define SHA2_BLOCK_SIZE 64

and

liboqs/src/common/sha2/sha2_ossl.c

Lines 121 to 123 in d704da0

    
           void OQS_SHA2_sha512_inc_blocks(OQS_SHA2_sha512_ctx *state, const uint8_t *in, size_t inblocks) { 
        
           	EVP_DigestUpdate((EVP_MD_CTX *) state->ctx, in, inblocks * SHA2_BLOCK_SIZE); 
        
           }

) while 128 bytes seem to be the length of a block used in the C implementation:

liboqs/src/common/sha2/sha2_c.c

Lines 624 to 625 in d704da0

    
           crypto_hashblocks_sha512_c(state->ctx, in, 128 * inblocks); 
        
           bytes += 128 * inblocks;

Or am I misunderstanding the C code here (didn't dive into it)?

dstebila · 2023-05-10T13:47:06Z

And another logical question: Why is SHA2_BLOCK_SIZE defined as 64 in the OpenSSL wrapper (

liboqs/src/common/sha2/sha2_ossl.c

Line 45 in d704da0

#define SHA2_BLOCK_SIZE 64

and

liboqs/src/common/sha2/sha2_ossl.c

Lines 121 to 123 in d704da0

void OQS_SHA2_sha512_inc_blocks(OQS_SHA2_sha512_ctx *state, const uint8_t *in, size_t inblocks) {

EVP_DigestUpdate((EVP_MD_CTX *) state->ctx, in, inblocks * SHA2_BLOCK_SIZE);

}

) while 128 bytes seem to be the length of a block used in the C implementation:

liboqs/src/common/sha2/sha2_c.c

Lines 624 to 625 in d704da0

crypto_hashblocks_sha512_c(state->ctx, in, 128 * inblocks);

bytes += 128 * inblocks;

Or am I misunderstanding the C code here (didn't dive into it)?

Oh! That's it! The SHA-256 block size is 64 bytes whereas the SHA-512 block size 128 bytes; in the C code it's using 64 and 128 correctly for the two different functions, but in the ossl wrapper it's using 64 everywhere.

baentsch · 2023-05-10T13:47:47Z

The last question indeed resolves the problem with the KATs: This diff resolves the issue:

diff --git a/src/common/sha2/sha2_ossl.c b/src/common/sha2/sha2_ossl.c
index b8e71cf9..e5b69e0d 100644
-       EVP_DigestUpdate((EVP_MD_CTX *) state->ctx, in, inblocks * SHA2_BLOCK_SIZE);
+       EVP_DigestUpdate((EVP_MD_CTX *) state->ctx, in, inblocks * 2 * SHA2_BLOCK_SIZE);

dstebila · 2023-05-10T13:48:58Z

The last question indeed resolves the problem with the KATs: This diff resolves the issue:

diff --git a/src/common/sha2/sha2_ossl.c b/src/common/sha2/sha2_ossl.c
index b8e71cf9..e5b69e0d 100644
-       EVP_DigestUpdate((EVP_MD_CTX *) state->ctx, in, inblocks * SHA2_BLOCK_SIZE);
+       EVP_DigestUpdate((EVP_MD_CTX *) state->ctx, in, inblocks * 2 * SHA2_BLOCK_SIZE);

Hold on, isn't the SHA384 block size also 128 bytes?

dstebila · 2023-05-10T13:51:56Z

Before we get too excited, about declaring this resolved, I think we should also strengthen the OQS test programs to detect this flaw.

baentsch · 2023-05-10T13:57:32Z

we should also strengthen the OQS test programs to detect this flaw.

Most definitely. And strengthen the way we use the OpenSSL APIs. Let's talk about this... now :)

* correct ARM SHA3 extension addition * correct compile option for ARM SHA * following https://developer.arm.com/documentation/101754/0618/armclang-Reference/armclang-Command-line-Options/-march * correct SHA3 enablement

This reverts commit f57ed6d.

* AVX2 test * AVX2 test * add patch; correct aarch64; correct documentation * enable Keccak for Sphincs even if OpenSSL shall provide SHA3 * properly handle xkcp enablement if only specific algorithms are selected * correct conditional setting * re-enable XKCP for other platforms * Windows support * alternate pqcrystals-AES removal

xvzcf · 2023-05-15T18:13:19Z

If I can get a once over and 1-2 approvals on this it should be good to merge

baentsch · 2023-05-16T04:41:33Z

If I can get a once over and 1-2 approvals on this it should be good to merge

Done. But it doesn't feel quite right that I give an approval on this as all changes done to get this to "green" were done by me in #1460. But if that got a good review, I think we're good to move forward.

baentsch · 2023-05-16T15:47:15Z

Thanks for this merge, @xvzcf . Are you going to look into the downstreams now? For oqs-openssl111 it should be as simple as copying over generate.yml from oqs-provider (if open-quantum-safe/oqs-provider#158 still passes OK after the #1420 merge).

xvzcf · 2023-05-16T15:48:28Z

Thanks for this merge, @xvzcf . Are you going to look into the downstreams now? For oqs-openssl111 it should be as simple as copying over generate.yml from oqs-provider (if open-quantum-safe/oqs-provider#158 still passes OK after the #1420 merge).

I'm working on it, I'm updating the names in liboqs first and then I'll update the downstreams

SPHINCS+ was updated in liboqs (open-quantum-safe/liboqs/pull/1420), and this makes old keys incompatible with the new implementation. New keys were generated using the oqs-provider for OpenSSL 3 openssl genpkey \ -provider default -provider oqsprovider \ -algorithm sphincsshake128fsimple \ -outform der \ -out bench_sphincs_fast_level1_key.der And certs_test.h was updated with these new keys using xxd xxd -i -c 10 -u bench_sphincs_fast_level1_key.der This was repeated for the 6 variants of SPHINCS+ that wolfSSL supports.

This also adds new keys for SPHINCS+. The reason is that SPHINCS+ was updated to 3.1 in liboqs (open-quantum-safe/liboqs/pull/1420), and old keys are incompatible with the new implementation. Keys were generated using the oqs-provider for OpenSSL 3 openssl genpkey \ -provider default -provider oqsprovider \ -algorithm sphincsshake128fsimple \ -outform der \ -out bench_sphincs_fast_level1_key.der And certs_test.h was updated using xxd xxd -i -c 10 -u bench_sphincs_fast_level1_key.der This was repeated for the 6 variants of SPHINCS+ that wolfSSL supports.

dstebila added this to the 0.8.0 milestone Mar 18, 2023

dstebila self-assigned this Mar 18, 2023

dstebila requested review from xvzcf, baentsch and bhess as code owners March 18, 2023 15:11

baentsch reviewed Mar 19, 2023

View reviewed changes

.CMake/alg_support.cmake Outdated Show resolved Hide resolved

baentsch requested changes Mar 19, 2023

View reviewed changes

bhess mentioned this pull request Mar 22, 2023

Review IDs open-quantum-safe/oqs-provider#126

Closed

This was referenced May 10, 2023

Bug when stopping provider on OSX open-quantum-safe/oqs-provider#168

Closed

Remove Kyber90s and Dilithium-AES variants #1417

Closed

xvzcf requested a review from jschanck as a code owner May 15, 2023 16:29

dstebila and others added 10 commits May 15, 2023 13:19

Switch to new PQClean commit for SPHINCS+ and remove old patch file

644b04d

Run copy_from_upstream

1832a7e

Updates to PQClean compatibility shim

fcb0090

Improves "compilability" on Apple M1 (ARM) (#1421)

6347f42

* correct ARM SHA3 extension addition * correct compile option for ARM SHA * following https://developer.arm.com/documentation/101754/0618/armclang-Reference/armclang-Command-line-Options/-march * correct SHA3 enablement

Remove SPHINCS+ robust and Haraka variants

f19172f

Apply copy_from_upstream

33427db

Delete obsolete files

26d8faa

Fix SHA2 block sizes in OpenSSL wrapper

0788346

Revert "Fix SHA2 block sizes in OpenSSL wrapper"

7432007

This reverts commit f57ed6d.

xvzcf force-pushed the ds-update-pqclean-20230317 branch from 2b36cdd to 1f267d5 Compare May 15, 2023 17:26

baentsch approved these changes May 16, 2023

View reviewed changes

xvzcf merged commit fd1b897 into main May 16, 2023

dstebila deleted the ds-update-pqclean-20230317 branch May 16, 2023 14:58

dstebila mentioned this pull request May 16, 2023

Remove remaining references to SPHINCS+-Haraka, Kyber 90s, Dilithium-AES #1465

Merged

baentsch mentioned this pull request May 16, 2023

Update McEliece (& Falcon) #1459

Closed

dstebila mentioned this pull request May 17, 2023

Extend test_hash to cover more input lengths #1468

Merged

This was referenced May 19, 2023

Sphincs implementation version dubious #1475

Closed

Algorithm & ID updates open-quantum-safe/openssl#447

Merged

iyanmv mentioned this pull request Jul 10, 2023

Rename sphincs algs to follow upstream wolfSSL/wolfssl#6576

Merged

4 tasks

This was referenced Oct 29, 2024

SPHINCS+ SHAKE aarch64 implementation is never enabled #1967

Closed

Remove SPHINCS+ aarch64 code #1972

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update SPHINCS+ #1420

Update SPHINCS+ #1420

dstebila commented Mar 18, 2023 •

edited

Loading

baentsch left a comment

dstebila commented Mar 19, 2023

baentsch commented Mar 19, 2023

dstebila commented Mar 19, 2023

baentsch commented Mar 20, 2023

xvzcf commented May 1, 2023 •

edited

Loading

baentsch commented May 2, 2023

baentsch commented May 9, 2023

xvzcf commented May 9, 2023

Martyrshot commented May 9, 2023

baentsch commented May 10, 2023

baentsch commented May 10, 2023

dstebila commented May 10, 2023

dstebila commented May 10, 2023

dstebila commented May 10, 2023

baentsch commented May 10, 2023

dstebila commented May 10, 2023

baentsch commented May 10, 2023

dstebila commented May 10, 2023

dstebila commented May 10, 2023

baentsch commented May 10, 2023

xvzcf commented May 15, 2023

baentsch commented May 16, 2023

baentsch commented May 16, 2023

xvzcf commented May 16, 2023

Update SPHINCS+ #1420

Update SPHINCS+ #1420

Conversation

dstebila commented Mar 18, 2023 • edited Loading

baentsch left a comment

Choose a reason for hiding this comment

dstebila commented Mar 19, 2023

baentsch commented Mar 19, 2023

dstebila commented Mar 19, 2023

baentsch commented Mar 20, 2023

xvzcf commented May 1, 2023 • edited Loading

baentsch commented May 2, 2023

baentsch commented May 9, 2023

xvzcf commented May 9, 2023

Martyrshot commented May 9, 2023

baentsch commented May 10, 2023

baentsch commented May 10, 2023

dstebila commented May 10, 2023

dstebila commented May 10, 2023

dstebila commented May 10, 2023

baentsch commented May 10, 2023

dstebila commented May 10, 2023

baentsch commented May 10, 2023

dstebila commented May 10, 2023

dstebila commented May 10, 2023

baentsch commented May 10, 2023

xvzcf commented May 15, 2023

baentsch commented May 16, 2023

baentsch commented May 16, 2023

xvzcf commented May 16, 2023

dstebila commented Mar 18, 2023 •

edited

Loading

xvzcf commented May 1, 2023 •

edited

Loading