Skip to content

Commit

Permalink
curve: use subtle::BlackBox optimization barrier (dalek-cryptograph…
Browse files Browse the repository at this point in the history
…y#662)

Replaces the security mitigation added in dalek-cryptography#659 and dalek-cryptography#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 dalek-cryptography#659/dalek-cryptography#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
  • Loading branch information
tarcieri authored and jmwample committed Jun 26, 2024
1 parent cfd30f3 commit 676d572
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 28 deletions.
2 changes: 1 addition & 1 deletion curve25519-dalek/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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 }

Expand Down
21 changes: 8 additions & 13 deletions curve25519-dalek/src/backend/serial/u32/scalar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

use core::fmt::Debug;
use core::ops::{Index, IndexMut};
use subtle::BlackBox;

#[cfg(feature = "zeroize")]
use zeroize::Zeroize;
Expand Down Expand Up @@ -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
Expand Down
21 changes: 7 additions & 14 deletions curve25519-dalek/src/backend/serial/u64/scalar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

use core::fmt::Debug;
use core::ops::{Index, IndexMut};
use subtle::BlackBox;

#[cfg(feature = "zeroize")]
use zeroize::Zeroize;
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 676d572

Please sign in to comment.