diff --git a/alpha_0.1.3_release_notes.md b/alpha_0.1.3_release_notes.md index 57f9e97c..9f9047cf 100644 --- a/alpha_0.1.3_release_notes.md +++ b/alpha_0.1.3_release_notes.md @@ -7,3 +7,5 @@ * bug fixes to the way SHA3/SHAKE handled absorbing and squeezing a partial final byte. * Design discussions about whether core::traits::XOF (in the abstract) should allow interleaving absorb -> squeeze -> absorb (ie "absorb-after-squeeze). Outcome: absorb-after-squeeze forbidden. Could be changed in the future. +* Refactored constant-time ct_equals_bytes () to use an optimization barrier based on unsafe read_volatile / + write_volatile insead of core::mem::black_box. diff --git a/crypto/utils/src/ct.rs b/crypto/utils/src/ct.rs index 6238bf28..a3f10a0b 100644 --- a/crypto/utils/src/ct.rs +++ b/crypto/utils/src/ct.rs @@ -560,52 +560,128 @@ where } } -/// Rust doesn't guarantee that anything can truly be constant-time under all compilation targets -/// and optimization levels. The following presents the standard constant-time shape. +// --------------------------------------------------------------------------------------------- +// Byte-slice comparison helpers +// +// The core idea here is that the data-dependent state is routed through +// volatile memory accesses on every iteration. The documentation of +// `core::ptr::read_volatile` / `write_volatile` states that they "are guaranteed to not be elided +// or reordered by the compiler" and that a volatile read "will actually access memory and not +// e.g. be lowered to reusing data from a previous read". So once the accumulator has been +// volatile-written and volatile-read back, the optimiser holds no facts about its value, and in +// particular cannot introduce either of the two early exits that would otherwise be legal: +// +// * leaving the loop once the accumulator is non-zero, because the final `== 0` is already +// decided (only legal if the compiler can see that the zero test is the sole consumer), and +// * leaving the loop once the accumulator is all-ones, because further ORs cannot change it +// (legal regardless of the consumer, which is why the barrier must be *inside* the loop). +// --------------------------------------------------------------------------------------------- + +/// As a performance optimization, we compare one machine-word at a time, which we assume to be the same +/// as the size of a `usize`. +type AccWord = usize; +const ACC_BYTES: usize = size_of::(); + +/// The cor operation that implements the optimization barrier by performing a +/// `*acc |= diff`, performed as a volatile store followed by a volatile load, so that the +/// compiler retains no knowledge of the accumulator's value afterwards. See the module comment +/// above for why this is the barrier used in this file. +#[inline(always)] +fn volatile_or_assign(acc: &mut AccWord, diff: AccWord) { + // SOUNDNESS: + // * We first write_volatile to the accumulator so that the compiler does not own the written value. + // * We then read_volatile back into `*acc` so that the compiler does not own the value of `*acc` + // on exit, meaning that any read on `*acc` after this exits cannot be elided either. + // SAFETY: + // * `acc` is a `&mut AccWord`, so it is non-null, aligned for `AccWord`, and points at an + // initialised `AccWord` inside a live Rust allocation (the caller's stack frame). It is + // therefore valid for both reads and writes for the whole call, which is exactly the + // precondition of `write_volatile` and `read_volatile`. + // * The `&mut` is exclusive, so no other reference can observe or race with these accesses. + // * `AccWord` is `Copy` with no drop glue, so overwriting it and bitwise-copying it back + // out neither leaks nor double-drops anything. + // * Every bit pattern is a valid `AccWord`, so the value read back is always initialised. + unsafe { + core::ptr::write_volatile(acc, *acc | diff); + *acc = core::ptr::read_volatile(acc); + } +} + +/// Constant-time equality of two byte slices. +/// +/// The runtime depends on the *lengths* of the inputs, which are treated as public, but not on +/// their contents or on the position of any difference. Slices of different lengths compare +/// unequal immediately. pub fn ct_eq_bytes(a: &[u8], b: &[u8]) -> bool { if a.len() != b.len() { return false; } - let mut result = 0u8; - for i in 0..a.len() { - result |= core::hint::black_box(a[i] ^ b[i]); + // Optimization: evaluate it one machine-word at a time. + // Both slices now have the same length, so the two chunkings line up exactly and the + // `zip`s below never drop an element. + let (words_a, tail_a) = a.as_chunks::(); + let (words_b, tail_b) = b.as_chunks::(); + + let mut acc: AccWord = 0; + for (x, y) in words_a.iter().zip(words_b) { + volatile_or_assign(&mut acc, AccWord::from_ne_bytes(*x) ^ AccWord::from_ne_bytes(*y)); + } + for (x, y) in tail_a.iter().zip(tail_b) { + volatile_or_assign(&mut acc, AccWord::from(x ^ y)); } - result == 0 + acc == 0 } -/// Rust doesn't guarantee that anything can truly be constant-time under all compilation targets -/// and optimization levels. The following presents the standard constant-time shape. +/// Constant-time check that every byte of `a` is zero. +/// +/// The runtime depends on the length of `a`, which is treated as public, but not on its contents +/// or on the position of the first non-zero byte. Same construction as [`ct_eq_bytes`] with the +/// XOR against the second operand omitted. pub fn ct_eq_zero_bytes(a: &[u8]) -> bool { - let mut result = 0u8; - for i in 0..a.len() { - result |= core::hint::black_box(a[i]); + let (words, tail) = a.as_chunks::(); + + let mut acc: AccWord = 0; + for x in words { + volatile_or_assign(&mut acc, AccWord::from_ne_bytes(*x)); + } + for x in tail { + volatile_or_assign(&mut acc, AccWord::from(*x)); } - result == 0 + acc == 0 } -/// Copies either the contents of `a` or `b` into `out` according to `take_a` -/// and it does it in a constant-time manner without branching. +/// Copies either the contents of `a` or `b` into `out` according to `take_a`, in a constant-time +/// manner without branching on `take_a`. +/// +/// `take_a` is expanded to an all-ones / all-zeros byte mask and the copy is `(a & mask) | +/// (b & !mask)` for every byte. The mask is passed through a volatile store/load once before the +/// loop, so the optimiser does not know it is one of only two values and cannot turn the masked +/// arithmetic back into a branch or a conditional move keyed on `take_a`. This is the same +/// placement as BoringSSL's `value_barrier_w(mask)` in `constant_time_select`; the mask does not +/// change inside the loop, so there is nothing to hide per iteration. pub fn conditional_copy_bytes( a: &[u8; LEN], b: &[u8; LEN], out: &mut [u8; LEN], take_a: bool, ) { - // we want the behaviour of - // if take_a { 0xFF } else { 0x00 } - // but without using any branches that could leak timing signals - let mask: u8 = (take_a as u8) - | (take_a as u8) << 1 - | (take_a as u8) << 2 - | (take_a as u8) << 3 - | (take_a as u8) << 4 - | (take_a as u8) << 5 - | (take_a as u8) << 6 - | (take_a as u8) << 7; - + // We want the behaviour of `if take_a { 0xFF } else { 0x00 }` without a branch. `true as u8` + // is exactly `1`, and `1u8.wrapping_neg()` is `0xFF`, while `0u8.wrapping_neg()` is `0`. + let mut mask: u8 = (take_a as u8).wrapping_neg(); debug_assert_eq!(mask, if take_a { 0xFF } else { 0x00 }); + // SAFETY: + // * `&mut mask` is a reference to an initialised, aligned `u8` local on this stack frame, so + // it is valid for reads and writes for the duration of both calls. + // * The reference is exclusive; nothing else can observe `mask` during the two accesses. + // * `u8` is `Copy` with no drop glue and every bit pattern is a valid `u8`, so the value + // read back is initialised and nothing is leaked or double-dropped. + unsafe { + core::ptr::write_volatile(&mut mask, mask); + mask = core::ptr::read_volatile(&mask); + } + for i in 0..LEN { - out[i] = core::hint::black_box(a[i] & mask) | core::hint::black_box(b[i] & !mask); + out[i] = (a[i] & mask) | (b[i] & !mask); } } diff --git a/crypto/utils/tests/ct_tests.rs b/crypto/utils/tests/ct_tests.rs index 7bb8ef0f..93ac3657 100644 --- a/crypto/utils/tests/ct_tests.rs +++ b/crypto/utils/tests/ct_tests.rs @@ -831,6 +831,32 @@ mod ct_bytes_tests { assert!(!ct_eq_bytes(&a, &b)); } + /// The implementation processes the input one machine word at a time (2, 4 or 8 bytes + /// depending on the target) and then the remaining tail byte-wise. Exercise every length up + /// to several words so that, whatever the word size, a difference in any position, word or + /// tail, is detected and equal inputs of every shape compare equal. + #[test] + fn test_ct_eq_bytes_word_boundaries() { + use bouncycastle_utils::ct::ct_eq_bytes; + + for len in 0..=40usize { + let a: [u8; 40] = core::array::from_fn(|i| (i as u8).wrapping_mul(37) ^ 0x5C); + let a = &a[..len]; + let mut b = [0u8; 40]; + b[..len].copy_from_slice(a); + assert!(ct_eq_bytes(a, &b[..len]), "len {len}"); + + // flip a single bit at each position in turn + for pos in 0..len { + for bit in [0x01u8, 0x80] { + b[pos] ^= bit; + assert!(!ct_eq_bytes(a, &b[..len]), "len {len} pos {pos} bit {bit:#x}"); + b[pos] ^= bit; + } + } + } + } + #[test] fn test_ct_eq_zero_bytes() { use bouncycastle_utils::ct::ct_eq_zero_bytes; @@ -853,6 +879,25 @@ mod ct_bytes_tests { assert!(!ct_eq_zero_bytes(&buf)); } + /// Same boundary sweep as for `ct_eq_bytes`: a non-zero byte at any position of any length + /// around the machine-word boundaries must be detected. + #[test] + fn test_ct_eq_zero_bytes_word_boundaries() { + use bouncycastle_utils::ct::ct_eq_zero_bytes; + + for len in 0..=40usize { + let mut buf = [0u8; 40]; + assert!(ct_eq_zero_bytes(&buf[..len]), "len {len}"); + for pos in 0..len { + for val in [0x01u8, 0x80] { + buf[pos] = val; + assert!(!ct_eq_zero_bytes(&buf[..len]), "len {len} pos {pos} val {val:#x}"); + buf[pos] = 0; + } + } + } + } + #[test] fn test_conditional_copy_bytes() { use bouncycastle_utils::ct::conditional_copy_bytes; @@ -867,6 +912,19 @@ mod ct_bytes_tests { conditional_copy_bytes(&a, &b, &mut out, false); assert_eq!(out, [0x10, 0x11, 0x12, 0x13]); + // every byte position must follow the flag independently, including 0x00 / 0xFF values + // that would expose a broken mask, and the empty array must be a no-op + let a: [u8; 32] = core::array::from_fn(|i| (i as u8).wrapping_mul(0x11)); + let b: [u8; 32] = core::array::from_fn(|i| !(i as u8).wrapping_mul(0x11)); + let mut out = [0xEEu8; 32]; + conditional_copy_bytes(&a, &b, &mut out, true); + assert_eq!(out, a); + conditional_copy_bytes(&a, &b, &mut out, false); + assert_eq!(out, b); + let mut empty = [0u8; 0]; + conditional_copy_bytes(&[], &[], &mut empty, true); + conditional_copy_bytes(&[], &[], &mut empty, false); + // test wrong-sized array // in fact: this won't even compile, so there's nothing to test // let c = [0x20, 0x21, 0x22];