From 8d419fec45099cd1845a620b5f7b37e783c81098 Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Thu, 30 Jun 2022 08:10:14 -0400 Subject: [PATCH] demonstrate using subtle-ng-derive with ConstantTimeCmp --- Cargo.lock | 19 +++- rust/protocol/Cargo.toml | 3 +- rust/protocol/src/curve.rs | 140 +++++++++++++---------------- rust/protocol/src/identity_key.rs | 9 +- rust/protocol/src/lib.rs | 1 - rust/protocol/src/utils.rs | 141 ------------------------------ 6 files changed, 88 insertions(+), 225 deletions(-) delete mode 100644 rust/protocol/src/utils.rs diff --git a/Cargo.lock b/Cargo.lock index f43ded6a85..0fb0d4ea74 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1014,7 +1014,8 @@ dependencies = [ "prost-build", "rand 0.7.3", "sha2", - "subtle", + "subtle-ng", + "subtle-ng-derive", "thiserror", "uuid", "x25519-dalek", @@ -2036,6 +2037,22 @@ version = "2.4.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6bdef32e8150c2a081110b42772ffe7d7c9032b606bc226c8260fd97e0976601" +[[package]] +name = "subtle-ng" +version = "2.5.0" +source = "git+https://github.com/cosmicexplorer/subtle?rev=287f4ead34b0cdd4185083bdd0cd37a38b0954a4#287f4ead34b0cdd4185083bdd0cd37a38b0954a4" + +[[package]] +name = "subtle-ng-derive" +version = "0.0.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d00ace24986956f69866a3ac7279e9545b22d84a35a0c8132910fcbb27200670" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "syn" version = "1.0.80" diff --git a/rust/protocol/Cargo.toml b/rust/protocol/Cargo.toml index 53ffe809b7..34b5dc65bd 100644 --- a/rust/protocol/Cargo.toml +++ b/rust/protocol/Cargo.toml @@ -23,7 +23,8 @@ itertools = "0.10.1" prost = "0.9" rand = "0.7.3" sha2 = "0.9" -subtle = "2.2.3" +subtle = { package = "subtle-ng", git = "https://github.com/cosmicexplorer/subtle", rev = "287f4ead34b0cdd4185083bdd0cd37a38b0954a4" } +subtle-ng-derive = "0.0.5" x25519-dalek = "1.0" hex = "0.4" log = "0.4" diff --git a/rust/protocol/src/curve.rs b/rust/protocol/src/curve.rs index 33e683742a..042eac3424 100644 --- a/rust/protocol/src/curve.rs +++ b/rust/protocol/src/curve.rs @@ -1,5 +1,5 @@ // -// Copyright 2020-2021 Signal Messenger, LLC. +// Copyright 2020-2022 Signal Messenger, LLC. // SPDX-License-Identifier: AGPL-3.0-only // @@ -7,50 +7,82 @@ pub(crate) mod curve25519; use crate::{Result, SignalProtocolError}; -use std::cmp::Ordering; use std::convert::TryFrom; use std::fmt; use arrayref::array_ref; +use displaydoc::Display; +use num_enum; use rand::{CryptoRng, Rng}; -use subtle::ConstantTimeEq; - -#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)] +use subtle::{self, Convertible, IteratedEq, IteratedGreater, IteratedOperation}; +use subtle_ng_derive::{ConstEq, ConstOrd, ConstantTimeEq, ConstantTimeGreater}; + +#[derive( + Debug, + Display, + Copy, + Clone, + ConstEq, + ConstOrd, + num_enum::TryFromPrimitive, + num_enum::IntoPrimitive, +)] +#[repr(u8)] pub enum KeyType { - Djb, + /// + Djb = 0x05u8, } -impl fmt::Display for KeyType { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - fmt::Debug::fmt(self, f) +impl Convertible for KeyType { + type To = u8; + fn for_constant_operation(&self) -> u8 { + (*self).into() } } -impl KeyType { - fn value(&self) -> u8 { - match &self { - KeyType::Djb => 0x05u8, +#[derive(Debug, Clone, Copy, ConstEq, ConstOrd)] +enum PublicKeyData { + DjbPublicKey([u8; curve25519::PUBLIC_KEY_LENGTH]), +} + +impl PublicKeyData { + pub(crate) fn key_type(&self) -> KeyType { + match self { + Self::DjbPublicKey(_) => KeyType::Djb, + } + } + pub(crate) fn key_data(&self) -> &[u8] { + match self { + Self::DjbPublicKey(ref k) => k.as_ref(), } } } -impl TryFrom for KeyType { - type Error = SignalProtocolError; - - fn try_from(x: u8) -> Result { - match x { - 0x05u8 => Ok(KeyType::Djb), - t => Err(SignalProtocolError::BadKeyType(t)), - } +impl subtle::ConstantTimeEq for PublicKeyData { + /// A constant-time comparison as long as the two keys have a matching type. + /// + /// If the two keys have different types, the comparison short-circuits, + /// much like comparing two slices of different lengths. + fn ct_eq(&self, other: &Self) -> subtle::Choice { + let mut x = IteratedEq::initiate(); + x.apply_eq(&self.key_type(), &other.key_type()); + x.apply_eq(self.key_data(), other.key_data()); + x.extract_result() } } -#[derive(Debug, Clone, Copy, Eq, PartialEq)] -enum PublicKeyData { - DjbPublicKey([u8; curve25519::PUBLIC_KEY_LENGTH]), +impl subtle::ConstantTimeGreater for PublicKeyData { + fn ct_gt(&self, other: &Self) -> subtle::Choice { + let mut x = IteratedGreater::initiate(); + x.apply_gt(&self.key_type(), &other.key_type()); + x.apply_gt(self.key_data(), other.key_data()); + x.extract_result() + } } -#[derive(Clone, Copy, Eq)] +impl subtle::ConstantTimeLess for PublicKeyData {} + +#[derive(Clone, Copy, ConstEq, ConstOrd, ConstantTimeEq, ConstantTimeGreater)] pub struct PublicKey { key: PublicKeyData, } @@ -64,7 +96,8 @@ impl PublicKey { if value.is_empty() { return Err(SignalProtocolError::NoKeyTypeIdentifier); } - let key_type = KeyType::try_from(value[0])?; + let key_type = + KeyType::try_from(value[0]).map_err(|e| SignalProtocolError::BadKeyType(e.number))?; match key_type { KeyType::Djb => { // We allow trailing data after the public key (why?) @@ -100,7 +133,7 @@ impl PublicKey { PublicKeyData::DjbPublicKey(v) => v.len(), }; let mut result = Vec::with_capacity(1 + value_len); - result.push(self.key_type().value()); + result.push(self.key.key_type().into()); match &self.key { PublicKeyData::DjbPublicKey(v) => result.extend_from_slice(v), } @@ -129,18 +162,6 @@ impl PublicKey { } } } - - fn key_data(&self) -> &[u8] { - match &self.key { - PublicKeyData::DjbPublicKey(ref k) => k.as_ref(), - } - } - - pub fn key_type(&self) -> KeyType { - match &self.key { - PublicKeyData::DjbPublicKey(_) => KeyType::Djb, - } - } } impl From for PublicKey { @@ -157,58 +178,23 @@ impl TryFrom<&[u8]> for PublicKey { } } -impl subtle::ConstantTimeEq for PublicKey { - /// A constant-time comparison as long as the two keys have a matching type. - /// - /// If the two keys have different types, the comparison short-circuits, - /// much like comparing two slices of different lengths. - fn ct_eq(&self, other: &PublicKey) -> subtle::Choice { - if self.key_type() != other.key_type() { - return 0.ct_eq(&1); - } - self.key_data().ct_eq(other.key_data()) - } -} - -impl PartialEq for PublicKey { - fn eq(&self, other: &PublicKey) -> bool { - bool::from(self.ct_eq(other)) - } -} - -impl Ord for PublicKey { - fn cmp(&self, other: &Self) -> Ordering { - if self.key_type() != other.key_type() { - return self.key_type().cmp(&other.key_type()); - } - - crate::utils::constant_time_cmp(self.key_data(), other.key_data()) - } -} - -impl PartialOrd for PublicKey { - fn partial_cmp(&self, other: &PublicKey) -> Option { - Some(self.cmp(other)) - } -} - impl fmt::Debug for PublicKey { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!( f, "PublicKey {{ key_type={}, serialize={:?} }}", - self.key_type(), + self.key.key_type(), self.serialize() ) } } -#[derive(Debug, Clone, Copy, Eq, PartialEq)] +#[derive(Debug, Clone, Copy)] enum PrivateKeyData { DjbPrivateKey([u8; curve25519::PRIVATE_KEY_LENGTH]), } -#[derive(Clone, Copy, Eq, PartialEq)] +#[derive(Clone, Copy)] pub struct PrivateKey { key: PrivateKeyData, } diff --git a/rust/protocol/src/identity_key.rs b/rust/protocol/src/identity_key.rs index 4777f5b803..81622631a6 100644 --- a/rust/protocol/src/identity_key.rs +++ b/rust/protocol/src/identity_key.rs @@ -1,21 +1,22 @@ // -// Copyright 2020 Signal Messenger, LLC. +// Copyright 2020-2022 Signal Messenger, LLC. // SPDX-License-Identifier: AGPL-3.0-only // use crate::proto; use crate::{KeyPair, PrivateKey, PublicKey, Result, SignalProtocolError}; +use prost::Message; use rand::{CryptoRng, Rng}; -use std::convert::TryFrom; +use subtle_ng_derive::{ConstEq, ConstOrd, ConstantTimeEq, ConstantTimeGreater}; -use prost::Message; +use std::convert::TryFrom; // Used for domain separation between alternate-identity signatures and other key-to-key signatures. const ALTERNATE_IDENTITY_SIGNATURE_PREFIX_1: &[u8] = &[0xFF; 32]; const ALTERNATE_IDENTITY_SIGNATURE_PREFIX_2: &[u8] = b"Signal_PNI_Signature"; -#[derive(Debug, PartialOrd, Ord, PartialEq, Eq, Clone, Copy)] +#[derive(Debug, ConstEq, ConstOrd, ConstantTimeEq, ConstantTimeGreater, Clone, Copy)] pub struct IdentityKey { public_key: PublicKey, } diff --git a/rust/protocol/src/lib.rs b/rust/protocol/src/lib.rs index 9ac6dbe1b0..5308151f9c 100644 --- a/rust/protocol/src/lib.rs +++ b/rust/protocol/src/lib.rs @@ -39,7 +39,6 @@ mod session; mod session_cipher; mod state; mod storage; -mod utils; use error::Result; diff --git a/rust/protocol/src/utils.rs b/rust/protocol/src/utils.rs deleted file mode 100644 index 67a108c81e..0000000000 --- a/rust/protocol/src/utils.rs +++ /dev/null @@ -1,141 +0,0 @@ -// -// Copyright 2020 Signal Messenger, LLC. -// SPDX-License-Identifier: AGPL-3.0-only -// - -use std::cmp::Ordering; - -fn expand_top_bit(a: u8) -> u8 { - //if (a >> 7) == 1 { 0xFF } else { 0 } - 0u8.wrapping_sub(a >> 7) -} - -fn ct_is_zero(a: u8) -> u8 { - //if a == 0 { 0xFF } else { 0 } - expand_top_bit(!a & a.wrapping_sub(1)) -} - -fn ct_is_eq(a: u8, b: u8) -> u8 { - //if a == b { 0xFF } else { 0 } - ct_is_zero(a ^ b) -} - -fn ct_is_lt(a: u8, b: u8) -> u8 { - //if a < b { 0xFF } else { 0 } - expand_top_bit(a ^ ((a ^ b) | ((a.wrapping_sub(b)) ^ a))) -} - -fn ct_select(mask: u8, a: u8, b: u8) -> u8 { - debug_assert!(mask == 0 || mask == 0xFF); - //if mask == 0xFF { a } else if mask == 0x00 { b } else { unreachable!(); } - b ^ (mask & (a ^ b)) -} - -/* -* If x and y are different lengths, this leaks information about -* their relative sizes. This is irrelevant as we always invoke it -* with two inputs of the same size. -* -* In addition it will leak the final comparison result, when the -* integer is translated to the Ordering enum. This seems unavoidable. -* -* The primary goal of this function is to not leak any additional -* information, besides the ordering, about the value of the two keys, -* say due to an early exit of the loop. -* -* It would be possible to instead have this function SHA-256 hash both -* inputs, then compare the resulting hashes in the usual non-const -* time way. We avoid this approach at the moment since it is not clear -* if applications will rely on public key ordering being defined in -* some particular way or not. - */ - -pub(crate) fn constant_time_cmp(x: &[u8], y: &[u8]) -> Ordering { - if x.len() < y.len() { - return Ordering::Less; - } - if x.len() > y.len() { - return Ordering::Greater; - } - - let mut result: u8 = 0; - - for i in 0..x.len() { - let a = x[x.len() - 1 - i]; - let b = y[x.len() - 1 - i]; - - let is_eq = ct_is_eq(a, b); - let is_lt = ct_is_lt(a, b); - - result = ct_select(is_eq, result, ct_select(is_lt, 1, 255)); - } - - debug_assert!(result == 0 || result == 1 || result == 255); - - if result == 0 { - Ordering::Equal - } else if result == 1 { - Ordering::Less - } else { - Ordering::Greater - } -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn test_constant_time_cmp() { - use rand::Rng; - - assert_eq!(constant_time_cmp(&[1], &[1]), Ordering::Equal); - assert_eq!(constant_time_cmp(&[0, 1], &[1]), Ordering::Greater); - assert_eq!(constant_time_cmp(&[1], &[0, 1]), Ordering::Less); - assert_eq!(constant_time_cmp(&[2], &[1, 0, 1]), Ordering::Less); - - let mut rng = rand::rngs::OsRng; - for len in 1..320 { - let x: Vec = (0..len).map(|_| rng.gen()).collect(); - let y: Vec = (0..len).map(|_| rng.gen()).collect(); - let expected = x.cmp(&y); - let result = constant_time_cmp(&x, &y); - assert_eq!(result, expected); - - let expected = y.cmp(&x); - let result = constant_time_cmp(&y, &x); - assert_eq!(result, expected); - } - } - - #[test] - fn test_ct_is_zero() { - assert_eq!(ct_is_zero(0), 0xFF); - - for i in 1..255 { - assert_eq!(ct_is_zero(i), 0x00); - } - } - - #[test] - fn test_ct_is_lt() { - for x in 0..255 { - for y in 0..255 { - let expected = if x < y { 0xFF } else { 0 }; - let result = ct_is_lt(x, y); - assert_eq!(result, expected); - } - } - } - - #[test] - fn test_ct_is_eq() { - for x in 0..255 { - for y in 0..255 { - let expected = if x == y { 0xFF } else { 0 }; - let result = ct_is_eq(x, y); - assert_eq!(result, expected); - } - } - } -}