FrodoKEM: Initial implementation#10870
Conversation
2028905 to
49806f2
Compare
|
Code generated by PR: |
7b7dd77 to
d31741e
Compare
|
Jenkins: retest this please Aborts |
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10870
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 8
8 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10870
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 5
5 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10870
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 7
7 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #10870
Scan targets checked: wolfcrypt-bugs, wolfcrypt-port-bugs, wolfcrypt-rs-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src
Findings: 6
4 finding(s) posted as inline comments (see file-level comments below)
High (2)
wc_FrodoKemKey_Free called twice on same key object
Function: test_wc_frodokem_encapsulate_kats
Category: double-free
The function calls wc_FrodoKemKey_Free on the same key object both at the end of each loop iteration (line 834) and unconditionally in the post-loop cleanup block (line 838). When the loop completes all iterations without error, the final iteration's in-loop Free zeroes the secret fields and invokes wc_Shake128_Free / wc_AesFree on the embedded hash and cipher objects. The unconditional post-loop Free then calls wc_Shake128_Free and wc_AesFree a second time on those same objects. wc_FrodoKemKey_Free (wc_frodokem.c:411-434) does not zero or NULL out the embedded wc_Shake or wc_Aes structs after freeing them, so the second call operates on stale internal state. Under WOLFSSL_SMALL_STACK the SHAKE and AES state buffers are heap-allocated; if wc_Shake128_Free / wc_AesFree do not NULL the internal pointer on first release, the second release is a heap double-free causing undefined behaviour.
Recommendation: Remove the in-loop wc_FrodoKemKey_Free call and let the post-loop block serve as the single cleanup point for both the success and early-exit paths. Alternatively, guard the post-loop Free with a check on key->params (non-NULL when the key is initialized, NULL after Free zeroes flags) so that the second call is a no-op when the key was already freed.
wc_FrodoKemKey_Free called twice on key/pubKey/privKey objects
Function: test_wc_frodokem_encode_decode
Category: double-free
The function calls wc_FrodoKemKey_Free on three key objects (privKey at line 1014, pubKey at line 1015, key at line 1016) both at the end of each loop iteration and unconditionally in the post-loop cleanup block (lines 1025, 1027, 1029). When the loop completes all iterations without error, the final iteration's in-loop Free calls zeroes the secret fields and invoke wc_Shake128_Free / wc_AesFree on the embedded hash and cipher objects. The unconditional post-loop Free calls then call wc_Shake128_Free and wc_AesFree a second time on those same objects. wc_FrodoKemKey_Free (wc_frodokem.c:411-434) does not zero or NULL out the embedded wc_Shake or wc_Aes structs after freeing them, so the second call operates on stale internal state. Under WOLFSSL_SMALL_STACK the SHAKE and AES state buffers are heap-allocated; if wc_Shake128_Free / wc_AesFree do not NULL the internal pointer on first release, the second release is a heap double-free causing undefined behaviour.
Recommendation: Remove the in-loop wc_FrodoKemKey_Free calls and let the post-loop block serve as the single cleanup point for both the success and early-exit paths. Alternatively, guard each post-loop Free with a check on the corresponding key's params field (non-NULL when the key is initialized, NULL after Free zeroes flags) so that the second calls are no-ops when the keys were already freed.
This review was generated automatically by Fenrir. Findings are non-blocking.
835b17c to
adb6f14
Compare
|
Jenkins: retest this please |
Frauschi
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: COMMENT
Findings: 2 total — 2 posted, 0 skipped
Posted findings
- [Info] Doxygen @return contract disagrees with implementation: BAD_FUNC_ARG vs BUFFER_E —
doc/dox_comments/header_files/wc_frodokem.h:170 - [Info] frodokem_kats[] lacks the empty-array sentinel that frodokem_types[] has —
tests/api/test_frodokem.c:738
Review generated by Skoll via Claude/Codex
|
Jenkins: retest this please Test failed due to port issue |
Supports Fast C code, small C code, small stack and assembly for x86_64, Aarch64, Aarch32, Thumb2.
Added support for encoding and decoding keys in ASN.1. Added support for X.509 certificates and CSRs. Generated certificates and CSRs. Not fo FrodoKEM-640 as is not in the specs.
JacobBarthelmeh
left a comment
There was a problem hiding this comment.
Noting that frodokem is currently behind the --enable-experimental flag. It appears to be finalized in ISO June of this year. Not sure we want it still behind the experimental flag? But if changed, that could be in a follow up PR.
The regenerate of ARM64 Windows AES asm to sync AES-GCM-SIV wolfcrypt/src/port/arm/armv8-aes-asm.asm added 5k + lines to this PR unrelated to frodokem implementation. Would be nice to have PR's be more targeted on large feature additions.
C implementation on 2.4 GHz i9
FRODOKEM 640 SHAKE 128 key gen 500 ops took 1.065 sec, avg 2.130 ms, 469.581 ops/sec, 2555478000 cycles 5110956.0 Cycles/op
FRODOKEM 640 SHAKE 128 encap 500 ops took 1.124 sec, avg 2.247 ms, 444.982 ops/sec, 2696747554 cycles 5393495.1 Cycles/op
FRODOKEM 640 SHAKE 128 decap 500 ops took 1.128 sec, avg 2.257 ms, 443.159 ops/sec, 2707838242 cycles 5415676.5 Cycles/op
FRODOKEM 976 SHAKE 192 key gen 300 ops took 1.417 sec, avg 4.722 ms, 211.775 ops/sec, 3399846254 cycles 11332820.8 Cycles/op
FRODOKEM 976 SHAKE 192 encap 300 ops took 1.461 sec, avg 4.870 ms, 205.322 ops/sec, 3506702046 cycles 11689006.8 Cycles/op
FRODOKEM 976 SHAKE 192 decap 300 ops took 1.438 sec, avg 4.794 ms, 208.597 ops/sec, 3451648718 cycles 11505495.7 Cycles/op
FRODOKEM 1344 SHAKE 256 key gen 200 ops took 1.713 sec, avg 8.566 ms, 116.746 ops/sec, 4111513816 cycles 20557569.1 Cycles/op
FRODOKEM 1344 SHAKE 256 encap 200 ops took 1.761 sec, avg 8.804 ms, 113.582 ops/sec, 4226050632 cycles 21130253.2 Cycles/op
FRODOKEM 1344 SHAKE 256 decap 200 ops took 1.745 sec, avg 8.726 ms, 114.605 ops/sec, 4188330614 cycles 20941653.1 Cycles/op
Assembly implementation on 2.4 GHz i9
FRODOKEM 640 SHAKE 128 key gen 1500 ops took 1.053 sec, avg 0.702 ms, 1425.062 ops/sec, 2526212652 cycles 1684141.8 Cycles/op
FRODOKEM 640 SHAKE 128 encap 1400 ops took 1.058 sec, avg 0.756 ms, 1322.783 ops/sec, 2540106044 cycles 1814361.5 Cycles/op
FRODOKEM 640 SHAKE 128 decap 1300 ops took 1.064 sec, avg 0.818 ms, 1222.053 ops/sec, 2553089574 cycles 1963915.1 Cycles/op
FRODOKEM 976 SHAKE 192 key gen 700 ops took 1.082 sec, avg 1.546 ms, 646.659 ops/sec, 2597974746 cycles 3711392.5 Cycles/op
FRODOKEM 976 SHAKE 192 encap 700 ops took 1.129 sec, avg 1.613 ms, 619.845 ops/sec, 2710361664 cycles 3871945.2 Cycles/op
FRODOKEM 976 SHAKE 192 decap 600 ops took 1.134 sec, avg 1.890 ms, 529.111 ops/sec, 2721552936 cycles 4535921.6 Cycles/op
FRODOKEM 1344 SHAKE 256 key gen 400 ops took 1.108 sec, avg 2.770 ms, 360.956 ops/sec, 2659608866 cycles 6649022.2 Cycles/op
FRODOKEM 1344 SHAKE 256 encap 400 ops took 1.142 sec, avg 2.856 ms, 350.177 ops/sec, 2741480264 cycles 6853700.7 Cycles/op
FRODOKEM 1344 SHAKE 256 decap 400 ops took 1.344 sec, avg 3.361 ms, 297.531 ops/sec, 3226565092 cycles 8066412.7 Cycles/op
The two runnable ARM assembly variants (AArch64 NEON and AArch32 NEON) pass
FrodoKEM's full keygen/encaps/decaps self-tests under emulation with QEMU.
Description
Supports Fast C code, small C code, small stack and assembly for x86_64, Aarch64, Aarch32, Thumb2.
Testing
Platform testing.
CI loop testing added.