fix: sigma generation uses 3 bits of entropy per byte instead of 8#12
fix: sigma generation uses 3 bits of entropy per byte instead of 8#12matsuro-hadouken wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes sigma generation in the IBE encrypt path to use full-byte CSPRNG output (8 bits/byte) instead of an incorrect [0, 8) distribution that severely reduced entropy.
Changes:
- Replace sigma generation via
Uniform::new(0u8, 8u8)sampling withrng.fill_bytes(&mut sigma). - Add a regression test intended to ensure sigma uses the full
[0, 256)byte range.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn test_sigma_uses_full_byte_range() { | ||
| let mut rng = rand::rng(); | ||
| let mut seen = [false; 256]; | ||
|
|
||
| // Generate enough sigma values to cover the full byte range. | ||
| // With 16 bytes per sigma and 256 possible values, ~100 iterations | ||
| // is statistically sufficient (coupon collector: ~1500 bytes needed, | ||
| // 100 * 16 = 1600). | ||
| for _ in 0..100 { | ||
| let mut sigma = [0u8; 16]; | ||
| rng.fill_bytes(&mut sigma); | ||
| for &byte in &sigma { | ||
| seen[byte as usize] = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
The new regression test does not exercise encrypt’s sigma generation (it only calls rng.fill_bytes directly), so it won’t fail if encrypt regresses back to biased sampling. Consider factoring sigma generation into a helper (or allowing encrypt to accept an RNG) and asserting on that code path instead.
| // Generate enough sigma values to cover the full byte range. | ||
| // With 16 bytes per sigma and 256 possible values, ~100 iterations | ||
| // is statistically sufficient (coupon collector: ~1500 bytes needed, | ||
| // 100 * 16 = 1600). | ||
| for _ in 0..100 { | ||
| let mut sigma = [0u8; 16]; | ||
| rng.fill_bytes(&mut sigma); | ||
| for &byte in &sigma { | ||
| seen[byte as usize] = true; | ||
| } | ||
| } | ||
|
|
||
| let covered = seen.iter().filter(|&&v| v).count(); | ||
| // With 1600 random bytes from a uniform [0, 256) distribution, | ||
| // the probability of missing even a single value is < 0.002%. | ||
| // The old buggy code would only ever produce values in [0, 8), | ||
| // covering at most 8 out of 256 values. | ||
| assert!( | ||
| covered > 200, | ||
| "sigma byte coverage too low: {covered}/256 — only [0, {}) seen, \ | ||
| expected full [0, 256) byte range", | ||
| seen.iter().rposition(|&v| v).unwrap_or(0) + 1, | ||
| ); |
There was a problem hiding this comment.
This test is probabilistic and can be flaky across CI runs (and it’s also relatively expensive for a unit test). A deterministic approach (e.g., using a seeded RNG or a small custom RNG that produces known bytes including values >= 128) would make the regression test stable while still catching the original [0,8)-range bug.
| assert!( | ||
| covered > 200, | ||
| "sigma byte coverage too low: {covered}/256 — only [0, {}) seen, \ | ||
| expected full [0, 256) byte range", | ||
| seen.iter().rposition(|&v| v).unwrap_or(0) + 1, |
There was a problem hiding this comment.
The test claims to assert sigma covers the full [0,256) range, but covered > 200 allows missing up to 56 byte values. If the requirement is truly full-range capability, tighten the assertion to check for evidence of high-bit values (e.g., observing a value >= 128) or otherwise align the test name/docs with what’s actually being asserted.
| assert!( | |
| covered > 200, | |
| "sigma byte coverage too low: {covered}/256 — only [0, {}) seen, \ | |
| expected full [0, 256) byte range", | |
| seen.iter().rposition(|&v| v).unwrap_or(0) + 1, | |
| let max_seen = seen.iter().rposition(|&v| v).unwrap_or(0); | |
| assert!( | |
| covered > 200 && max_seen >= 128, | |
| "sigma byte coverage too low or lacks high-bit values: \ | |
| {covered}/256 — only [0, {}) seen, expected full [0, 256) byte range", | |
| max_seen + 1, |
Uniform::new(0u8, 8u8) produces values in [0, 8) — only 3 bits per byte. For a 16-byte sigma this yields 48 bits of entropy instead of 128. Both reference implementations use full-range CSPRNG: Go: crypto/rand.Read(sigma) — https://github.com/drand/kyber/blob/83f793f8/encrypt/ibe/ibe.go#L64-L65 JS: randomBytes(msg.length) — https://github.com/drand/tlock-js/blob/17d817e/src/crypto/ibe.ts#L27 Fix: replace Uniform sampling with rng.fill_bytes(&mut sigma). Adds regression test asserting sigma covers the full [0, 256) byte range.
matsuro-hadouken
force-pushed
the
fix/sigma-entropy
branch
from
8045ec1 to
77a2b33
Compare
Uniform::new(0u8, 8u8) produces values in [0, 8) only 3 bits per byte. For a 16-byte sigma this yields 48 bits of entropy instead of 128.
Both reference implementations use full-range CSPRNG:
Go: crypto/rand.Read(sigma) https://github.com/drand/kyber/blob/83f793f8/encrypt/ibe/ibe.go#L64-L65
JS: randomBytes(msg.length) https://github.com/drand/tlock-js/blob/17d817e/src/crypto/ibe.ts#L27
Fix: replace Uniform sampling with rng.fill_bytes(&mut sigma). Adds regression test asserting sigma covers the full [0, 256) byte range.