Skip to content

Commit

Permalink
SECURITY: fix timing variability in backend/serial/u32/scalar.rs (dal…
Browse files Browse the repository at this point in the history
…ek-cryptography#661)

Similar security fix to dalek-cryptography#659, but for the 32-bit backend. See that PR
for more information about the problem. Relevant compiler outputs (thanks to @tarcieri):

Without fix
https://godbolt.org/z/zvaWxzvqv
Notice the `jns` ("jump if not sign") instruction on line 106.

With fix
https://godbolt.org/z/jc9j7eb8E
  • Loading branch information
tarcieri authored and yihau committed Jun 27, 2024
1 parent 29e5c29 commit d10985b
Showing 1 changed file with 9 additions and 1 deletion.
10 changes: 9 additions & 1 deletion src/backend/serial/u32/scalar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,14 @@ 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;

Expand All @@ -194,7 +202,7 @@ impl Scalar29 {
let underflow_mask = ((borrow >> 31) ^ 1).wrapping_sub(1);
let mut carry: u32 = 0;
for i in 0..9 {
carry = (carry >> 29) + difference[i] + (constants::L[i] & underflow_mask);
carry = (carry >> 29) + difference[i] + (constants::L[i] & black_box(underflow_mask));
difference[i] = carry & mask;
}

Expand Down

0 comments on commit d10985b

Please sign in to comment.