fix: constant-time Ed sig order check#437
Conversation
Replace the byte-by-byte s < curve-order comparison in wp_ed25519_digest_verify and wp_ed448_digest_verify with a branchless full-width magnitude compare using the existing wp_ct_int_mask_lt helper. The old loop broke early on the first differing byte, making iteration count (and thus timing) depend on the position of the highest differing s byte. Semantics preserved: reject iff s > curve order.
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.
Fixes a variable-time signature scalar (s) vs curve-order check in Ed25519/Ed448 verification by replacing an early-exit byte-compare loop with a constant-time full-width magnitude compare.
Changes:
- Replaced the branchy (early-break) order check in
wp_ed25519_digest_verifywith a fixed-iteration constant-time compare usingwp_ct_int_mask_lt. - Applied the same constant-time compare pattern to
wp_ed448_digest_verify.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| byte gt = 0; /* s > order found at a more-significant byte */ | ||
| byte lt = 0; /* s < order found at a more-significant byte */ | ||
| /* Constant-time full-width magnitude compare, big-endian scan with no | ||
| * early break: reject iff s > order (accept when equal or less). */ | ||
| for (i = ED25519_KEY_SIZE - 1; i >= 0; i--) { | ||
| if (sig[ED25519_KEY_SIZE + i] > wp_ed25519_order[i]) { | ||
| ok = 0; | ||
| } | ||
| if (sig[ED25519_KEY_SIZE + i] != wp_ed25519_order[i]) { | ||
| break; | ||
| } | ||
| byte s = sig[ED25519_KEY_SIZE + i]; | ||
| byte o = wp_ed25519_order[i]; | ||
| byte eq = (byte)~(gt | lt); /* still tied above */ | ||
| gt |= (byte)(eq & wp_ct_int_mask_lt(o, s)); /* o < s => s > order */ | ||
| lt |= (byte)(eq & wp_ct_int_mask_lt(s, o)); /* s < order */ | ||
| } | ||
| ok &= (int)(1u & ~(unsigned int)gt); |
| byte gt = 0; /* s > order found at a more-significant byte */ | ||
| byte lt = 0; /* s < order found at a more-significant byte */ | ||
| /* Constant-time full-width magnitude compare, big-endian scan with no | ||
| * early break: reject iff s > order (accept when equal or less). */ | ||
| for (i = ED448_KEY_SIZE - 1; i >= 0; i--) { | ||
| if (sig[ED448_KEY_SIZE + i] > wp_ed448_order[i]) { | ||
| ok = 0; | ||
| } | ||
| if (sig[ED448_KEY_SIZE + i] != wp_ed448_order[i]) { | ||
| break; | ||
| } | ||
| byte s = sig[ED448_KEY_SIZE + i]; | ||
| byte o = wp_ed448_order[i]; | ||
| byte eq = (byte)~(gt | lt); /* still tied above */ | ||
| gt |= (byte)(eq & wp_ct_int_mask_lt(o, s)); /* o < s => s > order */ | ||
| lt |= (byte)(eq & wp_ct_int_mask_lt(s, o)); /* s < order */ | ||
| } | ||
| ok &= (int)(1u & ~(unsigned int)gt); |
| /* Constant-time full-width magnitude compare, big-endian scan with no | ||
| * early break: reject iff s > order (accept when equal or less). */ | ||
| for (i = ED25519_KEY_SIZE - 1; i >= 0; i--) { | ||
| if (sig[ED25519_KEY_SIZE + i] > wp_ed25519_order[i]) { | ||
| ok = 0; | ||
| } | ||
| if (sig[ED25519_KEY_SIZE + i] != wp_ed25519_order[i]) { | ||
| break; | ||
| } | ||
| byte s = sig[ED25519_KEY_SIZE + i]; | ||
| byte o = wp_ed25519_order[i]; | ||
| byte eq = (byte)~(gt | lt); /* still tied above */ | ||
| gt |= (byte)(eq & wp_ct_int_mask_lt(o, s)); /* o < s => s > order */ | ||
| lt |= (byte)(eq & wp_ct_int_mask_lt(s, o)); /* s < order */ | ||
| } |
aidangarske
left a comment
aidangarske
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: COMMENT
Findings: 1 total — 1 posted, 0 skipped
Posted findings
- [Medium] Boundary coverage missing for rewritten Ed signature order compare —
src/wp_ecx_sig.c:460-467, src/wp_ecx_sig.c:677-684
Review generated by Skoll.
| byte lt = 0; /* s < order found at a more-significant byte */ | ||
| /* Constant-time full-width magnitude compare, big-endian scan with no | ||
| * early break: reject iff s > order (accept when equal or less). */ | ||
| for (i = ED25519_KEY_SIZE - 1; i >= 0; i--) { |
There was a problem hiding this comment.
🟡 [Medium] Boundary coverage missing for rewritten Ed signature order compare
💡 SUGGEST test
The PR replaces the scalar/order validation in both Ed25519 and Ed448 verification with new branchless comparison logic, but the existing ECX tests only cover valid signatures and generic bad signatures such as flipping sig[1], which mutates the R half rather than exercising S/order boundaries. This means endian mistakes, inverted mask operands, or equality handling regressions in the new compare could pass the current test suite.
Recommendation: Add focused regression coverage for the rewritten comparison, ideally table-driven cases for both Ed25519 and Ed448 with S values equal to the order, one below, one above, and differences in both high and low bytes. If the compare remains inline, consider factoring the magnitude check into a small static helper so the boundary cases can be unit-tested directly.
Bug:
wp_ed25519_digest_verifyandwp_ed448_digest_verify(src/wp_ecx_sig.c) validate that the signature scalarsis below the curve order with a byte-by-byte compare thatbreaks on the first differing byte. Iteration count — and therefore execution time — depends on the position of the highest differingsbyte, leaking information aboutsin a variable-time path.Fix: replace the branchy loop in both functions with a branchless full-width magnitude compare built on the in-tree
wp_ct_int_mask_lthelper (include/wolfprovider/internal.h, src/wp_internal.c). The scan runs a fixed number of iterations (32 for Ed25519, 57 for Ed448) with no early exit. Exact semantics preserved: reject iffs > curve_order(accept when equal or less).Impact: Ed signatures are normally public, so this matters chiefly for blind / threshold signing constructions where
s(or a component) is secret. Severity low.Build-verified: compiled the translation unit with
gcc -c -O2 -Wallagainst a wolfSSL--enable-allinstall + system OpenSSL headers (EXIT_CC=0). Disassembly of both functions confirms the loop terminator is now a fixed pointer-vs-end compare (data-independent iteration count) and the prior data-dependent early-breakjneis gone; the twowp_ct_int_mask_ltcalls are branchless.Reported by static analysis (Fenrir finding 841).
[fenrir-sweep:held]