From b4a374cb9e1ff75ce614474b165b6cf501889392 Mon Sep 17 00:00:00 2001 From: Tony Arcieri Date: Thu, 20 Jun 2024 07:38:15 -0600 Subject: [PATCH] curve: use `subtle::BlackBox` optimization barrier Replaces the security mitigation added in #659 and #661 for masking-related timing variability which used an inline `black_box` using the recently added `subtle::BlackBox` newtype (see dalek-cryptography/subtle#123) Internally `BlackBox` uses a volatile read by default (i.e. same strategy which was used before) or when the `core_hint_black_box` feature of `subtle` is enabled, it uses `core::hint::black_box` (whose documentation was recently updated to reflect the nuances of potential cryptographic use, see rust-lang/rust#126703) This PR goes ahead and uses `BlackBox` for both `mask` and `underflow_mask` where previously it was only used on `underflow_mask`. The general pattern of bitwise masking inside a loop seems worrisome for the optimizer potentially inserting branches in the future. Below are godbolt inspections of the generated assembly, which are free of the `jns` instructions originally spotted in #659/#661: - 32-bit (read_volatile): https://godbolt.org/z/TKo9fqza4 - 32-bit (hint::black_box): https://godbolt.org/z/caoMxYbET - 64-bit (read_volatile): https://godbolt.org/z/PM6zKjj1f - 64-bit (hint::black_box): https://godbolt.org/z/nseaPvdWv --- curve25519-dalek/Cargo.toml | 2 +- .../src/backend/serial/u32/scalar.rs | 21 +++++++------------ .../src/backend/serial/u64/scalar.rs | 21 +++++++------------ 3 files changed, 16 insertions(+), 28 deletions(-) diff --git a/curve25519-dalek/Cargo.toml b/curve25519-dalek/Cargo.toml index 8e480ada..dc3b2b4e 100644 --- a/curve25519-dalek/Cargo.toml +++ b/curve25519-dalek/Cargo.toml @@ -51,7 +51,7 @@ ff = { version = "0.13", default-features = false, optional = true } group = { version = "0.13", default-features = false, optional = true } rand_core = { version = "0.6.4", default-features = false, optional = true } digest = { version = "0.10", default-features = false, optional = true } -subtle = { version = "2.3.0", default-features = false } +subtle = { version = "2.6.0", default-features = false } serde = { version = "1.0", default-features = false, optional = true, features = ["derive"] } zeroize = { version = "1", default-features = false, optional = true } diff --git a/curve25519-dalek/src/backend/serial/u32/scalar.rs b/curve25519-dalek/src/backend/serial/u32/scalar.rs index d3df38c8..10eadd1b 100644 --- a/curve25519-dalek/src/backend/serial/u32/scalar.rs +++ b/curve25519-dalek/src/backend/serial/u32/scalar.rs @@ -12,6 +12,7 @@ use core::fmt::Debug; use core::ops::{Index, IndexMut}; +use subtle::BlackBox; #[cfg(feature = "zeroize")] use zeroize::Zeroize; @@ -185,30 +186,24 @@ impl Scalar29 { /// Compute `a - b` (mod l). pub fn sub(a: &Scalar29, b: &Scalar29) -> Scalar29 { - // Optimization barrier to prevent compiler from inserting branch instructions - // TODO(tarcieri): find a better home (or abstraction) for this - fn black_box(value: u32) -> u32 { - // SAFETY: `u32` is a simple integer `Copy` type and `value` lives on the stack so - // a pointer to it will be valid. - unsafe { core::ptr::read_volatile(&value) } - } - let mut difference = Scalar29::ZERO; - let mask = (1u32 << 29) - 1; + let mask = BlackBox::new((1u32 << 29) - 1); // a - b let mut borrow: u32 = 0; for i in 0..9 { borrow = a[i].wrapping_sub(b[i] + (borrow >> 31)); - difference[i] = borrow & mask; + difference[i] = borrow & mask.get(); } // conditionally add l if the difference is negative - let underflow_mask = ((borrow >> 31) ^ 1).wrapping_sub(1); + let underflow_mask = BlackBox::new(((borrow >> 31) ^ 1).wrapping_sub(1)); let mut carry: u32 = 0; for i in 0..9 { - carry = (carry >> 29) + difference[i] + (constants::L[i] & black_box(underflow_mask)); - difference[i] = carry & mask; + // SECURITY: `BlackBox` prevents LLVM from inserting a `jns` conditional on x86(_64) + // which can be used to bypass this section when `underflow_mask` is zero. + carry = (carry >> 29) + difference[i] + (constants::L[i] & underflow_mask.get()); + difference[i] = carry & mask.get(); } difference diff --git a/curve25519-dalek/src/backend/serial/u64/scalar.rs b/curve25519-dalek/src/backend/serial/u64/scalar.rs index 6c0eaf79..ade16f31 100644 --- a/curve25519-dalek/src/backend/serial/u64/scalar.rs +++ b/curve25519-dalek/src/backend/serial/u64/scalar.rs @@ -13,6 +13,7 @@ use core::fmt::Debug; use core::ops::{Index, IndexMut}; +use subtle::BlackBox; #[cfg(feature = "zeroize")] use zeroize::Zeroize; @@ -174,32 +175,24 @@ impl Scalar52 { /// Compute `a - b` (mod l) pub fn sub(a: &Scalar52, b: &Scalar52) -> Scalar52 { - // Optimization barrier to prevent compiler from inserting branch instructions - // TODO(tarcieri): find a better home (or abstraction) for this - fn black_box(value: u64) -> u64 { - // SAFETY: `u64` is a simple integer `Copy` type and `value` lives on the stack so - // a pointer to it will be valid. - unsafe { core::ptr::read_volatile(&value) } - } - let mut difference = Scalar52::ZERO; - let mask = (1u64 << 52) - 1; + let mask = BlackBox::new((1u64 << 52) - 1); // a - b let mut borrow: u64 = 0; for i in 0..5 { borrow = a[i].wrapping_sub(b[i] + (borrow >> 63)); - difference[i] = borrow & mask; + difference[i] = borrow & mask.get(); } // conditionally add l if the difference is negative - let underflow_mask = ((borrow >> 63) ^ 1).wrapping_sub(1); + let underflow_mask = BlackBox::new(((borrow >> 63) ^ 1).wrapping_sub(1)); let mut carry: u64 = 0; for i in 0..5 { - // SECURITY: `black_box` prevents LLVM from inserting a `jns` conditional on x86(_64) + // SECURITY: `BlackBox` prevents LLVM from inserting a `jns` conditional on x86(_64) // which can be used to bypass this section when `underflow_mask` is zero. - carry = (carry >> 52) + difference[i] + (constants::L[i] & black_box(underflow_mask)); - difference[i] = carry & mask; + carry = (carry >> 52) + difference[i] + (constants::L[i] & underflow_mask.get()); + difference[i] = carry & mask.get(); } difference