From 6fe93564cd8b83809f7d73174f85ce974b993895 Mon Sep 17 00:00:00 2001 From: Henry de Valence Date: Wed, 5 Jun 2019 20:45:07 -0700 Subject: [PATCH 1/4] Add a more comprehensive random multiscalar test. This exercises the constant- and variable- time code at large sizes, to hit every path of Straus/Pippenger. --- src/edwards.rs | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/src/edwards.rs b/src/edwards.rs index e43adb4..4dc9953 100644 --- a/src/edwards.rs +++ b/src/edwards.rs @@ -1255,6 +1255,71 @@ mod test { assert!(P1.compress().to_bytes() == P2.compress().to_bytes()); } + // A single iteration of a consistency check for MSM. + fn multiscalar_consistency_iter(n: usize) { + use core::iter; + let mut rng = rand::thread_rng(); + + // Construct random coefficients x0, ..., x_{n-1}, + // followed by some extra hardcoded ones. + let xs = (0..n) + .map(|_| Scalar::random(&mut rng)) + .collect::>(); + let check = xs.iter() + .map(|xi| xi * xi) + .sum::(); + + // Construct points G_i = x_i * B + let Gs = xs.iter() + .map(|xi| xi * &constants::ED25519_BASEPOINT_TABLE) + .collect::>(); + + // Compute H1 = (consttime) + let H1 = EdwardsPoint::multiscalar_mul(&xs, &Gs); + // Compute H2 = (vartime) + let H2 = EdwardsPoint::vartime_multiscalar_mul(&xs, &Gs); + // Compute H3 = = sum(xi^2) * B + let H3 = &check * &constants::ED25519_BASEPOINT_TABLE; + + assert_eq!(H1, H3); + assert_eq!(H2, H3); + } + + // Use different multiscalar sizes to hit different internal + // parameters. + + #[test] + fn multiscalar_consistency_n_100() { + let iters = 50; + for _ in 0..iters { + multiscalar_consistency_iter(100); + } + } + + #[test] + fn multiscalar_consistency_n_250() { + let iters = 50; + for _ in 0..iters { + multiscalar_consistency_iter(250); + } + } + + #[test] + fn multiscalar_consistency_n_500() { + let iters = 50; + for _ in 0..iters { + multiscalar_consistency_iter(500); + } + } + + #[test] + fn multiscalar_consistency_n_1000() { + let iters = 50; + for _ in 0..iters { + multiscalar_consistency_iter(1000); + } + } + #[test] fn vartime_precomputed_vs_nonprecomputed_multiscalar() { let mut rng = rand::thread_rng(); From 5f1d73bca01804214590441beb110d4da5b6b1d7 Mon Sep 17 00:00:00 2001 From: Henry de Valence Date: Wed, 5 Jun 2019 20:59:07 -0700 Subject: [PATCH 2/4] Fix a negate-with-overflow edgecase by widening before computation. This fixes a bug in the Pippenger implementation reported by Fernando Krell and diagnosed by Oleg Andreev. The problem is that at the largest problem sizes (using w=8), the signed digits fill the value range of an i8, and so doing computation on them to calculate the bucket index can hit an overflow. This was not caught in CI because the test suite didn't check all problem sizes; tests for these sizes which expose this bug were added in the previous commit. --- src/backend/serial/scalar_mul/pippenger.rs | 3 ++- src/backend/vector/scalar_mul/pippenger.rs | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/backend/serial/scalar_mul/pippenger.rs b/src/backend/serial/scalar_mul/pippenger.rs index 89ea723..5a028fd 100644 --- a/src/backend/serial/scalar_mul/pippenger.rs +++ b/src/backend/serial/scalar_mul/pippenger.rs @@ -125,7 +125,8 @@ impl VartimeMultiscalarMul for Pippenger { // Note: if we add support for precomputed lookup tables, // we'll be adding/subtracting point premultiplied by `digits[i]` to buckets[0]. for (digits, pt) in scalars_points.iter() { - let digit = digits[digit_index]; + // Widen digit so that we don't run into edge cases when w=8. + let digit = digits[digit_index] as i16; if digit > 0 { let b = (digit - 1) as usize; buckets[b] = (&buckets[b] + pt).to_extended(); diff --git a/src/backend/vector/scalar_mul/pippenger.rs b/src/backend/vector/scalar_mul/pippenger.rs index 0053e67..21d2d37 100644 --- a/src/backend/vector/scalar_mul/pippenger.rs +++ b/src/backend/vector/scalar_mul/pippenger.rs @@ -82,7 +82,8 @@ impl VartimeMultiscalarMul for Pippenger { // Note: if we add support for precomputed lookup tables, // we'll be adding/subtractiong point premultiplied by `digits[i]` to buckets[0]. for (digits, pt) in scalars_points.iter() { - let digit = digits[digit_index]; + // Widen digit so that we don't run into edge cases when w=8. + let digit = digits[digit_index] as i16; if digit > 0 { let b = (digit - 1) as usize; buckets[b] = &buckets[b] + pt; From 389d2bc9e2da8cc4196aa064b8fd82b27f725107 Mon Sep 17 00:00:00 2001 From: Henry de Valence Date: Wed, 5 Jun 2019 22:00:43 -0700 Subject: [PATCH 3/4] Ensure Pippenger works on manually-constructed extremal values. When using Scalar::from_bits to manually create unreduced Scalars (e.g., X/Ed25519 keys with specified bit patterns), it's possible to construct Scalar values that range up to 2^255-1. These shouldn't ever end up in a vartime multiscalar mul call anyways, because it doesn't handle secret data, but it is technically allowed by the type system and should be handled. When w=8, these can generate terminal carries that can't be folded into the last digit, but this can be handled by folding them into an extra digit instead. --- src/backend/serial/scalar_mul/pippenger.rs | 4 +- src/backend/vector/scalar_mul/pippenger.rs | 4 +- src/edwards.rs | 2 + src/scalar.rs | 100 +++++++++++++-------- 4 files changed, 71 insertions(+), 39 deletions(-) diff --git a/src/backend/serial/scalar_mul/pippenger.rs b/src/backend/serial/scalar_mul/pippenger.rs index 5a028fd..0cae2a1 100644 --- a/src/backend/serial/scalar_mul/pippenger.rs +++ b/src/backend/serial/scalar_mul/pippenger.rs @@ -88,14 +88,14 @@ impl VartimeMultiscalarMul for Pippenger { }; let max_digit: usize = 1 << w; - let digits_count: usize = (256 + w - 1) / w; // == ceil(256/w) + let digits_count: usize = Scalar::to_radix_2w_size_hint(w); let buckets_count: usize = max_digit / 2; // digits are signed+centered hence 2^w/2, excluding 0-th bucket // Collect optimized scalars and points in buffers for repeated access // (scanning the whole set per digit position). let scalars = scalars .into_iter() - .map(|s| s.borrow().to_radix_2w(w).0); + .map(|s| s.borrow().to_radix_2w(w)); let points = points .into_iter() diff --git a/src/backend/vector/scalar_mul/pippenger.rs b/src/backend/vector/scalar_mul/pippenger.rs index 21d2d37..f834a66 100644 --- a/src/backend/vector/scalar_mul/pippenger.rs +++ b/src/backend/vector/scalar_mul/pippenger.rs @@ -45,14 +45,14 @@ impl VartimeMultiscalarMul for Pippenger { }; let max_digit: usize = 1 << w; - let digits_count: usize = (256 + w - 1) / w; // == ceil(256/w) + let digits_count: usize = Scalar::to_radix_2w_size_hint(w); let buckets_count: usize = max_digit / 2; // digits are signed+centered hence 2^w/2, excluding 0-th bucket // Collect optimized scalars and points in a buffer for repeated access // (scanning the whole collection per each digit position). let scalars = scalars .into_iter() - .map(|s| s.borrow().to_radix_2w(w).0); + .map(|s| s.borrow().to_radix_2w(w)); let points = points .into_iter() diff --git a/src/edwards.rs b/src/edwards.rs index 4dc9953..c60069c 100644 --- a/src/edwards.rs +++ b/src/edwards.rs @@ -1264,6 +1264,8 @@ mod test { // followed by some extra hardcoded ones. let xs = (0..n) .map(|_| Scalar::random(&mut rng)) + // The largest scalar allowed by the type system, 2^255-1 + .chain(iter::once(Scalar::from_bits([0xff; 32]))) .collect::>(); let check = xs.iter() .map(|xi| xi * xi) diff --git a/src/scalar.rs b/src/scalar.rs index 00ebad8..575fefe 100644 --- a/src/scalar.rs +++ b/src/scalar.rs @@ -961,6 +961,24 @@ impl Scalar { output } + /// Returns a size hint indicating how many entries of the return + /// value of `to_radix_2w` are nonzero. + pub(crate) fn to_radix_2w_size_hint(w: usize) -> usize { + debug_assert!(w >= 6); + debug_assert!(w <= 8); + + let digits_count = match w { + 6 => (256 + w - 1)/w as usize, + 7 => (256 + w - 1)/w as usize, + // See comment in to_radix_2w on handling the terminal carry. + 8 => (256 + w - 1)/w + 1 as usize, + _ => panic!("invalid radix parameter"), + }; + + debug_assert!(digits_count <= 43); + digits_count + } + /// Creates a representation of a Scalar in radix 64, 128 or 256 for use with the Pippenger algorithm. /// For lower radix, use `to_radix_16`, which is used by the Straus multi-scalar multiplication. /// Higher radixes are not supported to save cache space. Radix 256 is near-optimal even for very @@ -979,13 +997,10 @@ impl Scalar { /// $$ /// with \\(-2\^w/2 \leq a_i < 2\^w/2\\) for \\(0 \leq i < (n-1)\\) and \\(-2\^w/2 \leq a_{n-1} \leq 2\^w/2\\). /// - pub(crate) fn to_radix_2w(&self, w: usize) -> ([i8; 43], usize) { + pub(crate) fn to_radix_2w(&self, w: usize) -> [i8; 43] { debug_assert!(w >= 6); debug_assert!(w <= 8); - let digits_count = (256 + w - 1)/w as usize; - debug_assert!(digits_count <= 43); - use byteorder::{ByteOrder, LittleEndian}; // Scalar formatted as four `u64`s with carry bit packed into the highest bit. @@ -997,6 +1012,7 @@ impl Scalar { let mut carry = 0u64; let mut digits = [0i8; 43]; + let digits_count = (256 + w - 1)/w as usize; for i in 0..digits_count { // Construct a buffer of bits of the scalar, starting at `bit_offset`. let bit_offset = i*w; @@ -1017,22 +1033,25 @@ impl Scalar { // Read the actual coefficient value from the window let coef = carry + (bit_buf & window_mask); // coef = [0, 2^r) - // Recenter coefficients from [0,2^r) to [-2^r/2, 2^r/2) + // Recenter coefficients from [0,2^w) to [-2^w/2, 2^w/2) carry = (coef + (radix/2) as u64) >> w; digits[i] = ((coef as i64) - (carry << w) as i64) as i8; } - // Apply the resulting carry to the last digit - // Since the highest bit of the 256-bit integer is 0, - // the last coefficient would always be in the lower half _inclusive_, - // so the carry in the end can be 1 iff the word equals 2^r/2. - // Since ±2^r/2 values are valid, to avoid adding an extra word, - // we allow the last word to touch the value 2^r/2. - // XXX: make sure tests cover this case, so the carry is non-zero and this line matters. - // Maybe it never happens to be non-zero for r=6/7/8?... - digits[digits_count-1] += (carry << w) as i8; + // When w < 8, we can fold the final carry onto the last digit d, + // because d < 2^w/2 so d + carry*2^w = d + 1*2^w < 2^(w+1) < 2^8. + // + // When w = 8, we can't fit carry*2^w into an i8. This should + // not happen anyways, because the final carry will be 0 for + // reduced scalars, but the Scalar invariant allows 255-bit scalars. + // To handle this, we expand the size_hint by 1 when w=8, + // and accumulate the final carry onto another digit. + match w { + 8 => digits[digits_count] += carry as i8, + _ => digits[digits_count-1] += (carry << w) as i8, + } - (digits, digits_count) + digits } /// Unpack this `Scalar` to an `UnpackedScalar` for faster arithmetic. @@ -1512,32 +1531,43 @@ mod test { } } + fn test_pippenger_radix_iter(scalar: Scalar, w: usize) { + let digits_count = Scalar::to_radix_2w_size_hint(w); + let digits = scalar.to_radix_2w(w); + + let radix = Scalar::from((1< Date: Wed, 5 Jun 2019 22:59:39 -0700 Subject: [PATCH 4/4] Ensure NAF works on manually-constructed extremal values. The NAF computation can generate a 1 in the last digit (only) when s = 2^255-1, so someone who manually constructed the value s = 2^255-1 and fed it into a NAF-using computation could generate an incorrect result. Some version of this bug has been present from the beginning of the library, but it has no security content, because the NAF computations are not applied to secret data, and the error occurs only on one value which is not constructed by any client caller. --- src/backend/serial/scalar_mul/precomputed_straus.rs | 2 +- src/backend/serial/scalar_mul/straus.rs | 2 +- src/backend/serial/scalar_mul/vartime_double_base.rs | 2 +- src/backend/vector/scalar_mul/precomputed_straus.rs | 2 +- src/backend/vector/scalar_mul/straus.rs | 2 +- src/backend/vector/scalar_mul/vartime_double_base.rs | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/backend/serial/scalar_mul/precomputed_straus.rs b/src/backend/serial/scalar_mul/precomputed_straus.rs index 4019b14..9c66c9a 100644 --- a/src/backend/serial/scalar_mul/precomputed_straus.rs +++ b/src/backend/serial/scalar_mul/precomputed_straus.rs @@ -85,7 +85,7 @@ impl VartimePrecomputedMultiscalarMul for VartimePrecomputedStraus { // nonzero NAF coefficient, but since we might have a lot of // them to search, it's not clear it's worthwhile to check. let mut S = ProjectivePoint::identity(); - for j in (0..255).rev() { + for j in (0..256).rev() { let mut R: CompletedPoint = S.double(); for i in 0..dp { diff --git a/src/backend/serial/scalar_mul/straus.rs b/src/backend/serial/scalar_mul/straus.rs index 4053ea3..862cf25 100644 --- a/src/backend/serial/scalar_mul/straus.rs +++ b/src/backend/serial/scalar_mul/straus.rs @@ -179,7 +179,7 @@ impl VartimeMultiscalarMul for Straus { let mut r = ProjectivePoint::identity(); - for i in (0..255).rev() { + for i in (0..256).rev() { let mut t: CompletedPoint = r.double(); for (naf, lookup_table) in nafs.iter().zip(lookup_tables.iter()) { diff --git a/src/backend/serial/scalar_mul/vartime_double_base.rs b/src/backend/serial/scalar_mul/vartime_double_base.rs index d95151f..42f6bd7 100644 --- a/src/backend/serial/scalar_mul/vartime_double_base.rs +++ b/src/backend/serial/scalar_mul/vartime_double_base.rs @@ -23,7 +23,7 @@ pub fn mul(a: &Scalar, A: &EdwardsPoint, b: &Scalar) -> EdwardsPoint { // Find starting index let mut i: usize = 255; - for j in (0..255).rev() { + for j in (0..256).rev() { i = j; if a_naf[i] != 0 || b_naf[i] != 0 { break; diff --git a/src/backend/vector/scalar_mul/precomputed_straus.rs b/src/backend/vector/scalar_mul/precomputed_straus.rs index 49d1be4..cc1404a 100644 --- a/src/backend/vector/scalar_mul/precomputed_straus.rs +++ b/src/backend/vector/scalar_mul/precomputed_straus.rs @@ -84,7 +84,7 @@ impl VartimePrecomputedMultiscalarMul for VartimePrecomputedStraus { // nonzero NAF coefficient, but since we might have a lot of // them to search, it's not clear it's worthwhile to check. let mut R = ExtendedPoint::identity(); - for j in (0..255).rev() { + for j in (0..256).rev() { R = R.double(); for i in 0..dp { diff --git a/src/backend/vector/scalar_mul/straus.rs b/src/backend/vector/scalar_mul/straus.rs index 506693d..285a5fd 100644 --- a/src/backend/vector/scalar_mul/straus.rs +++ b/src/backend/vector/scalar_mul/straus.rs @@ -94,7 +94,7 @@ impl VartimeMultiscalarMul for Straus { let mut Q = ExtendedPoint::identity(); - for i in (0..255).rev() { + for i in (0..256).rev() { Q = Q.double(); for (naf, lookup_table) in nafs.iter().zip(lookup_tables.iter()) { diff --git a/src/backend/vector/scalar_mul/vartime_double_base.rs b/src/backend/vector/scalar_mul/vartime_double_base.rs index 44d92f7..ff641cc 100644 --- a/src/backend/vector/scalar_mul/vartime_double_base.rs +++ b/src/backend/vector/scalar_mul/vartime_double_base.rs @@ -23,7 +23,7 @@ pub fn mul(a: &Scalar, A: &EdwardsPoint, b: &Scalar) -> EdwardsPoint { // Find starting index let mut i: usize = 255; - for j in (0..255).rev() { + for j in (0..256).rev() { i = j; if a_naf[i] != 0 || b_naf[i] != 0 { break;