Skip to content

Commit

Permalink
SECURITY: fix timing variability in backend/serial/u64/scalar.rs (#659)
Browse files Browse the repository at this point in the history
Timing variability of any kind is problematic when working with
potentially secret values such as elliptic curve scalars, and such
issues can potentially leak private keys and other secrets. Such a
problem was recently discovered in `curve25519-dalek`.

The `Scalar52::sub` function contained usage of a mask value inside of a
loop where LLVM saw an opportunity to insert a branch instruction
(`jns` on x86) to conditionally bypass this code section when the mask
value is set to zero, as can be seen in godbolt:

https://godbolt.org/z/PczYj7Pda

A similar problem was recently discovered in the Kyber reference
implementation:

https://groups.google.com/a/list.nist.gov/g/pqc-forum/c/hqbtIGFKIpU/m/cnE3pbueBgAJ

As discussed on that thread, one portable solution, which is also used
in this PR, is to introduce a volatile read as an optimization barrier,
which prevents the compiler from optimizing it away.

The fix can be validated in godbolt here:

https://godbolt.org/z/x8d46Yfah

The problem was discovered and the solution independently verified by
Alexander Wagner <alexander.wagner@aisec.fraunhofer.de> and
Lea Themint <lea.thiemt@tum.de> using their DATA tool:

https://github.com/Fraunhofer-AISEC/DATA

Co-authored-by: Tony Arcieri <bascule@gmail.com>
  • Loading branch information
rozbb and tarcieri authored Jun 18, 2024
1 parent 56bf398 commit 415892a
Showing 1 changed file with 11 additions and 1 deletion.
12 changes: 11 additions & 1 deletion curve25519-dalek/src/backend/serial/u64/scalar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,14 @@ 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;

Expand All @@ -188,7 +196,9 @@ impl Scalar52 {
let underflow_mask = ((borrow >> 63) ^ 1).wrapping_sub(1);
let mut carry: u64 = 0;
for i in 0..5 {
carry = (carry >> 52) + difference[i] + (constants::L[i] & underflow_mask);
// SECURITY: `black_box` 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;
}

Expand Down

0 comments on commit 415892a

Please # to comment.