curve: use subtle::BlackBox optimization barrier (#662)

Replaces the security mitigation added in #659 and #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 #659/#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

Author: tarcieri

curve25519-dalek/Cargo.toml

@@ -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 }
 

curve25519-dalek/src/backend/serial/u32/scalar.rs

@@ -12,6 +12,7 @@
 
 use core::fmt::Debug;
 use core::ops::{Index, IndexMut};
+use subtle::BlackBox;
 
 #[cfg(feature = "zeroize")]
 use zeroize::Zeroize;
@@ -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

curve25519-dalek/src/backend/serial/u64/scalar.rs

@@ -13,6 +13,7 @@
 
 use core::fmt::Debug;
 use core::ops::{Index, IndexMut};
+use subtle::BlackBox;
 
 #[cfg(feature = "zeroize")]
 use zeroize::Zeroize;
@@ -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