Repository navigation
Conversation
|
PR changes generated with: |
|
There was a problem hiding this comment.
🟡 Changes recommended
Critical issues in the newly default path can corrupt digit counts, read uninitialized digits, and suppress multiplication or copy failures.
3 open issues
What changed in this PR
This PR hardens wolfCrypt’s ladder-based cryptographic operations by making copies that access both candidate values the default, reducing secret-dependent cache access.
Changes:
- Updates SP integer math, portable SP C, fastmath, and SAKKE copy selection.
- Adds
WOLFSSL_SP_FAST_MODEXP_COPYas an opt-in, disabled on capability targets. - Documents the security trade-off and adds six CI configurations covering both modes.
| File | Description |
|---|---|
wolfcrypt/src/tfm.c |
Defaults fastmath ladders to conditional copies. |
wolfcrypt/src/sp_int.c |
Defaults SP integer exponentiation to conditional copies. |
wolfcrypt/src/sp_c64.c |
Updates generated 64-bit C exponentiation and ECC copy selection. |
wolfcrypt/src/sp_c32.c |
Updates generated 32-bit C exponentiation and ECC copy selection. |
wolfcrypt/src/sakke.c |
Defaults the SAKKE ladder to conditional copies. |
doc/ASM_AND_MATH_DEFINES.md |
Documents the opt-in and its cache-security implications. |
.github/configs/os-check-linux.json |
Adds default and opt-in coverage across math implementations. |
Review effort: Balanced
ba2fd30 to
ac3c829
Compare
|
PR changes generated with: |
There was a problem hiding this comment.
🔵 Needs a closer look
The side-channel changes span multiple math backends and require cryptographic maintainer review of optimized machine code and cache-access traces, since source inspection and functional CI cannot establish address-oblivious execution.
No open issues 🎉
3 resolved since last review
Review effort: Balanced
ac3c829 to
fb9b79e
Compare
|
Jenkins: retest this please |
cc12a71 to
0b54f3a
Compare
|
Jenkins: retest this please |
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
wolfcrypt/src/tfm.c— Pre-existing, security-relevant: fastmath RSA private-key users built with WC_RSA_NONBLOCK still leak every exponent bit to a cache observer under the new oblivious default. The TFM_EXPTMOD_NB_MUL state at tfm.c:1897 writes the product straight into&nb->R[nb->y^1], and TFM_EXPTMOD_NB_MUL_RED at tfm.c:1902 reduces it in place there. Only the SQR states were switched to fp_cond_copy. Fix: send the multiply and its reduction through the R[2] scratch, then fp_cond_copy into R[0] and R[1], as _fp_exptmod_ct already does, so no write address in fp_exptmod_nb depends on nb->y.Why this was flagged
The trigger is a fastmath build (USE_FAST_MATH, TFM_TIMING_RESISTANT) with WC_RSA_NONBLOCK and without WC_NO_CACHE_RESISTANT. RSA private operations reach fp_exptmod_nb from rsa.c:2539 with the private exponent &key->d. In each bit iteration nb->y is the raw exponent bit. TFM_EXPTMOD_NB_MUL (tfm.c:1897) runs fp_mul(&nb->R[0], &nb->R[1], &nb->R[nb->y^1]), and TFM_EXPTMOD_NB_MUL_RED (tfm.c:1902) runs fp_montgomery_reduce(&nb->R[nb->y^1], ...). Each of these writes a whole fp_int at a secret-selected address. That is the same Prime+Probe exposure the PR removes from the SQR/SQR_RED states (tfm.c:1913-1946) and from _fp_exptmod_ct, which multiplies into R[2] and copies back with fp_cond_copy (tfm.c:2081-2113). The base branch leaks in the same way, so this is pre-existing. The PR description says this nonblock ladder is covered in "both modes", but neither the default nor WOLFSSL_SP_FAST_MODEXP_COPY changes the MUL states. The exptModNb_t R[3] scratch (tfm.h:680) is already there for the fix.
Verification: Trigger: a fastmath build (TFM_TIMING_RESISTANT) with WC_RSA_NONBLOCK and without WC_NO_CACHE_RESISTANT, doing RSA private operations. At tfm.c:1897 and tfm.c:1902, the MUL and MUL_RED states write a whole fp_int at an address chosen by the secret bit, under the new default too. The base (c5438f7) has the same lines at 1878-1884, so the leak predates this PR.
|
clean with skoll. Tests good with multi-test with |
0b54f3a to
4ac3d8f
Compare
|
@claude review |
There was a problem hiding this comment.
🟡 Changes recommended
The documentation omits remaining secret-selected ECC accesses and misclassifies DSP32 scalar-multiplication ladders as modular exponentiation, potentially misleading readers about address-oblivious coverage.
1 open issue
Review effort: Balanced
4ac3d8f to
63a7544
Compare
|
@claude review |
There was a problem hiding this comment.
🟡 Changes recommended
Whole-point accesses through sp_digit* violate strict aliasing for the int infinity member, risking stale infinity flags in optimized ECC code across multiple backends.
1 open issue
1 resolved since last review
Review effort: Balanced
The cache-resistant modular exponentiation and ECC/SAKKE scalar-mult ladders
selected the working value from one of two objects (t[0]/t[1], R[0]/R[1],
c[0]/c[1]) on each secret exponent bit with address masking:
_sp_copy((sp_int*)(((size_t)t[0] & sp_off_on_addr[j^1]) +
((size_t)t[1] & sp_off_on_addr[j ])), t[3]);
The mask removes a secret-dependent branch but still resolves to a single
secret-dependent address, so only the selected object is touched. After the
first set exponent bit the selector equals the raw exponent bit, so each copy
discloses it at cache-line granularity, letting a co-resident observer on an
SMT sibling sharing L1D recover RSA CRT exponents dP/dQ (and DH/ECC/SAKKE
secrets) by Prime+Probe.
Make the address-oblivious copy the default for all builds: _sp_cond_copy
(sp_int.c), sp_cond_select/sp_cond_memcpy (generated sp_c64.c/sp_c32.c),
fp_cond_copy (tfm.c) and mp_cond_copy (sakke.c), which touch both candidates
every iteration with a data-dependent mask so the accessed address does not
depend on the secret. This was previously reached only under WC_NO_PTR_INT_CAST.
The faster pointer-masking copy stays available behind the new opt-in
WOLFSSL_SP_FAST_MODEXP_COPY, for deployments where a co-resident cache observer
is outside the threat model. It rebuilds a pointer from integer arithmetic on
two addresses, so it is forced off on capability targets (WC_NO_PTR_INT_CAST),
leaving their behaviour unchanged. Cost of the default is ~2.5-4% on RSA private
operations (less at larger key sizes); the specialised SP ladder cost is within
benchmark noise. The windowed SP assembly back ends are unaffected: they select
from a fully scanned table, not two objects.
All generated SP targets are regenerated from the wolfSSL sp scripts and carry
the change: sp_c64.c, sp_c32.c and the assembly back ends sp_x86_64.c,
sp_arm64.c, sp_arm32.c, sp_armthumb.c, sp_cortexm.c and sp_riscv64.c. The asm
back ends use a windowed mod_exp (unaffected), but their non-blocking ECC
scalar-mult ladder had the same secret-selected store and is now oblivious too.
Verified: x86_64 at runtime (host) and arm64 under qemu exercise the non-blocking
ECC ladder; arm32/riscv64 cross-compile; a preprocessor scan confirms no
secret-indexed store remains outside the WOLFSSL_SP_FAST_MODEXP_COPY branch.
Add os-check CI legs exercising both the oblivious default and the opt-in across
sp_int.c, the generated ladder, fastmath and SAKKE, and document
WOLFSSL_SP_FAST_MODEXP_COPY in doc/ASM_AND_MATH_DEFINES.md.
Harden the promoted oblivious copy helpers, which were previously only built
under WC_NO_PTR_INT_CAST:
- _sp_cond_copy()/fp_cond_copy(): mark WC_MAYBE_UNUSED - the definition guard is
broader than the exptmod ladders that use them (e.g. ECC-with-comp-key or
RSA-verify-only builds define them but compile no private exponentiation),
which tripped -Werror=unused-function (e.g. tinytls13-cert-rsaverify).
- _sp_cond_copy(): derive the 'used'/'sign' masks at their own field width. The
digit-wide mask zero-extended to sp_size_t dropped the high bits of 'used'
when SP_WORD_SIZE==8 and an operand had >=256 digits, copying it as length 0.
- sakke_modexp_loop(): guard each mp_cond_copy() on err so a failure in the
multiply or an earlier copy is preserved instead of overwritten.
Follow-up CI fixes for the now-default oblivious copy:
- Initialise the full digit span of the exptmod ladder temporaries via a new
_sp_init_size_ct() helper. _sp_init_size() only zeroes dp[0], but
_sp_cond_copy() scans a fixed m->used digits, so a working value with fewer
significant digits left indeterminate padding that the copy read - reported
by valgrind (3.22) on the Ada wrapper RSA test. Applied to both
_sp_exptmod_mont_ex variants and both _sp_exptmod_ex variants.
- _sp_cond_copy(): build the 'used' mask with an explicit cast over unsigned
arithmetic so it is -Warith-conversion clean (smallstack-intelasm-precalc).
- sakke.c: declare the local wc_off_on_addr only for the pointer-copy opt-in;
the default oblivious copy does not use it, which was an unused variable under
WC_NO_GLOBAL_OBJECT_POINTERS (linuxkm -Werror).
Complete the obliviousness: also make the ladder step itself address-oblivious,
not just the copy. The generated SP ladder (sp_c64.c/sp_c32.c) and the fastmath
non-blocking ladder (tfm.c) still wrote the Montgomery product / point sum into
the secret-selected object every iteration:
sp_X_mont_mul_W(t[y^1], t[0], t[1], m, mp); /* secret store address */
fp_mul(&R[0], &R[1], &R[nb->y^1]); /* and its reduction */
so the dominant per-bit store disclosed the exponent bit even with the copy made
oblivious (sp_int.c and the tfm blocking _fp_exptmod_ct were already correct via
_sp_copy_2_ct/fp_copy_2_ct). Route the product/sum through the fixed scratch
(t[2], R[2]) and move it into the secret slot by touching both candidates; no
new temporary is needed. Covers the blocking and non-blocking SP ladders and the
SP ECC add/dbl (regenerated from the sp scripts), and fp_exptmod_nb's
MUL/MUL_RED states. The WOLFSSL_SP_FAST_MODEXP_COPY path keeps the single
secret-selected store.
Also:
- sp_int.c: define _sp_cond_copy() when WC_PROTECT_ENCRYPTED_MEM is set even
with WC_NO_CACHE_RESISTANT. The protected-memory _sp_exptmod_ex ladder calls
it but it was guarded only by !WC_NO_CACHE_RESISTANT, so that combination
failed to compile (now the default caller rather than WC_NO_PTR_INT_CAST-only).
Mirrors the fp_cond_copy guard in tfm.c.
- doc/ASM_AND_MATH_DEFINES.md: note sp_dsp32.c (Hexagon DSP, EXTRA_DIST, not
built by configure) is not yet converted and still uses the secret-dependent
pointer copy.
Also zero the full 2*words span of the ladder temporaries in the generated
mod_exp_cache_resist (sp_c64.c/sp_c32.c). mont_norm only sets the low words
digits, but the now-default address-oblivious copy reads every digit of
t[0]/t[1], so the upper half was read uninitialized (valgrind 3.22 on the Ada
RSA test); mod_exp_small and the non-blocking ladder already zeroed.
doc/ASM_AND_MATH_DEFINES.md: record that all generated back ends are covered and
that the only remaining exception is sp_dsp32.c (Hexagon, EXTRA_DIST, not emitted
by the scripts), whose P-256 ECC scalar-mult ladder - not a private-key mod_exp -
still uses the secret-dependent pointer copy.
Copy a sp_digit word at a time in the generated sp_cond_memcpy()/sp_cond_select()
rather than a byte at a time. The oblivious ladder calls these several times per
exponent/scalar bit, so byte granularity was slow on embedded SP_SMALL builds
without auto-vectorisation (measured ~1.4x ECDSA P-256 sign at -Os; neutral on
x86_64 -O2, which already vectorises the byte loop). All callers pass sp_digit
aligned buffers a whole number of words wide. The word is moved with XMEMCPY
through a byte pointer, not an sp_digit lvalue, so a struct member that is not a
sp_digit (sp_point's int infinity) is not type-punned - which would break strict
aliasing and could leave an optimiser using a stale infinity flag. XMEMCPY of a
constant sizeof(sp_digit) lowers to a single load/store, keeping the word speed.
Regenerated across all back ends.
Add WOLFSSL_SP_FAST_MODEXP_COPY to .wolfssl_known_macro_extras (sorted) so the
unknown-macro source check recognises the new opt-in; it is tested but never
defined in-tree.
63a7544 to
262dccf
Compare
|
@Cluade review |
There was a problem hiding this comment.
🔵 Needs a closer look
Security-sensitive ladder changes across multiple CPU backends require specialist validation of optimized machine-code address independence and cross-target arithmetic correctness beyond source inspection.
No open issues 🎉
1 resolved since last review
Review effort: Balanced
Description
The cache-resistant modular exponentiation and ECC/SAKKE scalar-mult ladders selected the working value from one of two objects (t[0]/t[1], R[0]/R[1], c[0]/c[1]) on each secret exponent bit with address masking:
The mask removes a secret-dependent branch but still resolves to a single secret-dependent address, so only the selected object is touched. After the first set exponent bit the selector equals the raw exponent bit, so each copy discloses it at cache-line granularity, letting a co-resident observer on an SMT sibling sharing L1D recover RSA CRT exponents dP/dQ (and DH/ECC/SAKKE secrets) by Prime+Probe.
Make the address-oblivious copy the default for all builds: _sp_cond_copy (sp_int.c), sp_cond_select/sp_cond_memcpy (generated sp_c64.c/sp_c32.c), fp_cond_copy (tfm.c) and mp_cond_copy (sakke.c), which touch both candidates every iteration with a data-dependent mask so the accessed address does not depend on the secret. This was previously reached only under WC_NO_PTR_INT_CAST.
The faster pointer-masking copy stays available behind the new opt-in WOLFSSL_SP_FAST_MODEXP_COPY, for deployments where a co-resident cache observer is outside the threat model. It rebuilds a pointer from integer arithmetic on two addresses, so it is forced off on capability targets (WC_NO_PTR_INT_CAST), leaving their behaviour unchanged. Cost of the default is ~2.5-4% on RSA private operations (less at larger key sizes); the specialised SP ladder cost is within benchmark noise. The windowed SP assembly back ends are unaffected: they select from a fully scanned table, not two objects.
sp_c64.c and sp_c32.c are regenerated from the wolfSSL sp scripts. The other SP targets (sp_arm*, sp_cortexm, sp_riscv64) share the generator and still need regeneration to pick up the change.
Add os-check CI legs exercising both the oblivious default and the opt-in across sp_int.c, the generated ladder, fastmath and SAKKE, and document WOLFSSL_SP_FAST_MODEXP_COPY in doc/ASM_AND_MATH_DEFINES.md.
Fixes zd#22721
Testing
./configure --enable-sakke --enable-sp-math-all — sp_int.c + sakke.c, oblivious default
./configure - --enable-sakke --enable-sp-math-all -DWOLFSSL_SP_FAST_MODEXP_COPY — sp_int.c + sakke.c, pointer opt-in
./configure - --enable-sp --disable-sp-asm -DWOLFSSL_SP_LADDER_MODEXP — generated sp_c64.c/sp_c32.c ladder, oblivious default
./configure - --enable-sp --disable-sp-asm -DWOLFSSL_SP_LADDER_MODEXP -DWOLFSSL_SP_FAST_MODEXP_COPY — sp_c64.c/sp_c32.c ladder, pointer opt-in
./configure - --enable-fastmath — tfm.c, oblivious default
./configure - --enable-fastmath -DWOLFSSL_SP_FAST_MODEXP_COPY — tfm.c, pointer opt-in
./configure - --enable-fastmath -DWC_PROTECT_ENCRYPTED_MEM — tfm.c protect-encrypted-mem ladder (validated locally)
./configure - --enable-fastmath -DWC_RSA_NONBLOCK — tfm.c nonblock ladder, both modes (compile-checked locally)