From 9c95edeb5a9ae58b99fa0c6aaf99346fa766ae8c Mon Sep 17 00:00:00 2001 From: Sam Dealy Date: Fri, 3 Feb 2023 13:22:29 -0800 Subject: [PATCH 1/2] Make find_record constant time --- fog/view/enclave/impl/src/e_tx_out_store.rs | 36 ++++++++++----------- fog/view/enclave/impl/src/lib.rs | 1 + 2 files changed, 19 insertions(+), 18 deletions(-) diff --git a/fog/view/enclave/impl/src/e_tx_out_store.rs b/fog/view/enclave/impl/src/e_tx_out_store.rs index 18d072beb2..214d3feeb8 100644 --- a/fog/view/enclave/impl/src/e_tx_out_store.rs +++ b/fog/view/enclave/impl/src/e_tx_out_store.rs @@ -7,7 +7,7 @@ use alloc::vec; use aligned_cmov::{ - subtle::{Choice, ConstantTimeEq}, + subtle::{Choice, ConditionallySelectable, ConstantTimeEq, ConstantTimeGreater}, typenum::{Unsigned, U1024, U16, U240, U4096, U64}, A8Bytes, CMov, }; @@ -184,24 +184,24 @@ impl> ETxOutStore ); } - // TOOO: Per https://github.com/mobilecoinfoundation/mobilecoin/issues/2965, use a - // a constant time comparison function to always copy the same number of bytes. - // NOTE: As of right now, this code is not constant time and therefore - // blocks the v5 release. - // Code to implement: - // ``` - // const LENGTH_TO_COPY: usize = core::cmp::min(FIXED_CIPHERTEXT_LENGTH, - // ValueSize::USIZE - 1); - // (&result.ciphertext[..LENGTH_TO_COPY]).copy_from_slice(&value[1..LENGTH_TO_COPY]); - // ``` - let data_end = ValueSize::USIZE - value[0] as usize; - let payload = &value[1..data_end]; - // Use this instead of payload.len() because the slice `len` method isn't - // guaranteed to be constant time. - let payload_length = data_end - 1; - result.ciphertext[0..payload_length].copy_from_slice(payload); - result.payload_length = payload_length as u32; + // Always copy the same length to ensure constant time. Note that we are not + // copying the payload length because this is variable and would + // compromise obliviousness. + const LENGTH_TO_COPY: usize = core::cmp::min(FIXED_CIPHERTEXT_LENGTH, ValueSize::USIZE - 1); + result.ciphertext[..LENGTH_TO_COPY].copy_from_slice(&value[1..(LENGTH_TO_COPY + 1)]); + result.payload_length = ct_min( + FIXED_CIPHERTEXT_LENGTH as u64, + (ValueSize::USIZE - 1 - (value[0] as usize)) as u64, + ) as u32; result } } + +fn ct_min(mut lhs: T, rhs: T) -> T +where + T: ConditionallySelectable + ConstantTimeGreater, +{ + lhs.conditional_assign(&rhs, lhs.ct_gt(&rhs)); + lhs +} diff --git a/fog/view/enclave/impl/src/lib.rs b/fog/view/enclave/impl/src/lib.rs index fd9087e2de..602933437d 100644 --- a/fog/view/enclave/impl/src/lib.rs +++ b/fog/view/enclave/impl/src/lib.rs @@ -4,6 +4,7 @@ #![no_std] #![allow(clippy::result_large_err)] +#![feature(const_cmp)] extern crate alloc; From 26ca2c39cb8a6d138b1928acacd3f35e8e0175f3 Mon Sep 17 00:00:00 2001 From: Sam Dealy Date: Wed, 8 Feb 2023 15:20:50 -0800 Subject: [PATCH 2/2] Implement Chris's suggestion --- Cargo.lock | 1 + fog/view/enclave/impl/Cargo.toml | 2 ++ fog/view/enclave/impl/src/e_tx_out_store.rs | 26 +++++++-------------- fog/view/enclave/impl/src/lib.rs | 1 - fog/view/enclave/trusted/Cargo.lock | 1 + 5 files changed, 13 insertions(+), 18 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b2e56a60f5..b3a4d9a083 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4604,6 +4604,7 @@ dependencies = [ "mc-sgx-compat", "mc-sgx-report-cache-api", "mc-util-serial", + "static_assertions", ] [[package]] diff --git a/fog/view/enclave/impl/Cargo.toml b/fog/view/enclave/impl/Cargo.toml index b32307ca2c..3f59a5c19a 100644 --- a/fog/view/enclave/impl/Cargo.toml +++ b/fog/view/enclave/impl/Cargo.toml @@ -8,6 +8,8 @@ license = "GPL-3.0" [dependencies] aes-gcm = "0.10.1" aligned-cmov = "2.2" +static_assertions = "1.1.0" + mc-attest-ake = { path = "../../../../attest/ake", default-features = false } mc-attest-core = { path = "../../../../attest/core", default-features = false } mc-attest-enclave-api = { path = "../../../../attest/enclave-api", default-features = false } diff --git a/fog/view/enclave/impl/src/e_tx_out_store.rs b/fog/view/enclave/impl/src/e_tx_out_store.rs index 214d3feeb8..5c541e902f 100644 --- a/fog/view/enclave/impl/src/e_tx_out_store.rs +++ b/fog/view/enclave/impl/src/e_tx_out_store.rs @@ -7,7 +7,7 @@ use alloc::vec; use aligned_cmov::{ - subtle::{Choice, ConditionallySelectable, ConstantTimeEq, ConstantTimeGreater}, + subtle::{Choice, ConstantTimeEq}, typenum::{Unsigned, U1024, U16, U240, U4096, U64}, A8Bytes, CMov, }; @@ -184,24 +184,16 @@ impl> ETxOutStore ); } - // Always copy the same length to ensure constant time. Note that we are not - // copying the payload length because this is variable and would - // compromise obliviousness. - const LENGTH_TO_COPY: usize = core::cmp::min(FIXED_CIPHERTEXT_LENGTH, ValueSize::USIZE - 1); + // To preserve constant time execution, we always copy `ValueSize::USIZE - 1` + // bytes. To ensure the copy doesn't panic, assert that the length to + // copy is less than the maximum length that ciphertext can be, which is + // `FIXED_CIPHERTEXT_LENGTH`. + const LENGTH_TO_COPY: usize = ValueSize::USIZE - 1; + static_assertions::const_assert!(LENGTH_TO_COPY < FIXED_CIPHERTEXT_LENGTH); + result.ciphertext[..LENGTH_TO_COPY].copy_from_slice(&value[1..(LENGTH_TO_COPY + 1)]); - result.payload_length = ct_min( - FIXED_CIPHERTEXT_LENGTH as u64, - (ValueSize::USIZE - 1 - (value[0] as usize)) as u64, - ) as u32; + result.payload_length = (ValueSize::USIZE - 1 - (value[0] as usize)) as u32; result } } - -fn ct_min(mut lhs: T, rhs: T) -> T -where - T: ConditionallySelectable + ConstantTimeGreater, -{ - lhs.conditional_assign(&rhs, lhs.ct_gt(&rhs)); - lhs -} diff --git a/fog/view/enclave/impl/src/lib.rs b/fog/view/enclave/impl/src/lib.rs index 602933437d..fd9087e2de 100644 --- a/fog/view/enclave/impl/src/lib.rs +++ b/fog/view/enclave/impl/src/lib.rs @@ -4,7 +4,6 @@ #![no_std] #![allow(clippy::result_large_err)] -#![feature(const_cmp)] extern crate alloc; diff --git a/fog/view/enclave/trusted/Cargo.lock b/fog/view/enclave/trusted/Cargo.lock index a7407d7ade..3a0db087ee 100644 --- a/fog/view/enclave/trusted/Cargo.lock +++ b/fog/view/enclave/trusted/Cargo.lock @@ -1302,6 +1302,7 @@ dependencies = [ "mc-sgx-compat", "mc-sgx-report-cache-api", "mc-util-serial", + "static_assertions", ] [[package]]