From ad470bc969c58e30192e2bcd020b970b90cab60a Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Fri, 10 Mar 2023 06:27:44 -0500 Subject: [PATCH] \#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 | 10 +++-- crypto/dalek-ff-group/src/lib.rs | 54 ++++++++++++++++++++------- crypto/dleq/Cargo.toml | 8 +++- crypto/dleq/src/cross_group/mod.rs | 34 +++++++++++++---- crypto/dleq/src/cross_group/scalar.rs | 21 ++++++----- crypto/ed448/Cargo.toml | 3 ++ crypto/ed448/src/backend.rs | 37 +++++++++++++++--- crypto/ed448/src/point.rs | 11 ++++-- crypto/multiexp/Cargo.toml | 1 + crypto/multiexp/src/lib.rs | 31 +++++++++++++-- 10 files changed, 163 insertions(+), 47 deletions(-) diff --git a/crypto/dalek-ff-group/src/field.rs b/crypto/dalek-ff-group/src/field.rs index 8c9a927d..2afe6396 100644 --- a/crypto/dalek-ff-group/src/field.rs +++ b/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 { diff --git a/crypto/dalek-ff-group/src/lib.rs b/crypto/dalek-ff-group/src/lib.rs index a05725ff..5a63375e 100644 --- a/crypto/dalek-ff-group/src/lib.rs +++ b/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)); - #[cfg(feature = "black_box")] - let res = { - use core::hint::black_box; - Choice::from(black_box(u8::from(black_box(value)))) - }; +// Feature gated due to MSRV requirements +#[cfg(feature = "black_box")] +pub(crate) fn black_box(val: T) -> T { + core::hint::black_box(val) +} + +#[cfg(not(feature = "black_box"))] +pub(crate) fn black_box(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 diff --git a/crypto/dleq/Cargo.toml b/crypto/dleq/Cargo.toml index da114e0f..0032c447 100644 --- a/crypto/dleq/Cargo.toml +++ b/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"] diff --git a/crypto/dleq/src/cross_group/mod.rs b/crypto/dleq/src/cross_group/mod.rs index e5095b45..b59a84fc 100644 --- a/crypto/dleq/src/cross_group/mod.rs +++ b/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(val: T) -> T { + core::hint::black_box(val) +} + +#[cfg(not(feature = "black_box"))] +pub(crate) fn black_box(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: &mut R) -> std::io::Result { @@ -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(); diff --git a/crypto/dleq/src/cross_group/scalar.rs b/crypto/dleq/src/cross_group/scalar.rs index fefd819d..14ef71c9 100644 --- a/crypto/dleq/src/cross_group/scalar.rs +++ b/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( 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( 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( 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(); diff --git a/crypto/ed448/Cargo.toml b/crypto/ed448/Cargo.toml index 50a98c2f..fb2da204 100644 --- a/crypto/ed448/Cargo.toml +++ b/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 = [] diff --git a/crypto/ed448/src/backend.rs b/crypto/ed448/src/backend.rs index 617dfb5e..7daae853 100644 --- a/crypto/ed448/src/backend.rs +++ b/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(val: T) -> T { + core::hint::black_box(val) +} + +#[cfg(not(feature = "black_box"))] +pub(crate) fn black_box(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 { diff --git a/crypto/ed448/src/point.rs b/crypto/ed448/src/point.rs index 25916b95..54e8caac 100644 --- a/crypto/ed448/src/point.rs +++ b/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 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 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 for Point { bits = 0; } } + other.zeroize(); res } } diff --git a/crypto/multiexp/Cargo.toml b/crypto/multiexp/Cargo.toml index a54ae0d8..24ebd399 100644 --- a/crypto/multiexp/Cargo.toml +++ b/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"] diff --git a/crypto/multiexp/src/lib.rs b/crypto/multiexp/src/lib.rs index df4da3e3..9d309637 100644 --- a/crypto/multiexp/src/lib.rs +++ b/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(val: T) -> T { + core::hint::black_box(val) +} + +#[cfg(not(feature = "black_box"))] +pub(crate) fn black_box(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(pairs: &[(G::Scalar, G)], window: u8) -> Vec> @@ -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(); }