From 9480844b8decc0f2f863c001457b39203140bb4e Mon Sep 17 00:00:00 2001 From: Tony Arcieri Date: Mon, 30 Sep 2019 11:51:57 -0700 Subject: [PATCH] Switch from `clear_on_drop` to `zeroize` (fixes #281) `zeroize` is WASM-friendly as it has no dependencies on C compilers. Instead uses Rust's own volatile write semantics and compiler fences to ensure zeroization is not elided by the compiler. --- Cargo.toml | 6 +++--- src/backend/serial/curve_models/mod.rs | 19 +++++++++++++++++ src/backend/serial/scalar_mul/straus.rs | 7 ++++--- src/backend/serial/u32/field.rs | 8 +++++++ src/backend/serial/u32/scalar.rs | 8 +++++++ src/backend/serial/u64/field.rs | 8 +++++++ src/backend/serial/u64/scalar.rs | 8 +++++++ src/backend/vector/avx2/field.rs | 10 +++++++-- src/backend/vector/scalar_mul/straus.rs | 6 +++--- src/lib.rs | 2 +- src/scalar.rs | 11 ++++------ src/window.rs | 28 ++++++++++--------------- 12 files changed, 85 insertions(+), 36 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 563998d0d..19eb85536 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -40,16 +40,16 @@ harness = false rand_core = { version = "0.3.0", default-features = false } byteorder = { version = "^1.2.3", default-features = false, features = ["i128"] } digest = { version = "0.8", default-features = false } -clear_on_drop = "=0.2.3" subtle = { version = "2", default-features = false } serde = { version = "1.0", default-features = false, optional = true } packed_simd = { version = "0.3.0", features = ["into_bits"], optional = true } +zeroize = { version = "1.0.0", default-features = false } [features] -nightly = ["subtle/nightly", "clear_on_drop/nightly"] +nightly = ["subtle/nightly"] default = ["std", "u64_backend"] std = ["alloc", "subtle/std", "rand_core/std"] -alloc = [] +alloc = ["zeroize/alloc"] # The u32 backend uses u32s with u64 products. u32_backend = [] diff --git a/src/backend/serial/curve_models/mod.rs b/src/backend/serial/curve_models/mod.rs index c0b573025..5d5850f9a 100644 --- a/src/backend/serial/curve_models/mod.rs +++ b/src/backend/serial/curve_models/mod.rs @@ -128,6 +128,8 @@ use core::ops::{Add, Neg, Sub}; use subtle::Choice; use subtle::ConditionallySelectable; +use zeroize::Zeroize; + use constants; use edwards::EdwardsPoint; @@ -182,6 +184,14 @@ pub struct AffineNielsPoint { pub xy2d: FieldElement, } +impl Zeroize for AffineNielsPoint { + fn zeroize(&mut self) { + self.y_plus_x.zeroize(); + self.y_minus_x.zeroize(); + self.xy2d.zeroize(); + } +} + /// A pre-computed point on the \\( \mathbb P\^3 \\) model for the /// curve, represented as \\((Y+X, Y-X, Z, 2dXY)\\) in "Niels coordinates". /// @@ -195,6 +205,15 @@ pub struct ProjectiveNielsPoint { pub T2d: FieldElement, } +impl Zeroize for ProjectiveNielsPoint { + fn zeroize(&mut self) { + self.Y_plus_X.zeroize(); + self.Y_minus_X.zeroize(); + self.Z.zeroize(); + self.T2d.zeroize(); + } +} + // ------------------------------------------------------------------------ // Constructors // ------------------------------------------------------------------------ diff --git a/src/backend/serial/scalar_mul/straus.rs b/src/backend/serial/scalar_mul/straus.rs index 9a97b093f..b63e16287 100644 --- a/src/backend/serial/scalar_mul/straus.rs +++ b/src/backend/serial/scalar_mul/straus.rs @@ -106,7 +106,7 @@ impl MultiscalarMul for Straus { J: IntoIterator, J::Item: Borrow, { - use clear_on_drop::ClearOnDrop; + use zeroize::Zeroizing; use backend::serial::curve_models::ProjectiveNielsPoint; use window::LookupTable; @@ -119,12 +119,12 @@ impl MultiscalarMul for Straus { // This puts the scalar digits into a heap-allocated Vec. // To ensure that these are erased, pass ownership of the Vec into a - // ClearOnDrop wrapper. + // Zeroizing wrapper. let scalar_digits_vec: Vec<_> = scalars .into_iter() .map(|s| s.borrow().to_radix_16()) .collect(); - let scalar_digits = ClearOnDrop::new(scalar_digits_vec); + let scalar_digits = Zeroizing::new(scalar_digits_vec); let mut Q = EdwardsPoint::identity(); for j in (0..64).rev() { @@ -137,6 +137,7 @@ impl MultiscalarMul for Straus { Q = (&Q + &R_i).to_extended(); } } + Q } } diff --git a/src/backend/serial/u32/field.rs b/src/backend/serial/u32/field.rs index ba57604d7..603ed3fc3 100644 --- a/src/backend/serial/u32/field.rs +++ b/src/backend/serial/u32/field.rs @@ -24,6 +24,8 @@ use core::ops::{Sub, SubAssign}; use subtle::Choice; use subtle::ConditionallySelectable; +use zeroize::Zeroize; + /// A `FieldElement2625` represents an element of the field /// \\( \mathbb Z / (2\^{255} - 19)\\). /// @@ -55,6 +57,12 @@ impl Debug for FieldElement2625 { } } +impl Zeroize for FieldElement2625 { + fn zeroize(&mut self) { + self.0.zeroize(); + } +} + impl<'b> AddAssign<&'b FieldElement2625> for FieldElement2625 { fn add_assign(&mut self, _rhs: &'b FieldElement2625) { for i in 0..10 { diff --git a/src/backend/serial/u32/scalar.rs b/src/backend/serial/u32/scalar.rs index 8dce8a38b..8dd54bd29 100644 --- a/src/backend/serial/u32/scalar.rs +++ b/src/backend/serial/u32/scalar.rs @@ -13,6 +13,8 @@ use core::fmt::Debug; use core::ops::{Index, IndexMut}; +use zeroize::Zeroize; + use constants; /// The `Scalar29` struct represents an element in ℤ/lℤ as 9 29-bit limbs @@ -25,6 +27,12 @@ impl Debug for Scalar29 { } } +impl Zeroize for Scalar29 { + fn zeroize(&mut self) { + self.0.zeroize(); + } +} + impl Index for Scalar29 { type Output = u32; fn index(&self, _index: usize) -> &u32 { diff --git a/src/backend/serial/u64/field.rs b/src/backend/serial/u64/field.rs index e1054c562..1cb7778b9 100644 --- a/src/backend/serial/u64/field.rs +++ b/src/backend/serial/u64/field.rs @@ -20,6 +20,8 @@ use core::ops::{Sub, SubAssign}; use subtle::Choice; use subtle::ConditionallySelectable; +use zeroize::Zeroize; + /// A `FieldElement51` represents an element of the field /// \\( \mathbb Z / (2\^{255} - 19)\\). /// @@ -44,6 +46,12 @@ impl Debug for FieldElement51 { } } +impl Zeroize for FieldElement51 { + fn zeroize(&mut self) { + self.0.zeroize(); + } +} + impl<'b> AddAssign<&'b FieldElement51> for FieldElement51 { fn add_assign(&mut self, _rhs: &'b FieldElement51) { for i in 0..5 { diff --git a/src/backend/serial/u64/scalar.rs b/src/backend/serial/u64/scalar.rs index f0d8edb92..97069ad3d 100644 --- a/src/backend/serial/u64/scalar.rs +++ b/src/backend/serial/u64/scalar.rs @@ -14,6 +14,8 @@ use core::fmt::Debug; use core::ops::{Index, IndexMut}; +use zeroize::Zeroize; + use constants; /// The `Scalar52` struct represents an element in @@ -27,6 +29,12 @@ impl Debug for Scalar52 { } } +impl Zeroize for Scalar52 { + fn zeroize(&mut self) { + self.0.zeroize(); + } +} + impl Index for Scalar52 { type Output = u64; fn index(&self, _index: usize) -> &u64 { diff --git a/src/backend/vector/avx2/field.rs b/src/backend/vector/avx2/field.rs index a375253d7..d1a84a0aa 100644 --- a/src/backend/vector/avx2/field.rs +++ b/src/backend/vector/avx2/field.rs @@ -41,6 +41,7 @@ const D_LANES64: u8 = 0b11_00_00_00; use core::ops::{Add, Mul, Neg}; use packed_simd::{i32x8, u32x8, u64x4, IntoBits}; +use zeroize::Zeroize; use backend::vector::avx2::constants::{P_TIMES_16_HI, P_TIMES_16_LO, P_TIMES_2_HI, P_TIMES_2_LO}; use backend::serial::u64::field::FieldElement51; @@ -449,7 +450,7 @@ impl FieldElement2625x4 { // hi (c(x3), c(y3), c(x2), c(y2), c(z3), c(w3), c(z2), c(w2)) // -> (c(x1), c(y1), c(x2), c(y2), c(z1), c(w1), c(z2), c(w2)) // - // which is exactly the vector of carryins for + // which is exactly the vector of carryins for // // ( x2, y2, x3, y3, z2, w2, z3, w3). // @@ -462,7 +463,7 @@ impl FieldElement2625x4 { let mut v = self.0; - let c10 = rotated_carryout(v[0]); + let c10 = rotated_carryout(v[0]); v[0] = (v[0] & masks) + combine(u32x8::splat(0), c10); let c32 = rotated_carryout(v[1]); @@ -873,6 +874,11 @@ impl<'a, 'b> Mul<&'b FieldElement2625x4> for &'a FieldElement2625x4 { } } +impl Zeroize for FieldElement2625x4 { + fn zeroize(&mut self) { + self.0.zeroize(); + } +} #[cfg(test)] mod test { diff --git a/src/backend/vector/scalar_mul/straus.rs b/src/backend/vector/scalar_mul/straus.rs index 0e49d3fbb..7206cf39f 100644 --- a/src/backend/vector/scalar_mul/straus.rs +++ b/src/backend/vector/scalar_mul/straus.rs @@ -12,7 +12,7 @@ use core::borrow::Borrow; -use clear_on_drop::ClearOnDrop; +use zeroize::Zeroizing; use backend::vector::{CachedPoint, ExtendedPoint}; use edwards::EdwardsPoint; @@ -54,8 +54,8 @@ impl MultiscalarMul for Straus { .into_iter() .map(|s| s.borrow().to_radix_16()) .collect(); - // Pass ownership to a ClearOnDrop wrapper - let scalar_digits = ClearOnDrop::new(scalar_digits_vec); + // Pass ownership to a `Zeroizing` wrapper + let scalar_digits = Zeroizing::new(scalar_digits_vec); let mut Q = ExtendedPoint::identity(); for j in (0..64).rev() { diff --git a/src/lib.rs b/src/lib.rs index d1db2ef18..0216628e7 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -42,11 +42,11 @@ extern crate std; extern crate packed_simd; extern crate byteorder; -extern crate clear_on_drop; pub extern crate digest; extern crate rand_core; #[cfg(test)] extern crate rand_os; +extern crate zeroize; // Used for traits related to constant-time code. extern crate subtle; diff --git a/src/scalar.rs b/src/scalar.rs index 6f9d52b41..dab83e5ff 100644 --- a/src/scalar.rs +++ b/src/scalar.rs @@ -354,7 +354,7 @@ impl<'a> Neg for &'a Scalar { fn neg(self) -> Scalar { let self_R = UnpackedScalar::mul_internal(&self.unpack(), &constants::R); let self_mod_l = UnpackedScalar::montgomery_reduce(&self_R); - UnpackedScalar::sub(&UnpackedScalar::zero(), &self_mod_l).pack() + UnpackedScalar::sub(&UnpackedScalar::zero(), &self_mod_l).pack() } } @@ -760,18 +760,15 @@ impl Scalar { // externally, but there's no corresponding distinction for // field elements. - use clear_on_drop::ClearOnDrop; - use clear_on_drop::clear::ZeroSafe; - // Mark UnpackedScalars as zeroable. - unsafe impl ZeroSafe for UnpackedScalar {} + use zeroize::Zeroizing; let n = inputs.len(); let one: UnpackedScalar = Scalar::one().unpack().to_montgomery(); - // Wrap the scratch storage in a ClearOnDrop to wipe it when + // Place scratch storage in a Zeroizing wrapper to wipe it when // we pass out of scope. let scratch_vec = vec![one; n]; - let mut scratch = ClearOnDrop::new(scratch_vec); + let mut scratch = Zeroizing::new(scratch_vec); // Keep an accumulator of all of the previous products let mut acc = Scalar::one().unpack().to_montgomery(); diff --git a/src/window.rs b/src/window.rs index ec4c17434..3b0142216 100644 --- a/src/window.rs +++ b/src/window.rs @@ -25,6 +25,8 @@ use edwards::EdwardsPoint; use backend::serial::curve_models::ProjectiveNielsPoint; use backend::serial::curve_models::AffineNielsPoint; +use zeroize::Zeroize; + /// A lookup table of precomputed multiples of a point \\(P\\), used to /// compute \\( xP \\) for \\( -8 \leq x \leq 8 \\). /// @@ -40,23 +42,6 @@ use backend::serial::curve_models::AffineNielsPoint; #[derive(Copy, Clone)] pub struct LookupTable(pub(crate) [T; 8]); -use clear_on_drop::clear::ZeroSafe; - -/// This type isn't actually zeroable (all zero bytes are not valid -/// points), but we want to be able to use `clear_on_drop` to erase slices -/// of `LookupTable`. -/// -/// Since the `ZeroSafe` trait is only used by `clear_on_drop`, the only -/// situation where this would be a problem is if code attempted to use -/// a `ClearOnDrop` to erase a `LookupTable` and then used the table -/// afterwards. -/// -/// Normally this is not a problem, since the table's storage is usually -/// dropped too. -/// -/// XXX is this a good compromise? -unsafe impl ZeroSafe for LookupTable {} - impl LookupTable where T: Identity + ConditionallySelectable + ConditionallyNegatable, @@ -120,6 +105,15 @@ impl<'a> From<&'a EdwardsPoint> for LookupTable { } } +impl Zeroize for LookupTable +where + T: Copy + Default + Zeroize +{ + fn zeroize(&mut self) { + self.0.zeroize(); + } +} + /// Holds odd multiples 1A, 3A, ..., 15A of a point A. #[derive(Copy, Clone)] pub(crate) struct NafLookupTable5(pub(crate) [T; 8]);