Repository navigation
Conversation
|
There was a problem hiding this comment.
🟡 Changes recommended
Critical timing leaks remain in ECC exceptional branches and the modular-inversion fallback, while an unconditional table assertion also fails for the timing-resistant heap-math configuration.
3 open issues
What changed in this PR
This PR hardens secret-dependent arithmetic in wolfCrypt and the OpenSSL compatibility layer against timing side channels, continuing the work from #11159.
Changes:
- Adds blinded or Fermat-based inversion paths and timing-resistant ECC mapping and fixed-point multiplication.
- Reworks heap-math constant-time helpers and comparison handling.
- Adds SAKKE RNG requirements, an RNG-accepting API, and regression and fault-injection coverage.
| File | Description |
|---|---|
wolfssl/wolfcrypt/settings.h |
Separates exported comparison API suppression from internal availability. |
wolfssl/wolfcrypt/sakke.h |
Declares the RNG-accepting RSK API. |
wolfssl/wolfcrypt/memory.h |
Updates the comparison API guard. |
wolfcrypt/src/sp_int.c |
Enables modular multiplication for SAKKE. |
wolfcrypt/src/sakke.c |
Adds inversion blinding, Fermat conversion, and RNG handling. |
wolfcrypt/src/rsa.c |
Removes the alternate unblinded key-generation inverse. |
wolfcrypt/src/port/atmel/atmel.c |
Uses constant-time GCM tag comparison. |
wolfcrypt/src/misc.c |
Uses volatile comparison loads. |
wolfcrypt/src/memory.c |
Updates the exported comparison implementation guard. |
wolfcrypt/src/integer.c |
Reworks constant-time modular arithmetic helpers. |
wolfcrypt/src/eccsi.c |
Blinds signing inversion and clears temporary secrets. |
wolfcrypt/src/ecc.c |
Adds timing-resistant comb multiplication and mapping. |
wolfcrypt/src/dsa.c |
Always uses the blinded signing path. |
wolfcrypt/benchmark/benchmark.c |
Reuses the benchmark RNG for RSK generation. |
tests/unit-mcdc/test_sakke_whitebox.c |
Tests rejection of a noninvertible RSK input. |
tests/unit-mcdc/test_sakke_fault_whitebox.c |
Exercises blinding retries and exhaustion. |
tests/unit-mcdc/test_integer_whitebox.c |
Updates source-line references in comments. |
tests/unit-mcdc/test_integer_fault_whitebox.c |
Tests helper allocation failures and invalid arguments. |
tests/unit-mcdc/test_eccsi_fault_whitebox.c |
Tests signing blinding retries and limits. |
tests/unit-mcdc/test_ecc_whitebox.c |
Compares cached comb results with the ladder. |
tests/unit-mcdc/test_ecc_fault_whitebox.c |
Adapts calls to the new multiplier signature. |
tests/api/test_wolfmath.h |
Registers heap-math regression tests. |
tests/api/test_wolfmath.c |
Tests arithmetic results, aliasing, and input validation. |
tests/api/test_sakke.c |
Tests the RNG-accepting RSK API. |
tests/api/test_ossl_bn.c |
Tests composite-modulus inversion and aliasing. |
tests/api/test_ecc.h |
Registers ECC regression tests. |
tests/api/test_ecc.c |
Adds boundary-scalar and cache-consistency tests. |
src/ssl_bn.c |
Adds best-effort inversion blinding and random-buffer clearing. |
src/pk_ec.c |
Documents the variable-time Shamir path. |
doc/dox_comments/header_files/sakke.h |
Adds the new SAKKE API to documentation. |
doc/dox_comments/header_files/doxygen_groups.h |
Updates RSK documentation references. |
doc/dox_comments/header_files-ja/sakke.h |
Adds the API to Japanese documentation. |
doc/dox_comments/header_files-ja/doxygen_groups.h |
Updates Japanese RSK documentation references. |
configure.ac |
Enforces SAKKE RNG requirements and bundle selection. |
CMakeLists.txt |
Rejects SAKKE without RNG support. |
.wolfssl_known_macro_extras |
Updates recognized comparison and inversion macros. |
.github/configs/os-check-linux.json |
Adds fixed-point ECC and SP-math SAKKE configurations. |
Review effort: Balanced
2f9289b to
51fdd2e
Compare
|
Jenkins retest this please. Size-config test RROR: Error cloning remote repo 'origin |
81f4296 to
9b70426
Compare
9b70426 to
68967cc
Compare
…he SAKKE random draw
68967cc to
692a832
Compare
|
@claude review |
There was a problem hiding this comment.
🔵 Needs a closer look
The new secret-scalar comb path still uses point addition with scalar-dependent exceptional branches and extra doubling, leaving a critical timing side channel despite fixed table scans and randomized projective coordinates.
No open issues 🎉
1 resolved since last review
Review effort: Balanced
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It's a large side-channel hardening change to core ECC, ECCSI, SAKKE and bignum code, and it changes some build and API behavior, so a human review is still worthwhile.
What was reviewed:
- The new constant-time FP_ECC comb (
fp_comb_ct/fp_lut_select) in wolfcrypt/src/ecc.c, and howwc_ecc_mulmod_ex/ex2choose between the constant-time and variable-time paths. - The blinded inverses: ECCSI signing, SAKKE RSK derivation (with the new
wc_MakeSakkeRsk_exandsakke_rand_nonzero_mod_q), andwolfssl_bn_invmod_blindin src/ssl_bn.c, including its unblinded fallback and its handling of factors that share a divisor with m. - The mask-based
mp_addmod_ct/mp_submod_ctrewrite in integer.c, the removal ofWOLFSSL_MP_INVMOD_CONSTANT_TIME, and the newWC_NO_CONST_CMP_APIand SAKKE-requires-RNG checks across configure.ac, CMakeLists.txt and settings.h.
Extended reasoning...
The PR is about 1750 added lines across 37 files. It hardens how secret scalars and inverses are handled in wolfCrypt ECC, ECCSI and SAKKE, in heap-math integer.c and in the OpenSSL-compat BN layer. All of that is cryptographic, timing-sensitive code, where a regression in constant-time behavior or blinding would be high severity. It also changes behavior: the FP_LUT default drops from 8 to 6, SAKKE now fails to build without an RNG, WOLFSSL_MP_INVMOD_CONSTANT_TIME is removed, and minimal builds define WC_NO_CONST_CMP_API instead of WOLFSSL_NO_CONST_CMP. Because of its size and the crypto surface it touches, it isn't a candidate for automated approval, even though the bug hunt found nothing.


Description
Hardens secret-handling in ECC, ECCSI, SAKKE and the OpenSSL compatibility BN layer against timing side channels. Takes over #11159; its commits keep their author.
mapset uses the constant-time map. SP curve paths are unchanged. Two-scalarEC_POINT_mul()keeps Shamir's trick, which is variable time, and is documented as not for secret scalars.FP_ECCunderECC_TIMING_RESISTANTuses a new constant-time comb.wolfSSL_BN_mod_inverse()blinds its operand when an RNG is available._ctprimitives no longer branch on reduced operands, andConstantCompare()loads are volatile and used for the ATECC GCM tag check.Behavior changes
FP_ECCwithECC_TIMING_RESISTANT: key generation, ECDH and signing are slower than the old variable-time comb, andFP_LUTdefaults to 6 rather than 8, makingecc_mul2add()slower with a smaller table.--enable-spremains fastest for the curves it covers. Heap math, a scalar longer than the modulus and a modulus too large for the comb use the ladder.wc_MakeSakkeRsk()instantiates an RNG per call and can fail on RNG errors;wc_MakeSakkeRsk_ex()is added to pass one. SAKKE withWC_NO_RNGis now a build error, configure and CMake reject it, and--enable-all-cryptoleaves SAKKE off with--disable-rng.WOLFSSL_MP_INVMOD_CONSTANT_TIMEis removed and has no effect. DSA signing and RSA key generation always use their blinded or Fermat inverse, so builds that defined it now draw one more random value per DSA signature.mp_addmod_ct(),mp_submod_ct()andmp_div_2_mod_ct()returnMP_VALfor a negative operand or one wider than the modulus.WOLFSSL_NO_CONST_CMPis no longer defined automatically. MinimalWOLFCRYPT_ONLYbuilds defineWC_NO_CONST_CMP_APIinstead, leaving out onlywc_ConstantCompare().Resolves F-7097, F-7098, F-7099, F-7426, F-7427, F-15120, F-16129. ZD 22745. Reported by Alexandre Laroche.