SECURITY: fix timing variability in backend/serial/u64/scalar.rs (dal…

…ek-cryptography#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 <[email protected]> and
Lea Themint <[email protected]> using their DATA tool:

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

Co-authored-by: Tony Arcieri <[email protected]>

src/backend/serial/u64/scalar.rs

@@ -170,6 +170,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;
 
@@ -184,7 +192,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;
         }