\#242 Expand usage of black_box/zeroize

This commit greatly expands the usage of black_box/zeroize on bits, as it
originally should have. It is likely overkill, leading to less efficient
code generation, yet does its best to be comprehensive where comprehensiveness
is extremely annoying to achieve.

In the future, this usage of black_box may be desirable to move to its own
crate.

Credit to @AaronFeickert for identifying the original commit was incomplete.

crypto/dalek-ff-group/src/field.rs

@@ -1,5 +1,6 @@
-use core::ops::{Add, AddAssign, Sub, SubAssign, Neg, Mul, MulAssign};
+use core::ops::{DerefMut, Add, AddAssign, Sub, SubAssign, Neg, Mul, MulAssign};
 
+use zeroize::Zeroize;
 use rand_core::RngCore;
 
 use subtle::{
@@ -11,7 +12,7 @@ use crypto_bigint::{Integer, Encoding, U256, U512};
 
 use group::ff::{Field, PrimeField, FieldBits, PrimeFieldBits};
 
-use crate::{constant_time, math, from_uint};
+use crate::{u8_from_bool, constant_time, math, from_uint};
 
 // 2^255 - 19
 // Uses saturating_sub because checked_sub isn't available at compile time
@@ -200,10 +201,11 @@ impl FieldElement {
 
     let mut res = FieldElement::one();
     let mut bits = 0;
-    for (i, bit) in other.to_le_bits().iter().rev().enumerate() {
+    for (i, mut bit) in other.to_le_bits().iter_mut().rev().enumerate() {
       bits <<= 1;
-      let bit = u8::from(*bit);
+      let mut bit = u8_from_bool(bit.deref_mut());
       bits |= bit;
+      bit.zeroize();
 
       if ((i + 1) % 4) == 0 {
         if i != 3 {

crypto/dalek-ff-group/src/lib.rs

@@ -3,7 +3,7 @@
 
 use core::{
   borrow::Borrow,
-  ops::{Deref, Add, AddAssign, Sub, SubAssign, Neg, Mul, MulAssign},
+  ops::{Deref, DerefMut, Add, AddAssign, Sub, SubAssign, Neg, Mul, MulAssign},
   iter::{Iterator, Sum},
   hash::{Hash, Hasher},
 };
@@ -41,18 +41,34 @@ use group::{
 
 pub mod field;
 
-// Convert a boolean to a Choice in a *presumably* constant time manner
-fn choice(value: bool) -> Choice {
-  #[cfg(not(feature = "black_box"))]
-  let res = Choice::from(u8::from(value));
+// Feature gated due to MSRV requirements
 #[cfg(feature = "black_box")]
-  let res = {
-    use core::hint::black_box;
-    Choice::from(black_box(u8::from(black_box(value))))
-  };
+pub(crate) fn black_box<T>(val: T) -> T {
+  core::hint::black_box(val)
+}
+
+#[cfg(not(feature = "black_box"))]
+pub(crate) fn black_box<T>(val: T) -> T {
+  val
+}
+
+fn u8_from_bool(bit_ref: &mut bool) -> u8 {
+  let bit_ref = black_box(bit_ref);
+
+  let mut bit = black_box(*bit_ref);
+  let res = black_box(bit as u8);
+  bit.zeroize();
+  debug_assert!((res | 1) == 1);
+
+  bit_ref.zeroize();
   res
 }
 
+// Convert a boolean to a Choice in a *presumably* constant time manner
+fn choice(mut value: bool) -> Choice {
+  Choice::from(u8_from_bool(&mut value))
+}
+
 macro_rules! deref_borrow {
   ($Source: ident, $Target: ident) => {
     impl Deref for $Source {
@@ -202,10 +218,11 @@ impl Scalar {
 
     let mut res = Scalar::one();
     let mut bits = 0;
-    for (i, bit) in other.to_le_bits().iter().rev().enumerate() {
+    for (i, mut bit) in other.to_le_bits().iter_mut().rev().enumerate() {
       bits <<= 1;
-      let bit = u8::from(*bit);
+      let mut bit = u8_from_bool(bit.deref_mut());
       bits |= bit;
+      bit.zeroize();
 
       if ((i + 1) % 4) == 0 {
         if i != 3 {
@@ -288,7 +305,18 @@ impl PrimeField for Scalar {
   // The number of leading zero bits in the little-endian bit representation of (modulus - 1)
   const S: u32 = 2;
   fn is_odd(&self) -> Choice {
-    choice(self.to_le_bits()[0])
+    // This is probably overkill? Yet it's better safe than sorry since this is a complete
+    // decomposition of the scalar
+    let mut bits = self.to_le_bits();
+    let res = choice(bits[0]);
+    // This shouldn't need mut since it should be a mutable reference
+    // Per the bitvec docs, writing through a derefence requires mut, writing through one of its
+    // methods does not
+    // We do not use one of its methods to ensure we write via zeroize
+    for mut bit in bits.iter_mut() {
+      bit.deref_mut().zeroize();
+    }
+    res
   }
   fn multiplicative_generator() -> Self {
     // This was calculated with the method from the ff crate docs

crypto/dleq/Cargo.toml

@@ -39,8 +39,14 @@ transcript = { package = "flexible-transcript", path = "../transcript", features
 [features]
 std = []
 serialize = ["std"]
-experimental = ["std", "thiserror", "multiexp"]
+
+# Needed for cross-group DLEqs
+black_box = []
 secure_capacity_difference = []
+experimental = ["std", "thiserror", "multiexp"]
 
 # Only applies to experimental, yet is default to ensure security
+# experimental doesn't mandate it itself in case two curves with extreme
+# capacity differences are desired to be used together, in which case the user
+# must specify experimental without default features
 default = ["secure_capacity_difference"]

crypto/dleq/src/cross_group/mod.rs

@@ -1,4 +1,6 @@
-use core::ops::Deref;
+use core::ops::{Deref, DerefMut};
+#[cfg(feature = "serialize")]
+use std::io::{Read, Write};
 
 use thiserror::Error;
 
@@ -27,8 +29,28 @@ pub(crate) mod aos;
 mod bits;
 use bits::{BitSignature, Bits};
 
-#[cfg(feature = "serialize")]
-use std::io::{Read, Write};
+// Feature gated due to MSRV requirements
+#[cfg(feature = "black_box")]
+pub(crate) fn black_box<T>(val: T) -> T {
+  core::hint::black_box(val)
+}
+
+#[cfg(not(feature = "black_box"))]
+pub(crate) fn black_box<T>(val: T) -> T {
+  val
+}
+
+fn u8_from_bool(bit_ref: &mut bool) -> u8 {
+  let bit_ref = black_box(bit_ref);
+
+  let mut bit = black_box(*bit_ref);
+  let res = black_box(bit as u8);
+  bit.zeroize();
+  debug_assert!((res | 1) == 1);
+
+  bit_ref.zeroize();
+  res
+}
 
 #[cfg(feature = "serialize")]
 pub(crate) fn read_point<R: Read, G: PrimeGroup>(r: &mut R) -> std::io::Result<G> {
@@ -224,15 +246,13 @@ where
     let mut these_bits: u8 = 0;
     // Needed to zero out the bits
     #[allow(unused_assignments)]
-    for (i, mut raw_bit) in raw_bits.iter_mut().enumerate() {
+    for (i, mut bit) in raw_bits.iter_mut().enumerate() {
       if i == capacity {
         break;
       }
 
-      let mut bit = u8::from(*raw_bit);
-      *raw_bit = false;
-
       // Accumulate this bit
+      let mut bit = u8_from_bool(bit.deref_mut());
       these_bits |= bit << (i % bits_per_group);
       bit.zeroize();
 

crypto/dleq/src/cross_group/scalar.rs

@@ -1,21 +1,26 @@
+use core::ops::DerefMut;
+
 use ff::PrimeFieldBits;
 
 use zeroize::Zeroize;
 
+use crate::cross_group::u8_from_bool;
+
 /// Convert a uniform scalar into one usable on both fields, clearing the top bits as needed.
 pub fn scalar_normalize<F0: PrimeFieldBits + Zeroize, F1: PrimeFieldBits>(
   mut scalar: F0,
 ) -> (F0, F1) {
   let mutual_capacity = F0::CAPACITY.min(F1::CAPACITY);
 
-  // The security of a mutual key is the security of the lower field. Accordingly, this bans a
-  // difference of more than 4 bits
+  // A mutual key is only as secure as its weakest group
+  // Accordingly, this bans a capacity difference of more than 4 bits to prevent a curve generally
+  // offering n-bits of security from being forced into a situation with much fewer bits
   #[cfg(feature = "secure_capacity_difference")]
-  assert!((F0::CAPACITY.max(F1::CAPACITY) - mutual_capacity) < 4);
+  assert!((F0::CAPACITY.max(F1::CAPACITY) - mutual_capacity) <= 4);
 
   let mut res1 = F0::zero();
   let mut res2 = F1::zero();
-  // Uses the bit view API to ensure a consistent endianess
+  // Uses the bits API to ensure a consistent endianess
   let mut bits = scalar.to_le_bits();
   scalar.zeroize();
   // Convert it to big endian
@@ -24,9 +29,9 @@ pub fn scalar_normalize<F0: PrimeFieldBits + Zeroize, F1: PrimeFieldBits>(
   let mut skip = bits.len() - usize::try_from(mutual_capacity).unwrap();
   // Needed to zero out the bits
   #[allow(unused_assignments)]
-  for mut raw_bit in bits.iter_mut() {
+  for mut bit in bits.iter_mut() {
     if skip > 0 {
-      *raw_bit = false;
+      bit.deref_mut().zeroize();
       skip -= 1;
       continue;
     }
@@ -34,9 +39,7 @@ pub fn scalar_normalize<F0: PrimeFieldBits + Zeroize, F1: PrimeFieldBits>(
     res1 = res1.double();
     res2 = res2.double();
 
-    let mut bit = u8::from(*raw_bit);
-    *raw_bit = false;
-
+    let mut bit = u8_from_bool(bit.deref_mut());
     res1 += F0::from(bit.into());
     res2 += F1::from(bit.into());
     bit.zeroize();

crypto/ed448/Cargo.toml

@@ -32,3 +32,6 @@ dalek-ff-group = { path = "../dalek-ff-group", version = "^0.1.2" }
 hex = "0.4"
 
 ff-group-tests = { path = "../ff-group-tests" }
+
+[features]
+black_box = []

crypto/ed448/src/backend.rs

@@ -1,12 +1,36 @@
+use zeroize::Zeroize;
+
+// Feature gated due to MSRV requirements
+#[cfg(feature = "black_box")]
+pub(crate) fn black_box<T>(val: T) -> T {
+  core::hint::black_box(val)
+}
+
+#[cfg(not(feature = "black_box"))]
+pub(crate) fn black_box<T>(val: T) -> T {
+  val
+}
+
+pub(crate) fn u8_from_bool(bit_ref: &mut bool) -> u8 {
+  let bit_ref = black_box(bit_ref);
+
+  let mut bit = black_box(*bit_ref);
+  let res = black_box(bit as u8);
+  bit.zeroize();
+  debug_assert!((res | 1) == 1);
+
+  bit_ref.zeroize();
+  res
+}
+
 #[doc(hidden)]
 #[macro_export]
 macro_rules! field {
   ($FieldName: ident, $MODULUS: ident, $WIDE_MODULUS: ident, $NUM_BITS: literal) => {
-    use core::ops::{Add, AddAssign, Neg, Sub, SubAssign, Mul, MulAssign};
-
-    use rand_core::RngCore;
+    use core::ops::{DerefMut, Add, AddAssign, Neg, Sub, SubAssign, Mul, MulAssign};
 
     use subtle::{Choice, CtOption, ConstantTimeEq, ConstantTimeLess, ConditionallySelectable};
+    use rand_core::RngCore;
 
     use generic_array::{typenum::U57, GenericArray};
     use crypto_bigint::{Integer, Encoding};
@@ -18,6 +42,8 @@ macro_rules! field {
     use dalek_ff_group::{from_wrapper, math_op};
     use dalek_ff_group::{constant_time, from_uint, math};
 
+    use $crate::backend::u8_from_bool;
+
     fn reduce(x: U1024) -> U512 {
       U512::from_le_slice(&x.reduce(&$WIDE_MODULUS).unwrap().to_le_bytes()[.. 64])
     }
@@ -59,10 +85,11 @@ macro_rules! field {
 
         let mut res = Self(U512::ONE);
         let mut bits = 0;
-        for (i, bit) in other.to_le_bits().iter().rev().enumerate() {
+        for (i, mut bit) in other.to_le_bits().iter_mut().rev().enumerate() {
           bits <<= 1;
-          let bit = u8::from(*bit);
+          let mut bit = u8_from_bool(bit.deref_mut());
           bits |= bit;
+          bit.zeroize();
 
           if ((i + 1) % 4) == 0 {
             if i != 3 {

crypto/ed448/src/point.rs

@@ -1,5 +1,5 @@
 use core::{
-  ops::{Add, AddAssign, Neg, Sub, SubAssign, Mul, MulAssign},
+  ops::{DerefMut, Add, AddAssign, Neg, Sub, SubAssign, Mul, MulAssign},
   iter::Sum,
 };
 
@@ -19,6 +19,7 @@ use group::{
 };
 
 use crate::{
+  backend::u8_from_bool,
   scalar::{Scalar, MODULUS as SCALAR_MODULUS},
   field::{FieldElement, MODULUS as FIELD_MODULUS, Q_4},
 };
@@ -218,7 +219,7 @@ impl<'a> Sum<&'a Point> for Point {
 
 impl Mul<Scalar> for Point {
   type Output = Point;
-  fn mul(self, other: Scalar) -> Point {
+  fn mul(self, mut other: Scalar) -> Point {
     // Precompute the optimal amount that's a multiple of 2
     let mut table = [Point::identity(); 16];
     table[1] = self;
@@ -228,10 +229,11 @@ impl Mul<Scalar> for Point {
 
     let mut res = Self::identity();
     let mut bits = 0;
-    for (i, bit) in other.to_le_bits().iter().rev().enumerate() {
+    for (i, mut bit) in other.to_le_bits().iter_mut().rev().enumerate() {
       bits <<= 1;
-      let bit = u8::from(*bit);
+      let mut bit = u8_from_bool(bit.deref_mut());
       bits |= bit;
+      bit.zeroize();
 
       if ((i + 1) % 4) == 0 {
         if i != 3 {
@@ -243,6 +245,7 @@ impl Mul<Scalar> for Point {
         bits = 0;
       }
     }
+    other.zeroize();
     res
   }
 }

crypto/multiexp/Cargo.toml

@@ -27,4 +27,5 @@ k256 = { version = "0.12", features = ["bits"] }
 dalek-ff-group = { path = "../dalek-ff-group" }
 
 [features]
+black_box = []
 batch = ["rand_core"]

crypto/multiexp/src/lib.rs

@@ -1,5 +1,7 @@
 #![cfg_attr(docsrs, feature(doc_auto_cfg))]
 
+use core::ops::DerefMut;
+
 use zeroize::Zeroize;
 
 use ff::PrimeFieldBits;
@@ -19,6 +21,29 @@ pub use batch::BatchVerifier;
 #[cfg(test)]
 mod tests;
 
+// Feature gated due to MSRV requirements
+#[cfg(feature = "black_box")]
+pub(crate) fn black_box<T>(val: T) -> T {
+  core::hint::black_box(val)
+}
+
+#[cfg(not(feature = "black_box"))]
+pub(crate) fn black_box<T>(val: T) -> T {
+  val
+}
+
+fn u8_from_bool(bit_ref: &mut bool) -> u8 {
+  let bit_ref = black_box(bit_ref);
+
+  let mut bit = black_box(*bit_ref);
+  let res = black_box(bit as u8);
+  bit.zeroize();
+  debug_assert!((res | 1) == 1);
+
+  bit_ref.zeroize();
+  res
+}
+
 // Convert scalars to `window`-sized bit groups, as needed to index a table
 // This algorithm works for `window <= 8`
 pub(crate) fn prep_bits<G: Group>(pairs: &[(G::Scalar, G)], window: u8) -> Vec<Vec<u8>>
@@ -33,10 +58,8 @@ where
     let mut bits = pair.0.to_le_bits();
     groupings.push(vec![0; (bits.len() + (w_usize - 1)) / w_usize]);
 
-    for (i, mut raw_bit) in bits.iter_mut().enumerate() {
-      let mut bit = u8::from(*raw_bit);
-      (*raw_bit).zeroize();
-
+    for (i, mut bit) in bits.iter_mut().enumerate() {
+      let mut bit = u8_from_bool(bit.deref_mut());
       groupings[p][i / w_usize] |= bit << (i % w_usize);
       bit.zeroize();
     }