-
Notifications
You must be signed in to change notification settings - Fork 495
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
Pull new HQC implementation from upstream #1585
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for getting into the kitchen (particularly its sink :) and moving this upgrade through CI @SWilson4 ! I added single comments, one of which I feel strongly about (as usual :) The version of HQC does not seem to have been updated. Am I right in this understanding? If so, can we please fix this before merge, either by patch or upstream update?
Oh, one more thing: This PR needs to update the top-level |
Both should be fixed now. I also incremented the |
Pardon the double force-push; I did the rebase before #1595 was merged, so it needed to be done again to avoid a merge conflict with main. |
@SWilson4 FYI and FWIW, I was intrigued by the single CI failure and ran some experiments on the CCI MacOS instance via SSH: Result: The shared lib config option |
I think I know what's happening, but I don't know why it would be caused by this particular config... if you look at the failed results, the computed KAT hashes are different at each run. This leads me to believe that either
If, for instance, a deterministic PRNG were used with an uninitialized buffer it might explain the apparent randomness. However, the former possibility seems more likely to me. From the point of view of the non-HQC KATs, the only thing that's changed (other than being wrapped in an |
I just stepped through the code in a debugger on the M1 machine and confirmed that this is indeed the case. It's very confusing: the NIST DRBG is indeed set as the |
If the code is built without OpenSSL the same issue appears. What RNG is used then? Why is this dependent on the linkage mode of the library?? |
tests/CMakeLists.txt
Outdated
add_executable(kat_kem kat_kem.c) | ||
target_link_libraries(kat_kem PRIVATE ${API_TEST_DEPS}) | ||
# KAT KEM needs to call the internal SHA3 functions directly, hence the extra dependencies | ||
add_executable(kat_kem kat_kem.c ${COMMON_OBJS}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm -- this line makes me wonder: If one adds all COMMON_OBJS and links against liboqs
(line below, API_TEST_DEPS) which symbols "win"? The ones from the objects list or the ones from the library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm -- this line makes me wonder: If one adds all COMMON_OBJS and links against
liboqs
(line below, API_TEST_DEPS) which symbols "win"? The ones from the objects list or the ones from the library?
Maybe the "winner" is picked differently on Mac vs Linux...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about this theory: oqs_randombytes_algorithm
is a static function pointer in an object file; Linux keeps using that throughout the code after it has been initialized by the call in kat_kem.c
. MacOS in turn "overloads" it with a new one when loading the library for all further invocations of library functions. I don't quite understand why the lib doesn't get loaded right at the moment OQS_randombytes_switch_algorithm
gets called, though.... So, just a wild theory, still no idea how to fix.
In that case the system RNG ( |
* fix kat_kem linkage * remove armhf CI support * Revert "remove armhf CI support" This reverts commit af759bb.
Thanks very much to @baentsch for getting CI to pass. I'm going to go ahead and merge into main, since there were no HQC-related changes after the approving reviews, and the changes since then are largely authored by one of the approving reviewers. As a final check, I ran the constant time tests locally just now and HQC passed with flying colours. Hopefully this will also be confirmed in the weekly run. |
Sync HQC with liboqs: open-quantum-safe/liboqs#1585
* Update Sphincs+ PQClean patch * Don't apply PQClean Dilithium and Kyber patches * Run copy_from_upstream; don't apply Dilithium and Kyber changes * Run HQC KATs with custom PRNG * Satisfy astyle * Add licence for common code * Fix CI build errors * Update HQC version, OQS version, and SOVERSION * Move HQC PRNG into test file * Satisfy astyle * Fix SHA3 link error * Reset HQC issues/passes * fixup! Fix SHA3 link error * fix kat_kem linkage to make HQC PR pass CI (#1601) * fix kat_kem linkage * remove armhf CI support * Revert "remove armhf CI support" This reverts commit af759bb. --------- Co-authored-by: Michael Baentsch <[email protected]>
* Update Sphincs+ PQClean patch * Don't apply PQClean Dilithium and Kyber patches * Run copy_from_upstream; don't apply Dilithium and Kyber changes * Run HQC KATs with custom PRNG * Satisfy astyle * Add licence for common code * Fix CI build errors * Update HQC version, OQS version, and SOVERSION * Move HQC PRNG into test file * Satisfy astyle * Fix SHA3 link error * Reset HQC issues/passes * fixup! Fix SHA3 link error * fix kat_kem linkage to make HQC PR pass CI (#1601) * fix kat_kem linkage * remove armhf CI support * Revert "remove armhf CI support" This reverts commit af759bb. --------- Co-authored-by: Michael Baentsch <[email protected]>
* Update Sphincs+ PQClean patch * Don't apply PQClean Dilithium and Kyber patches * Run copy_from_upstream; don't apply Dilithium and Kyber changes * Run HQC KATs with custom PRNG * Satisfy astyle * Add licence for common code * Fix CI build errors * Update HQC version, OQS version, and SOVERSION * Move HQC PRNG into test file * Satisfy astyle * Fix SHA3 link error * Reset HQC issues/passes * fixup! Fix SHA3 link error * fix kat_kem linkage to make HQC PR pass CI (#1601) * fix kat_kem linkage * remove armhf CI support * Revert "remove armhf CI support" This reverts commit af759bb. --------- Co-authored-by: Michael Baentsch <[email protected]>
* Update Sphincs+ PQClean patch * Don't apply PQClean Dilithium and Kyber patches * Run copy_from_upstream; don't apply Dilithium and Kyber changes * Run HQC KATs with custom PRNG * Satisfy astyle * Add licence for common code * Fix CI build errors * Update HQC version, OQS version, and SOVERSION * Move HQC PRNG into test file * Satisfy astyle * Fix SHA3 link error * Reset HQC issues/passes * fixup! Fix SHA3 link error * fix kat_kem linkage to make HQC PR pass CI (#1601) * fix kat_kem linkage * remove armhf CI support * Revert "remove armhf CI support" This reverts commit af759bb. --------- Co-authored-by: Michael Baentsch <[email protected]>
Run copy_from_upstream to pull the recently merged HQC updates from PQClean. Ignore Kyber and Dilithium updates as we don't yet want to move to the "standard" version.
This is unfortunately a bit of a "kitchen sink" PR: it contains
Notably, this PR does not contain the latest Kyber and Dilithium updates from PQClean, as this would entail updating liboqs to be in sync with the "standard" track. If/when this is merged, we will be out of sync with PQClean.
When patching HQC, I made a number of minor changes to the upstream code, mostly to address undefined or implementation-defined behaviour and endianness issues. I also wrote a new modular reduction routine to address PQClean/PQClean#482; this can be found in
vector.c
. It might be easier to review my changes directly against the reference implementation; you can view the patches here.Fixes #1319.
Fixes #995.