An easy-to-host PDS on the ATProtocol, MacOS. Grandma-approved.
fix(crypto): address PR review issues for MM-93
- Replace branching GF(2^8) reduction with branchless mask:
(a as i8 >> 7) as u8 selects 0x1b without branching on secret bits
- Add upper-bound index check (> 3) in combine_shares; silent wrong
reconstruction on out-of-range indices was not caught before
- Switch fill_bytes -> try_fill_bytes so RNG failure returns
CryptoError::SecretSharing instead of panicking
- Remove #[derive(Clone)] from ShamirShare — no call site uses it and
Clone on a secret-bearing type is inconsistent with P256Keypair
- Expand combine_with_index_zero_fails to test both argument positions
- Add combine_with_index_out_of_range_fails test (index: 4)
- Expand gf_mul_is_commutative to exhaustive 256×256 check
- Update gf_mul/gf_inv doc comments: describe branchless reduction,
fix "repeated squaring" -> "binary exponentiation (square-and-multiply)",
add standard -> GF(2^8) Lagrange derivation step
authored by
Tangled
d845272e
26db06ee
+59
-37
+59
-37
crates/crypto/src/shamir.rs
+59
-37
crates/crypto/src/shamir.rs
···
6
6
/// A single Shamir secret share for a 32-byte secret.
7
7
///
8
8
/// `data` contains secret material and is zeroized on drop.
9
-
#[derive(Clone)]
10
9
pub struct ShamirShare {
11
10
/// Share index: 1, 2, or 3. Not secret.
12
11
pub index: u8,
···
24
23
/// p(x) = x^8 + x^4 + x^3 + x + 1 (0x11b).
25
24
pub fn split_secret(secret: &[u8; 32]) -> Result<[ShamirShare; 3], CryptoError> {
26
25
let mut coeffs = Zeroizing::new([0u8; 32]);
27
-
OsRng.fill_bytes(coeffs.as_mut());
26
+
OsRng
27
+
.try_fill_bytes(coeffs.as_mut())
28
+
.map_err(|e| CryptoError::SecretSharing(format!("OS RNG unavailable: {e}")))?;
28
29
29
30
let mut s1 = Zeroizing::new([0u8; 32]);
30
31
let mut s2 = Zeroizing::new([0u8; 32]);
···
33
34
// Polynomial: f(x) = secret[i] + coeffs[i]·x in GF(2^8).
34
35
// f(0) = secret[i]. Shares are f(1), f(2), f(3).
35
36
//
36
-
// The secret byte is in the first argument of gf_mul so that the
37
-
// second argument (the constant share index) controls branching —
38
-
// no timing side-channel on the secret value.
37
+
// Secret bytes are in the first argument of gf_mul. The polynomial
38
+
// reduction inside gf_mul is branchless (mask-based), so bit patterns
39
+
// of the secret are not observable through branch timing. The `if b & 1`
40
+
// branch in gf_mul is on the public share index.
39
41
for i in 0..32 {
40
42
let s = secret[i];
41
43
let a = coeffs[i];
···
58
60
///
59
61
/// # Algorithm
60
62
///
61
-
/// Lagrange interpolation at x=0 in GF(2^8):
63
+
/// Lagrange interpolation at x=0 in GF(2^8). For two points (x_a, y_a)
64
+
/// and (x_b, y_b) on the degree-1 polynomial f(x) = secret + coeff·x:
62
65
///
63
66
/// ```text
64
-
/// f(0) = y_a · (x_b / (x_a ⊕ x_b)) ⊕ y_b · (x_a / (x_a ⊕ x_b))
67
+
/// Standard Lagrange: f(0) = y_a · (0−x_b)/(x_a−x_b) + y_b · (0−x_a)/(x_b−x_a)
68
+
/// In GF(2^8) (−x = x): f(0) = y_a · x_b/(x_a⊕x_b) ⊕ y_b · x_a/(x_a⊕x_b)
65
69
/// ```
66
-
///
67
-
/// In GF(2^8): subtraction = XOR, and (0 − x) = x.
68
70
pub fn combine_shares(
69
71
a: &ShamirShare,
70
72
b: &ShamirShare,
71
73
) -> Result<Zeroizing<[u8; 32]>, CryptoError> {
72
-
if a.index == 0 || b.index == 0 {
74
+
if a.index == 0 || a.index > 3 || b.index == 0 || b.index > 3 {
73
75
return Err(CryptoError::SecretReconstruction(
74
76
"share index must be in [1, 3]".into(),
75
77
));
···
82
84
83
85
let x_a = a.index;
84
86
let x_b = b.index;
85
-
// x_a ⊕ x_b is guaranteed nonzero since x_a ≠ x_b.
87
+
// x_a ⊕ x_b is guaranteed nonzero since x_a ≠ x_b; gf_div cannot fail here.
86
88
let denom = gf_add(x_a, x_b);
87
-
// Lagrange basis values at x=0. These are derived from public indices,
88
-
// so leaking their value through timing (as the `b` arg in gf_mul) is fine.
89
+
// Lagrange basis values at x=0, derived from public indices — timing is fine.
89
90
let l_a = gf_div(x_b, denom)?;
90
91
let l_b = gf_div(x_a, denom)?;
91
92
92
93
let mut secret = Zeroizing::new([0u8; 32]);
93
94
for i in 0..32 {
94
-
// Secret share bytes are in the first (non-branching) argument of gf_mul.
95
+
// Secret share bytes are in the first argument of gf_mul (branchless reduction).
95
96
secret[i] = gf_add(gf_mul(a.data[i], l_a), gf_mul(b.data[i], l_b));
96
97
}
97
98
Ok(secret)
···
107
108
/// GF(2^8) multiplication using the AES irreducible polynomial
108
109
/// p(x) = x^8 + x^4 + x^3 + x + 1 (represented as 0x11b; low byte 0x1b).
109
110
///
110
-
/// Use the "Russian peasant" (double-and-add) algorithm:
111
-
///
112
-
/// For each of the 8 bits of `b` (LSB first):
113
-
/// 1. If the current bit of `b` is 1, XOR `a` into `result`.
114
-
/// 2. Check whether `a`'s high bit (0x80) is set before shifting.
115
-
/// 3. Left-shift `a` by 1.
116
-
/// 4. If the high bit was set, XOR `a` with 0x1b to reduce modulo p(x).
117
-
/// 5. Right-shift `b` by 1.
111
+
/// Uses the double-and-add algorithm (Russian peasant), processing 8 bits of
112
+
/// `b` LSB-first. The polynomial reduction is **branchless**: an arithmetic
113
+
/// right-shift produces a mask that selects the reduction constant without
114
+
/// branching on bits of `a`. The `if b & 1` branch is on `b` (the public
115
+
/// share index or Lagrange coefficient), not on secret data.
118
116
///
119
-
/// The loop runs exactly 8 times (constant-time with respect to `a`,
120
-
/// which is where secret values are placed by convention).
117
+
/// By convention, secret values are always passed as the **first argument**
118
+
/// so that branching on `b` never leaks secret bit patterns.
121
119
fn gf_mul(mut a: u8, mut b: u8) -> u8 {
122
120
let mut result = 0u8;
123
121
for _ in 0..8 {
124
122
if b & 1 != 0 {
125
123
result ^= a;
126
124
}
127
-
// GF(2^8) "doubling": left-shift + conditional reduction.
128
-
// This is NOT gf_add(a, a) — that would always be 0 in characteristic 2.
129
-
let high_bit = (a & 0x80) != 0;
130
-
a <<= 1;
131
-
if high_bit {
132
-
a ^= 0x1b; // reduce mod x^8 + x^4 + x^3 + x + 1
133
-
}
125
+
// Branchless GF(2^8) doubling. Arithmetic right-shift of the signed
126
+
// reinterpretation of `a` fills with the high bit, producing 0xFF when
127
+
// bit 7 is set and 0x00 otherwise. The mask selects the reduction
128
+
// constant (0x1b = low byte of 0x11b) without branching on `a`.
129
+
let mask = (a as i8 >> 7) as u8;
130
+
a = (a << 1) ^ (mask & 0x1b);
134
131
b >>= 1;
135
132
}
136
133
result
···
139
136
/// Multiplicative inverse in GF(2^8) via Fermat's little theorem:
140
137
/// a^(2^8 − 2) = a^254 = a^(−1) (since |GF(2^8)*| = 255).
141
138
///
142
-
/// Computed via repeated squaring on top of `gf_mul`.
139
+
/// Computed via binary exponentiation (square-and-multiply) on top of `gf_mul`.
143
140
fn gf_inv(a: u8) -> Result<u8, CryptoError> {
144
141
if a == 0 {
145
142
return Err(CryptoError::SecretReconstruction(
146
143
"GF(2^8) inverse of 0 is undefined".into(),
147
144
));
148
145
}
149
-
// a^254 by repeated squaring (254 = 0b11111110).
146
+
// a^254 by binary exponentiation (254 = 0b11111110).
150
147
let mut result = 1u8;
151
148
let mut base = a;
152
149
let mut exp = 254u8;
···
290
287
data: Zeroizing::new([0u8; 32]),
291
288
};
292
289
let shares = split_secret(&[0x42_u8; 32]).unwrap();
293
-
let result = combine_shares(&zero_share, &shares[0]);
294
-
assert!(matches!(result, Err(CryptoError::SecretReconstruction(_))));
290
+
// Both argument positions must be guarded.
291
+
assert!(matches!(
292
+
combine_shares(&zero_share, &shares[0]),
293
+
Err(CryptoError::SecretReconstruction(_))
294
+
));
295
+
assert!(matches!(
296
+
combine_shares(&shares[0], &zero_share),
297
+
Err(CryptoError::SecretReconstruction(_))
298
+
));
299
+
}
300
+
301
+
#[test]
302
+
fn combine_with_index_out_of_range_fails() {
303
+
let bad_share = ShamirShare {
304
+
index: 4,
305
+
data: Zeroizing::new([0u8; 32]),
306
+
};
307
+
let shares = split_secret(&[0x42_u8; 32]).unwrap();
308
+
// Both argument positions must be guarded.
309
+
assert!(matches!(
310
+
combine_shares(&bad_share, &shares[0]),
311
+
Err(CryptoError::SecretReconstruction(_))
312
+
));
313
+
assert!(matches!(
314
+
combine_shares(&shares[0], &bad_share),
315
+
Err(CryptoError::SecretReconstruction(_))
316
+
));
295
317
}
296
318
297
319
// ── GF(2^8) arithmetic invariants ────────────────────────────────────────
···
314
336
315
337
#[test]
316
338
fn gf_mul_is_commutative() {
317
-
for a in [0x00_u8, 0x01, 0x02, 0x03, 0x0A, 0x53, 0xCA, 0xFF] {
318
-
for b in [0x00_u8, 0x01, 0x02, 0x03, 0x0A, 0x53, 0xCA, 0xFF] {
339
+
for a in 0_u8..=255 {
340
+
for b in 0_u8..=255 {
319
341
assert_eq!(
320
342
gf_mul(a, b),
321
343
gf_mul(b, a),