Skip to content

Check the ECC scalar length without timing that depends on its top bit, and clear the mp_int in mp_rand - #12129

Open
dgarske wants to merge 2 commits into
wolfSSL:masterfrom
dgarske:ecc_scalar_len
Open

dgarske wants to merge 2 commits into
wolfSSL:masterfrom
dgarske:ecc_scalar_len

Conversation

@dgarske

@dgarske dgarske commented Nov 13, 2026

Copy link
Copy Markdown
Member

Description

Two follow-ups to #12023.

ECC scalar length. The generic point multiplication entry points (wc_ecc_mulmod_ex(), wc_ecc_mulmod_ex2() and the SP wrapper) range-checked the scalar with mp_count_bits(), and the FP_ECC comb compared its byte length. With fastmath and heap math, and with SP math on targets that have no bit-scan instruction, mp_count_bits() loops over the bits of the top digit, so the time it takes depends on the scalar's bit length. ecc_k_too_long() now ORs together the bits of the scalar at and above the limit instead, so only the number of digits in the scalar affects the time. The limits and error codes are unchanged. The remaining value-dependent branches in the generic point arithmetic, and the separate wc_ecc_mulmod_ex() implementations in the NXP KSDK and STM32 ports, are not changed here.

mp_rand on heap math. mp_rand() sized its output with mp_set_bit(), which only ORs one bit into whatever a already holds. When a had more digits than requested, the result kept the old high digits and their sign. a is now zeroed first, with mp_2expt(). SP math and fastmath set used directly and are unchanged.

Testing

test_wc_ecc_mulmod_k_len covers the scalar length boundary on P-256 and P-521, and test_mp_rand covers a call on an mp_int that already holds a wider value, on every math back end.

ZD 22745.

@dgarske dgarske self-assigned this Nov 13, 2026
Copilot AI balanced review requested due to automatic review settings November 13, 2026 20:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The P-256 test guard omits the minimum-key-size condition, causing the new test to fail on valid builds where ECC_MIN_KEY_SZ exceeds 256 and P-256 is excluded.

1 open issue
What changed in this PR

Follows up on #12023 by hardening wolfCrypt’s ECC scalar-length checks and correcting random-integer generation when reusing heap-math values.

Changes:

  • Replaces scalar bit-count checks with digit-based checks that avoid timing dependence on the top set bit while preserving existing limits.
  • Clears heap-math mp_rand() output before sizing it, preventing stale high digits and sign from surviving.
  • Adds regression tests for ECC scalar-length boundaries and random-integer reuse.
File Description
wolfcrypt/​src/​wolfmath.c Clears reused heap-math integers before random generation.
wolfcrypt/​src/​ecc.c Adds and applies the scalar-length helper across multiplication paths.
tests/​api/​test_wolfmath.c Tests random generation into a previously wider integer.
tests/​api/​test_ecc.h Declares and registers the scalar-length test.
tests/​api/​test_ecc.c Adds P-256 and P-521 scalar-length boundary tests.

Review effort: Balanced

Comment thread tests/api/test_ecc.c Resolved
@github-actions

github-actions Bot commented Nov 13, 2026 •

Copy link
Copy Markdown

MemBrowse Memory Report · Explore what changed →

gcc-arm-cortex-m0plus

  • FLASH: .text +76 B (+0.1%, 71,387 B / 262,144 B, total: 27% used)

gcc-arm-cortex-m3

  • FLASH: .text +76 B (+0.1%, 132,365 B / 262,144 B, total: 50% used)

gcc-arm-cortex-m4

  • FLASH: .text +128 B (+0.1%, 209,704 B / 262,144 B, total: 80% used)

gcc-arm-cortex-m4-baremetal

  • FLASH: .text +64 B (+0.1%, 74,159 B / 262,144 B, total: 28% used)

gcc-arm-cortex-m4-crypto-only

  • FLASH: .text +64 B (+0.0%, 182,859 B / 262,144 B, total: 70% used)

gcc-arm-cortex-m4-dtls13

  • FLASH: .text +64 B (+0.0%, 197,408 B / 1,048,576 B, total: 19% used)

gcc-arm-cortex-m4-min-ecc

  • FLASH: .text +64 B (+0.1%, 68,625 B / 262,144 B, total: 26% used)

gcc-arm-cortex-m4-openssl-compat

  • FLASH: .text +64 B (+0.0%, 804,420 B / 1,048,576 B, total: 77% used)

gcc-arm-cortex-m4-pkcs7

  • FLASH: .text +128 B (+0.1%, 224,204 B / 262,144 B, total: 86% used)

gcc-arm-cortex-m4-pq

  • FLASH: .text +128 B (+0.0%, 314,352 B / 1,048,576 B, total: 30% used)

gcc-arm-cortex-m4-sp-math

  • FLASH: .text +64 B (+0.1%, 68,625 B / 262,144 B, total: 26% used)

gcc-arm-cortex-m4-tls12

  • FLASH: .text +64 B (+0.0%, 133,129 B / 262,144 B, total: 51% used)

gcc-arm-cortex-m4-tls13

  • FLASH: .text +64 B (+0.0%, 251,010 B / 262,144 B, total: 96% used)

gcc-arm-cortex-m7

  • FLASH: .text +64 B (+0.0%, 209,704 B / 262,144 B, total: 80% used)

gcc-arm-cortex-m7-pq

  • FLASH: .text +128 B (+0.0%, 315,184 B / 1,048,576 B, total: 30% used)

gcc-arm-cortex-m7-tls13

  • FLASH: .text +64 B (+0.0%, 251,010 B / 262,144 B, total: 96% used)

zephyr-frdm_rw612

  • FLASH: datas +16 B, log_const_area -8 B, rodata +28 B, text +11,832 B (+4.2%, 294,332 B / 67,108,864 B, total: 0% used)
  • RAM: bss +49 B, datas +16 B (+0.1%, 58,423 B / 983,040 B, total: 6% used)

No memory changes detected for:


Catch memory regressions in CI with MemBrowse. Free for open source.

@dgarske
dgarske force-pushed the ecc_scalar_len branch 2 times, most recently from b999b63 to d4ffd30 Compare November 13, 2026 23:09
@dgarske
dgarske requested review from wolfSSL-Fenrir-bot and a balanced review from Copilot November 13, 2026 23:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Secret-scalar timing changes require human validation of target/compiler side-channel behavior on targets without bit-scan instructions, which source inspection and a standalone boundary check cannot establish.

No open issues 🎉

1 resolved since last review

Review effort: Balanced

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fenrir Automated Review — PR #12129

Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfssl-bugs
Not reviewed (no scan target covers them): tests/api/test_ecc.h
Coverage: 4 of 4 in-scope changed file(s) opened by the reviewer

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Reported findings require changes before merge.

Review tier: Max

Comment thread tests/api/test_ecc.c
}

/* Check the scalar length limit of wc_ecc_mulmod() on one curve. */
static int test_ecc_mulmod_k_len_curve(int curveId)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static test helper unused when no per-curve guard holds, breaking -Werror builds · Dead/unreachable code

Static test_ecc_mulmod_k_len_curve is guarded only by TEST_ECC_MULMOD_K_LEN, which has no curve conditions. With WOLFSSL_SP_MATH+WOLFSSL_SP_NO_256 and no WOLFSSL_SP_521, or P-384-only ECC_USER_CURVES, both call sites drop out and -Wunused-function under -Werror fails the build.

Suggested fix: Add the OR of the P-256 and P-521 call-site conditions to the TEST_ECC_MULMOD_K_LEN guard, as test_ecc_import_compressed_no_root does at line 1501.
Basis: GCC manual, -Wunused-function: warns whenever a non-inline static function is unused and is enabled by -Wall; wolfSSL's AX_HARDEN_CC_COMPILER_FLAGS adds -Werror in VCS checkouts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants