From 921bd7ced0af512677ee8e1d5b05ba26417696b9 Mon Sep 17 00:00:00 2001 From: Tony Arcieri Date: Wed, 17 Jul 2024 12:05:46 -0600 Subject: [PATCH 1/2] curve: use `subtle::Choice` for constant-time fixes (#665) Alternative to #659/#661 and #662 which leverages `subtle::Choice` and `subtle::ConditionallySelectable` as the optimization barriers. Really the previous masking was there to conditionally add the scalar field modulus on underflow, so instead of that, we can conditionally select zero or the modulus using a `Choice` constructed from the underflow bit. --- curve25519-dalek/src/backend/serial/u32/scalar.rs | 15 +++++++-------- curve25519-dalek/src/backend/serial/u64/scalar.rs | 15 +++++++-------- 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/curve25519-dalek/src/backend/serial/u32/scalar.rs b/curve25519-dalek/src/backend/serial/u32/scalar.rs index 10eadd1b..b0f6bd09 100644 --- a/curve25519-dalek/src/backend/serial/u32/scalar.rs +++ b/curve25519-dalek/src/backend/serial/u32/scalar.rs @@ -12,7 +12,7 @@ use core::fmt::Debug; use core::ops::{Index, IndexMut}; -use subtle::BlackBox; +use subtle::{Choice, ConditionallySelectable}; #[cfg(feature = "zeroize")] use zeroize::Zeroize; @@ -187,23 +187,22 @@ impl Scalar29 { /// Compute `a - b` (mod l). pub fn sub(a: &Scalar29, b: &Scalar29) -> Scalar29 { let mut difference = Scalar29::ZERO; - let mask = BlackBox::new((1u32 << 29) - 1); + let mask = (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.get(); + difference[i] = borrow & mask; } // conditionally add l if the difference is negative - let underflow_mask = BlackBox::new(((borrow >> 31) ^ 1).wrapping_sub(1)); let mut carry: u32 = 0; for i in 0..9 { - // 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(); + let underflow = Choice::from((borrow >> 31) as u8); + let addend = u32::conditional_select(&0, &constants::L[i], underflow); + carry = (carry >> 29) + difference[i] + addend; + difference[i] = carry & mask; } difference diff --git a/curve25519-dalek/src/backend/serial/u64/scalar.rs b/curve25519-dalek/src/backend/serial/u64/scalar.rs index ade16f31..57e2d9b0 100644 --- a/curve25519-dalek/src/backend/serial/u64/scalar.rs +++ b/curve25519-dalek/src/backend/serial/u64/scalar.rs @@ -13,7 +13,7 @@ use core::fmt::Debug; use core::ops::{Index, IndexMut}; -use subtle::BlackBox; +use subtle::{Choice, ConditionallySelectable}; #[cfg(feature = "zeroize")] use zeroize::Zeroize; @@ -176,23 +176,22 @@ impl Scalar52 { /// Compute `a - b` (mod l) pub fn sub(a: &Scalar52, b: &Scalar52) -> Scalar52 { let mut difference = Scalar52::ZERO; - let mask = BlackBox::new((1u64 << 52) - 1); + let mask = (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.get(); + difference[i] = borrow & mask; } // conditionally add l if the difference is negative - let underflow_mask = BlackBox::new(((borrow >> 63) ^ 1).wrapping_sub(1)); let mut carry: u64 = 0; for i in 0..5 { - // 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] & underflow_mask.get()); - difference[i] = carry & mask.get(); + let underflow = Choice::from((borrow >> 63) as u8); + let addend = u64::conditional_select(&0, &constants::L[i], underflow); + carry = (carry >> 52) + difference[i] + addend; + difference[i] = carry & mask; } difference From 35e78b21efb61a1de148cbcf4cfe2c3d559fbec0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20W=C3=BClker?= Date: Fri, 26 Jul 2024 04:24:39 +0200 Subject: [PATCH 2/2] Enable unexpected cfgs lint (#656) Previously, the only way to configure this lint was using a build.rs file, but now it can be done using Cargo.toml as well. --- curve25519-dalek/Cargo.toml | 10 ++++++++++ curve25519-dalek/src/backend/vector/ifma/edwards.rs | 2 +- curve25519-dalek/src/backend/vector/ifma/field.rs | 2 +- curve25519-dalek/src/lib.rs | 2 -- 4 files changed, 12 insertions(+), 4 deletions(-) diff --git a/curve25519-dalek/Cargo.toml b/curve25519-dalek/Cargo.toml index dc3b2b4e..f0896ba7 100644 --- a/curve25519-dalek/Cargo.toml +++ b/curve25519-dalek/Cargo.toml @@ -71,3 +71,13 @@ group-bits = ["group", "ff/bits"] [target.'cfg(all(not(curve25519_dalek_backend = "fiat"), not(curve25519_dalek_backend = "serial"), target_arch = "x86_64"))'.dependencies] curve25519-dalek-derive = { version = "0.1", path = "../curve25519-dalek-derive" } + +[lints.rust.unexpected_cfgs] +level = "warn" +check-cfg = [ + 'cfg(allow_unused_unsafe)', + 'cfg(curve25519_dalek_backend, values("fiat", "serial", "simd"))', + 'cfg(curve25519_dalek_diagnostics, values("build"))', + 'cfg(curve25519_dalek_bits, values("32", "64"))', + 'cfg(nightly)', +] diff --git a/curve25519-dalek/src/backend/vector/ifma/edwards.rs b/curve25519-dalek/src/backend/vector/ifma/edwards.rs index f8605fe5..3de7030a 100644 --- a/curve25519-dalek/src/backend/vector/ifma/edwards.rs +++ b/curve25519-dalek/src/backend/vector/ifma/edwards.rs @@ -249,7 +249,7 @@ impl<'a> From<&'a edwards::EdwardsPoint> for NafLookupTable8 { } } -#[cfg(target_feature = "avx512ifma,avx512vl")] +#[cfg(all(target_feature = "avx512ifma", target_feature = "avx512vl"))] #[cfg(test)] mod test { use super::*; diff --git a/curve25519-dalek/src/backend/vector/ifma/field.rs b/curve25519-dalek/src/backend/vector/ifma/field.rs index fa8ce2dd..ebdfba1b 100644 --- a/curve25519-dalek/src/backend/vector/ifma/field.rs +++ b/curve25519-dalek/src/backend/vector/ifma/field.rs @@ -629,7 +629,7 @@ impl<'a, 'b> Mul<&'b F51x4Reduced> for &'a F51x4Reduced { } } -#[cfg(target_feature = "avx512ifma,avx512vl")] +#[cfg(all(target_feature = "avx512ifma", target_feature = "avx512vl"))] #[cfg(test)] mod test { use super::*; diff --git a/curve25519-dalek/src/lib.rs b/curve25519-dalek/src/lib.rs index fecfe888..d8666453 100644 --- a/curve25519-dalek/src/lib.rs +++ b/curve25519-dalek/src/lib.rs @@ -42,8 +42,6 @@ unused_lifetimes, unused_qualifications )] -// Requires MSRV 1.77 as it does not allow build.rs gating -#![allow(unexpected_cfgs)] //------------------------------------------------------------------------ // External dependencies: