Skip to content

Fix compiler-introduced branches in CT code - #12088

Open
SparkiDev wants to merge 1 commit into
wolfSSL:masterfrom
SparkiDev:thumb1_branch_fixes
Open

SparkiDev wants to merge 1 commit into
wolfSSL:masterfrom
SparkiDev:thumb1_branch_fixes

Conversation

@SparkiDev

@SparkiDev SparkiDev commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

Description

misc.c: add ctHide32(), a value barrier that hides a value from the optimizer so mask formulas cannot be turned back into branches.
ML-KEM (mlkem_cmp_c) and FrodoKEM decapsulation: use ctHide32() on the ciphertext-comparison result so implicit rejection does not branch on ciphertext validity.
wc_RNG_TestSeed(): rewrite the RCT/APT loops branch-free. Barriers are enabled via WC_RNG_SEED_CT_BARRIER, defined by default on affected targets; other targets use plain comparisons. Also shorten the majority-vote dependency chain.

Fixes zd#22785, zd#22784, zd#22772

Testing

Checking of assembly output for different platforms.

@SparkiDev SparkiDev self-assigned this Nov 11, 2026
Copilot AI balanced review requested due to automatic review settings November 11, 2026 01:39
@github-actions

github-actions Bot commented Nov 11, 2026 •

Copy link
Copy Markdown

MemBrowse Memory Report · Explore what changed →

gcc-arm-cortex-m0plus

  • FLASH: .text -12 B (-0.0%, 71,059 B / 262,144 B, total: 27% used)

gcc-arm-cortex-m3

  • FLASH: .text -12 B (-0.0%, 132,009 B / 262,144 B, total: 50% used)

gcc-arm-cortex-m4-min-ecc

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

gcc-arm-cortex-m4-sp-math

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

gcc-arm-cortex-m4-tls12

  • FLASH: .text -64 B (-0.0%, 132,753 B / 262,144 B, total: 51% used)

gcc-arm-cortex-m7-pq

  • FLASH: .text -64 B (-0.0%, 313,936 B / 1,048,576 B, total: 30% used)

stm32-sim-stm32h753

  • FLASH: .text -512 B (-0.3%, 203,564 B / 2,097,152 B, total: 10% used)

zephyr-frdm_rw612

  • FLASH: datas +16 B, log_const_area -8 B, rodata +20 B, text +10,016 B (+3.6%, 292,508 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.

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

With WC_RNG_SEED_CT_BARRIER enabled, the accepted cutoff 0xffffffffU causes the constant-time comparison to incorrectly reject non-repeating seeds with ENTROPY_RT_E.

1 open issue
What changed in this PR

This PR hardens wolfCrypt’s constant-time code against compiler-introduced branches in KEM decapsulation and RNG seed health tests.

Changes:

  • Adds the ctHide32() optimizer value barrier.
  • Applies barriers to ML-KEM and FrodoKEM ciphertext-validity masks.
  • Reworks RNG health-test loops with target-dependent barriers and a shorter majority-vote dependency chain.
File Description
wolfssl/​wolfcrypt/​misc.h Declares ctHide32().
wolfcrypt/​src/​wc_mlkem_poly.c Protects the ciphertext-comparison mask with barriers.
wolfcrypt/​src/​wc_frodokem.c Protects implicit-rejection mask generation with barriers.
wolfcrypt/​src/​random.c Adds conditional barriers and rewrites seed health-test arithmetic.
wolfcrypt/​src/​misc.c Implements the value barrier with assembly and volatile fallbacks.

Review effort: Balanced

Comment thread wolfcrypt/src/random.c Resolved
@SparkiDev
SparkiDev requested a balanced review from Copilot November 11, 2026 01:53
@SparkiDev

Copy link
Copy Markdown
Contributor Author

@claude review

@claude claude 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.

I reviewed this PR and didn't find any bugs. It changes constant-time code in the RNG seed health tests and in ML-KEM/FrodoKEM implicit rejection, so a human crypto reviewer should still look at it.

What was reviewed:

  • ctHide32 in misc.c/misc.h: the inline-asm guard, the volatile fallback, and that the NO_INLINE prototype matches.
  • The wc_RNG_TestSeed rewrite. The RCT mask select still gives the same result now that prevByte is always updated. votes + (same<<1) - 1 matches the old vote update, including when take is set. The borrow formula in RngSeedCtGTE is correct across the full word32 range.
  • The isEq/mlkem_cmp_c barrier wrapping. The inputs stay below 2^31, so the >> 31 formulas are still exact.
  • Not done: no check of the generated assembly on Thumb-1, RISC-V, MSP430 or AVR. Whether the compiler still emits branches can only be confirmed by compiling for those targets.
Extended reasoning...

The change touches wolfCrypt constant-time code: misc.c/misc.h, the seed health tests in random.c, and the ML-KEM and FrodoKEM decapsulation comparisons. This is security-sensitive crypto code where the side-channel properties matter. The diff is small (about 127 lines) and reading it found no logic errors. Whether it works depends on the code the compiler generates for specific embedded targets, which reading the source cannot confirm. Because it is crypto hardening code, a human review is warranted rather than an automated approval.

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 RNG health-test changes truncate accepted repetition-count cutoffs and mishandle large APT vote counts, causing false seed rejections or missed APT failures.

2 open issues
1 resolved since last review

Review effort: Balanced

Comment thread wolfcrypt/src/random.c Resolved
Comment thread wolfcrypt/src/random.c Resolved

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

Constant-time guarantees require cryptography-maintainer validation of generated assembly across supported targets and compiler configurations, including volatile fallback and barrier-disabled paths, but those artifacts are unavailable.

No open issues 🎉

2 resolved since last review

Review effort: Balanced

@SparkiDev

Copy link
Copy Markdown
Contributor Author

Jenkins: retest this please

misc.c: add ctHide32(), a value barrier that hides a value from the optimizer so mask formulas cannot be turned back into branches.
ML-KEM (mlkem_cmp_c) and FrodoKEM decapsulation: use ctHide32() on the ciphertext-comparison result so implicit rejection does not branch on ciphertext validity.
wc_RNG_TestSeed(): rewrite the RCT/APT loops branch-free.  Barriers are enabled via WC_RNG_SEED_CT_BARRIER, defined by default on affected targets; other targets use plain comparisons. Also shorten the majority-vote dependency chain.
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.

2 participants