fix: constant time - #54
Conversation
📝 WalkthroughWalkthroughThis PR implements constant-time cryptographic operations across U1024 and FieldElement types, introduces comprehensive benchmarks and test coverage for constant-time behavior, updates multiple dependencies, refactors RNG patterns in examples, and removes obsolete code. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
examples/generate_params.rs (2)
96-109: Masking logic is correct, but consider input validation.The bit-masking logic correctly ensures the generated BigUint has exactly
bitsbits. The mask(1u8 << extra_bits) - 1applied to the most significant byte is correct for little-endian byte order.However, consider adding validation for edge cases, such as when
bits = 0, which would cause unexpected behavior.🛡️ Suggested input validation
fn gen_biguint<R: Rng>(rng: &mut R, bits: usize) -> BigUint { + if bits == 0 { + return BigUint::from(0u32); + } let bytes_needed = (bits + 7).div_ceil(8); let mut bytes = vec![0u8; bytes_needed]; rng.fill(&mut bytes[..]);
111-127: Rejection sampling is not constant-time.While the rejection sampling approach is mathematically correct for generating uniform random values in the range
[low, high), it is not constant-time because the number of loop iterations varies depending on the random values generated.Given that this PR focuses on constant-time cryptographic operations, this might be acceptable for example code used in parameter generation (not runtime cryptographic operations). However, be aware that this could leak timing information about the range size or random values in side-channel sensitive contexts.
Additionally, consider adding input validation for the case where
low >= high.🛡️ Suggested input validation
fn gen_biguint_range<R: Rng>(rng: &mut R, low: &BigUint, high: &BigUint) -> BigUint { use num_traits::Zero; + assert!(low < high, "low must be less than high"); let range = high - low; - if range.is_zero() { - return low.clone(); - }src/poly/univariate.rs (1)
119-124: Consider performance implications of the new approach.The refactored derivative implementation is mathematically correct and more explicit. The
saturating_sub(1)on Line 119 adds defensive robustness, though it's redundant given the early return on Line 115-117.The new scalar multiplication approach (Lines 121-122) creates a
FieldElementfor each index, which may have overhead compared to accumulation-based approaches. For polynomials with many terms, consider profiling to ensure this doesn't introduce performance regressions.benches/constant_time_bench.rs (1)
93-95: Consider formatting the long hex string for readability.The 256-character hex modulus on Line 94 could be split across multiple lines for improved readability.
♻️ Proposed formatting
let modulus = U1024::from_hex( - "0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFB43", + "0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF\ + FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFB43", );
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
Cargo.tomlbenches/constant_time_bench.rsexamples/generate_params.rsexamples/poly_display.rslumen-math-macros/src/lib.rssrc/big_int/u1024.rssrc/field/element.rssrc/poly/univariate.rstests/constant_time_test.rstests/derive_test.rstests/digest_test.rstests/field_element_test.rs
💤 Files with no reviewable changes (4)
- tests/field_element_test.rs
- tests/derive_test.rs
- examples/poly_display.rs
- tests/digest_test.rs
🧰 Additional context used
🧬 Code graph analysis (6)
benches/constant_time_bench.rs (2)
src/big_int/u1024.rs (4)
conditional_select(999-1020)zero(973-975)from_u64(403-407)from_hex(322-343)src/field/element.rs (2)
conditional_select(190-192)zero(126-131)
src/field/element.rs (3)
src/big_int/u1024.rs (1)
conditional_select(999-1020)src/traits.rs (1)
conditional_select(25-25)src/big_int/backend/avx2.rs (1)
conditional_select(56-78)
src/poly/univariate.rs (2)
src/field/element.rs (1)
new(107-115)src/big_int/u1024.rs (1)
from_u64(403-407)
src/big_int/u1024.rs (3)
src/field/element.rs (2)
one(134-136)conditional_select(190-192)src/traits.rs (2)
one(22-22)conditional_select(25-25)src/big_int/backend/avx2.rs (1)
conditional_select(56-78)
examples/generate_params.rs (1)
src/big_int/u1024.rs (1)
bits(284-297)
tests/constant_time_test.rs (2)
src/big_int/u1024.rs (4)
from_u64(403-407)conditional_select(999-1020)bits(284-297)from_hex(322-343)src/field/element.rs (1)
conditional_select(190-192)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test and Coverage
🔇 Additional comments (24)
examples/generate_params.rs (1)
11-11: No action needed. Therand::rng()API is correct for rand 0.9 (the version specified in Cargo.toml). This function replaced the deprecatedrand::thread_rng()as of rand 0.9.0 and is the recommended approach.src/poly/univariate.rs (1)
13-13: LGTM!The addition of
U1024to the imports is necessary for the updatedderivativeimplementation.lumen-math-macros/src/lib.rs (1)
144-146: LGTM!The conversion from imperative
forloops to functional-stylefor_eachis a clean stylistic improvement that maintains the same behavior while making the code more idiomatic.Also applies to: 155-157, 170-172, 180-182
Cargo.toml (2)
36-38: LGTM!The new
constant_time_benchbenchmark target is correctly configured withharness = false, which is appropriate for Criterion-based benchmarks.
8-8: Verify compatibility of dependency updates and confirm code compiles with new versions.The dependency bumps include significant breaking changes:
rand0.8 → 0.9: Major API changes including method renames (gen→random,gen_range→random_range,thread_rng()→rng()), trait renames (from_rng→try_from_rng,from_entropy→from_os_rng), feature renames (serde1→serde,getrandom→os_rng), and MSRV bump to 1.63.0criterion0.5 → 0.8 (minor version bumps)proptest1.4 → 1.9 (minor version bumps)Direct usage of affected rand APIs appears minimal in the codebase. However,
num-bigint0.4.6 depends onrandvia itsfeatures = ["rand"], which requires verification that the version is compatible. Run the full test suite to confirm all functionality works correctly with these updated versions.src/field/element.rs (2)
190-192: LGTM!The new
conditional_selectfunction correctly provides a constant-time selection primitive by delegating toU1024::conditional_select. This is a key building block for constant-time cryptographic operations.
202-206: Excellent constant-time refactoring.The refactored
powmethod eliminates timing side-channels by:
- Always computing the
product(Line 203)- Using
conditional_selectto choose the result without branching (Line 205)- Using
square()for explicit squaring (Line 206)This approach trades a small performance cost (computing unused multiplications) for security, which is the correct tradeoff for cryptographic code.
benches/constant_time_bench.rs (7)
1-11: LGTM!The file header provides clear documentation of the benchmark's purpose and usage, with helpful guidelines for interpreting constant-time results (timing differences within ~5% noise margins).
23-47: LGTM!The
bench_conditional_selectfunction provides excellent coverage:
- Fixed
trueandfalsecases establish baseline timing- The alternating case defeats branch prediction, which is critical for detecting timing variations
53-85: LGTM!The
bench_bitsfunction tests a comprehensive range of bit patterns (zero, small, medium, large, sparse high-limb) to verify constant-time behavior of bit-length operations across different input distributions.
91-142: Excellent coverage of constant-time exponentiation.The
bench_mod_pow_hamming_weightfunction is critical for verifying constant-time behavior in modular exponentiation. By testing exponents with Hamming weights of 1, 32, 64, and 128, it can detect timing leaks that depend on the number of set bits—a common vulnerability in non-constant-time implementations.The
sample_size(50)is appropriate for balancing statistical significance with benchmark duration for this expensive operation.
148-178: LGTM!The
bench_div_remfunction appropriately tests division with varying dividend sizes (small, medium, large) against a fixed divisor. Thesample_size(50)is suitable for this moderately expensive operation.
188-226: Excellent timing leak detection design.The
bench_timing_leak_detectionfunction is specifically crafted to expose timing side-channels by comparing:
- Sparse vs. dense exponents in modular exponentiation
truevs.falseinconditional_selectThe
sample_size(100)provides good statistical power for detecting timing differences. This is a critical test for validating the constant-time implementation.
228-236: LGTM!The benchmark group configuration correctly includes all test functions and uses the standard Criterion setup.
tests/constant_time_test.rs (5)
13-113: Comprehensive test coverage forconditional_select.The tests cover the essential cases: true/false selection, zero values, max values, identical inputs, and per-limb verification. The assertions correctly match the implementation semantics where
choice=truereturns the first argument.
119-178: Thorough edge-case coverage forbits().Tests correctly verify boundary conditions including zero, single-bit values, multi-limb placements, and full 1024-bit maximum. The expected values align with the constant-time implementation logic.
184-269: Good coverage ofmod_powincluding mathematical properties.The test suite validates basic operations, edge cases (zero/one exponent, modulus one), Fermat's Little Theorem verification, and panic behavior. This provides confidence in both correctness and robustness.
275-342: Solid test coverage for constant-timediv_rem.The tests verify key invariants (
q * b + r = a) and cover important edge cases. The panic test correctly expects "Division by zero".
349-459: Valuable property-based tests for constant-time guarantees.The Hamming weight tests are particularly important for cryptographic implementations—they verify that results remain correct regardless of bit patterns, which is essential for timing-attack resistance. The invariant checks across multiple values provide additional confidence.
src/big_int/u1024.rs (5)
145-146: Clean addition ofONEconstant.The constant correctly represents the value 1 with 16 limbs, consistent with the existing
one()method. This enables more efficient compile-time usage.
802-819: Correct constant-time binary long division.The implementation properly uses:
- Fixed 1024 iterations (constant time regardless of operand sizes)
- Mask-based
conditional_selectfor both remainder and quotient updates- Borrow check from
borrowing_subto determine subtraction feasibilityThe algorithm correctly implements the binary division invariant where
remainderaccumulates bits and conditionally subtracts the divisor.
928-944: Proper constant-time modular exponentiation.The implementation correctly:
- Always computes the multiplication (
result.mod_mul(&base, ...))- Uses
conditional_selectto conditionally updateresult- Always squares
baseregardless of the exponent bitThis ensures uniform execution time across all exponent values, preventing timing attacks on the secret exponent.
1010-1018: Solid constant-time conditional selection withblack_box.The implementation correctly uses:
- Mask derivation via
wrapping_sub(avoiding branches)core::hint::black_boxto prevent compiler optimizations that might reintroduce data-dependent branches- Bitwise operations that execute in constant time
The semantics are consistent with the AVX2 implementation in
src/big_int/backend/avx2.rs.
284-297: Well-implemented constant-time bit-length computation.The mask-based conditional update ensures no data-dependent branching. The iteration order (low-to-high) correctly allows higher limb results to overwrite lower ones.
Note on
leading_zeros(): This maps to LLVM'sctlzintrinsic, which on x86_64 compiles to a singleLZCNTinstruction (constant-time) only when the target supports it (modern CPUs: Intel Haswell+, AMD Bulldozer+) or when compiled with-C target-feature=+lzcnt. Without these conditions, LLVM falls back toBSRplus a zero-check, which is not constant-time. If constant-time behavior is required across all x86_64 targets, ensure compilation flags are set accordingly or use a custom constant-time leading-zeros implementation.
Codecov Report❌ Patch coverage is
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
1 participant
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.