SECURITY: fix timing variability in backend/serial/u64/scalar.rs (#659)

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>
This commit is contained in:
Michael Rosenberg 2024-06-18 19:49:31 +02:00 committed by GitHub
parent 56bf398d0c
commit 415892acf1
No known key found for this signature in database
GPG key ID: B5690EEEBB952194

View file

@ -174,6 +174,14 @@ impl Scalar52 {
/// Compute `a - b` (mod l) /// Compute `a - b` (mod l)
pub fn sub(a: &Scalar52, b: &Scalar52) -> Scalar52 { 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 mut difference = Scalar52::ZERO;
let mask = (1u64 << 52) - 1; let mask = (1u64 << 52) - 1;
@ -188,7 +196,9 @@ impl Scalar52 {
let underflow_mask = ((borrow >> 63) ^ 1).wrapping_sub(1); let underflow_mask = ((borrow >> 63) ^ 1).wrapping_sub(1);
let mut carry: u64 = 0; let mut carry: u64 = 0;
for i in 0..5 { 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; difference[i] = carry & mask;
} }