Fixes to cryptobox API

Cargo.toml

@@ -146,5 +146,5 @@ bulletproofs = { git = "https://github.com/dalek-cryptography/bulletproofs", rev
 cpuid-bool = { git = "https://github.com/eranrund/RustCrypto-utils", rev = "74f8e04e9d18d93fc6d05c72756c236dc88daa19" }
 
 # We need to patch aes-gcm so we can make some fields/functions/structs pub in order to have a constant time decrypt
-aes-gcm = { git = "https://github.com/xoloki/AEADs", rev = "d1a8517d3dd867ed9c5794002add67992a42f6aa" }
+aes-gcm = { git = "https://github.com/mobilecoinofficial/AEADs", rev = "d1a8517d3dd867ed9c5794002add67992a42f6aa" }
 

consensus/enclave/trusted/Cargo.toml

@@ -77,7 +77,6 @@ bulletproofs = { git = "https://github.com/dalek-cryptography/bulletproofs", rev
 cpuid-bool = { git = "https://github.com/eranrund/RustCrypto-utils", rev = "74f8e04e9d18d93fc6d05c72756c236dc88daa19" }
 
 # We need to patch aes-gcm so we can make some fields/functions/structs pub in order to have a constant time decrypt
-aes-gcm = { git = "https://github.com/xoloki/AEADs", rev = "d1a8517d3dd867ed9c5794002add67992a42f6aa" }
-
+aes-gcm = { git = "https://github.com/mobilecoinofficial/AEADs", rev = "d1a8517d3dd867ed9c5794002add67992a42f6aa" }
 
 [workspace]

crypto/box/Cargo.toml

@@ -5,15 +5,16 @@ authors = ["MobileCoin"]
 edition = "2018"
 
 [dependencies]
-aead = "0.3"
 aes-gcm = { version = "0.6.0" }
 blake2 = { version = "0.9", default-features = false }
 digest = { version = "0.9" }
 failure = { version = "0.1.8", default-features = false }
 hkdf = { version = "0.9.0", default-features = false }
+rand_core = { version = "0.5", default-features = false }
+
 mc-crypto-ct-aead = { path = "../ct-aead" }
 mc-crypto-keys = { path = "../keys", default-features = false }
-rand_core = { version = "0.5", default-features = false }
+
 
 [dev_dependencies]
 mc-util-from-random = { path = "../../util/from-random" }

crypto/box/src/fixed_buffer.rs

@@ -1,3 +1,4 @@
+use crate::aead;
 use aead::{Buffer, Error};
 
 /// The rust aead crate is organized around a Buffer trait which abstracts

crypto/box/src/hkdf_blake2b_aes_128_gcm.rs

@@ -1,17 +1,20 @@
-use crate::traits::{CryptoBox, Error};
-
-use aead::{
-    generic_array::{
-        sequence::{Concat, Split},
-        typenum::{Sum, U12, U16, U32},
-        GenericArray,
+use crate::{
+    aead::{
+        generic_array::{
+            sequence::{Concat, Split},
+            typenum::{Sum, U12, U16, U32},
+            GenericArray,
+        },
+        Aead, Error as AeadError, NewAead,
     },
-    Aead, Error as AeadError, NewAead,
+    traits::{CryptoBox, Error},
 };
+
 use aes_gcm::Aes128Gcm;
 use blake2::Blake2b;
 use core::convert::TryFrom;
 use hkdf::Hkdf;
+use mc_crypto_ct_aead::CtAeadDecrypt;
 use mc_crypto_keys::{
     CompressedRistrettoPublic, RistrettoPrivate, RistrettoPublic, RISTRETTO_PUBLIC_LEN,
 };
@@ -70,7 +73,7 @@ impl CryptoBox for RistrettoHkdfBlake2bAes128Gcm {
         key: &RistrettoPrivate,
         tag: &GenericArray<u8, Self::FooterSize>,
         buffer: &mut [u8],
-    ) -> Result<(), Error> {
+    ) -> Result<CtDecryptResult, Error> {
         // ECDH
         use mc_crypto_keys::KexReusablePrivate;
         let curve_point_bytes =
@@ -94,10 +97,7 @@ impl CryptoBox for RistrettoHkdfBlake2bAes128Gcm {
         // AES
         let mac_ref = <&GenericArray<u8, AesMacLen>>::from(&tag[RISTRETTO_PUBLIC_LEN..]);
         let aead = Aes128Gcm::new(aes_key);
-        aead.decrypt_in_place_detached(&aes_nonce, &aad, buffer, mac_ref)
-            .map_err(|_| Error::MacFailed)?;
-
-        Ok(())
+        Ok(aead.ct_decrypt_in_place_detached(&aes_nonce, &aad, buffer, mac_ref));
     }
 }
 

crypto/box/src/hkdf_box.rs

@@ -1,21 +1,23 @@
-use crate::traits::{CryptoBox, Error};
-
-use aead::{
-    generic_array::{
-        sequence::{Concat, Split},
-        typenum::{Sum, Unsigned},
-        ArrayLength, GenericArray,
+use crate::{
+    aead::{
+        generic_array::{
+            sequence::{Concat, Split},
+            typenum::{Sum, Unsigned},
+            ArrayLength, GenericArray,
+        },
+        AeadInPlace, Error as AeadError, NewAead,
     },
-    AeadInPlace, Error as AeadError, NewAead,
+    traits::{CryptoBox, Error},
 };
+
 use core::{
     convert::TryFrom,
     marker::PhantomData,
     ops::{Add, Sub},
 };
 use digest::{BlockInput, Digest, FixedOutput, Reset, Update};
 use hkdf::Hkdf;
-use mc_crypto_ct_aead::CtAeadDecrypt;
+use mc_crypto_ct_aead::{CtAeadDecrypt, CtDecryptResult};
 use mc_crypto_keys::{Kex, ReprBytes};
 use rand_core::{CryptoRng, RngCore};
 
@@ -93,7 +95,7 @@ where
         key: &KexAlgo::Private,
         tag: &GenericArray<u8, Self::FooterSize>,
         buffer: &mut [u8],
-    ) -> Result<bool, Error> {
+    ) -> Result<CtDecryptResult, Error> {
         // ECDH
         use mc_crypto_keys::KexReusablePrivate;
         // TODO: In generic_array 0.14 the tag can be split without copying it

crypto/box/src/lib.rs

@@ -16,6 +16,7 @@
 extern crate alloc;
 
 pub use aead::generic_array;
+pub use mc_crypto_ct_aead::aead;
 
 mod hkdf_box;
 mod traits;
@@ -55,7 +56,7 @@ mod test {
                         algo.decrypt(&a, &ciphertext).expect("decryption failed!");
                     assert_eq!(plaintext.len(), decrypted.len());
                     assert_eq!(plaintext, &&decrypted[..]);
-                    assert_eq!(success, true);
+                    assert_eq!(bool::from(success), true);
                 }
             }
         });
@@ -76,12 +77,8 @@ mod test {
             for plaintext in &[&plaintext1[..], &plaintext2[..]] {
                 for _reps in 0..50 {
                     let ciphertext = algo.encrypt(&mut rng, &a_pub, plaintext).unwrap();
-                    let decrypted = algo.decrypt(&not_a, &ciphertext);
-                    if decrypted.is_err() {
-                        assert_eq!(decrypted, Err(Error::MacFailed));
-                    } else {
-                        assert_eq!(decrypted.unwrap().0, false);
-                    }
+                    let (success, _decrypted) = algo.decrypt(&not_a, &ciphertext).unwrap();
+                    assert_eq!(bool::from(success), false);
                 }
             }
         });
@@ -106,7 +103,7 @@ mod test {
                         .decrypt_fixed_length(&a, &ciphertext)
                         .expect("decryption failed!");
                     assert_eq!(plaintext, &decrypted);
-                    assert_eq!(success, true);
+                    assert_eq!(bool::from(success), true);
                 }
             }
         });

crypto/box/src/traits.rs

@@ -1,3 +1,4 @@
+use crate::aead;
 use alloc::vec::Vec;
 
 use aead::{
@@ -10,10 +11,15 @@ use aead::{
 };
 use core::ops::{Add, Sub};
 use failure::Fail;
+use mc_crypto_ct_aead::CtDecryptResult;
 use mc_crypto_keys::{Kex, KeyError};
 use rand_core::{CryptoRng, RngCore};
 
 /// Error type for decryption
+///
+/// Note that mac failed is indicated separately from this enum,
+/// because a design goal is that we can be constant-time with respect to mac failure,
+/// but enum cannot be matched or accessed without branching.
 #[derive(PartialEq, Eq, Fail, Debug)]
 pub enum Error {
     #[fail(display = "Error decoding curvepoint: {}", _0)]
@@ -23,8 +29,6 @@ pub enum Error {
         _0, _1
     )]
     TooShort(usize, usize),
-    #[fail(display = "Mac failed")]
-    MacFailed,
     #[fail(display = "Unknown algorithm code: {}", _0)]
     UnknownAlgorithm(usize),
     #[fail(display = "Wrong magic bytes")]
@@ -56,13 +60,14 @@ pub trait CryptoBox<KexAlgo: Kex>: Default {
     ///
     /// Returns
     /// `Ok(true)` if decryption succeeds. `buffer` contains the plaintext and SHOULD be zeroized after use.
-    /// `Ok(false) if MAC check fails. `buffer` contains failed plaintext and SHOULD be zeroized after use.
+    /// `Ok(false) if MAC check / aead fails. `buffer` contains failed plaintext and SHOULD be zeroized after use.
+    /// `Err` if something is wrong with the cryptogram format.
     fn decrypt_in_place_detached(
         &self,
         key: &KexAlgo::Private,
         footer: &GenericArray<u8, Self::FooterSize>,
         buffer: &mut [u8],
-    ) -> Result<bool, Error>;
+    ) -> Result<CtDecryptResult, Error>;
 
     // Provided functions
     // These functions consume and produce "cryptograms" where the footer bytes
@@ -87,7 +92,11 @@ pub trait CryptoBox<KexAlgo: Kex>: Default {
     /// If status is false then mac check failed and plaintext should be zeroed.
     ///
     /// Meant to mirror aead::decrypt
-    fn decrypt(&self, key: &KexAlgo::Private, cryptogram: &[u8]) -> Result<(bool, Vec<u8>), Error> {
+    fn decrypt(
+        &self,
+        key: &KexAlgo::Private,
+        cryptogram: &[u8],
+    ) -> Result<(CtDecryptResult, Vec<u8>), Error> {
         let mut result = cryptogram.to_vec();
         let status = self.decrypt_in_place(key, &mut result)?;
         Ok((status, result))
@@ -127,7 +136,7 @@ pub trait CryptoBox<KexAlgo: Kex>: Default {
         &self,
         key: &KexAlgo::Private,
         cryptogram: &mut impl aead::Buffer,
-    ) -> Result<bool, Error> {
+    ) -> Result<CtDecryptResult, Error> {
         // Extract the footer from end of ciphertext, doing bounds checks
         if cryptogram.len() < Self::FooterSize::USIZE {
             return Err(Error::TooShort(cryptogram.len(), Self::FooterSize::USIZE));
@@ -175,7 +184,7 @@ pub trait CryptoBox<KexAlgo: Kex>: Default {
         &self,
         key: &KexAlgo::Private,
         cryptogram: &GenericArray<u8, L>,
-    ) -> Result<(bool, GenericArray<u8, Diff<L, Self::FooterSize>>), Error>
+    ) -> Result<(CtDecryptResult, GenericArray<u8, Diff<L, Self::FooterSize>>), Error>
     where
         L: ArrayLength<u8>
             + Sub<Self::FooterSize>

crypto/box/src/versioned.rs

@@ -17,23 +17,24 @@
 //! 0 = hkdf_blake2b_aes_128_gcm
 
 use crate::{
+    aead::{
+        generic_array::{
+            arr,
+            sequence::Concat,
+            typenum::{Unsigned, U50},
+            GenericArray,
+        },
+        Error as AeadError,
+    },
     hkdf_box::HkdfBox,
     traits::{CryptoBox, Error},
 };
 
-use aead::{
-    generic_array::{
-        arr,
-        sequence::Concat,
-        typenum::{Unsigned, U50},
-        GenericArray,
-    },
-    Error as AeadError,
-};
 use aes_gcm::Aes128Gcm;
 use alloc::vec::Vec;
 use blake2::Blake2b;
 use failure::Fail;
+use mc_crypto_ct_aead::CtDecryptResult;
 use mc_crypto_keys::{Kex, Ristretto};
 use rand_core::{CryptoRng, RngCore};
 
@@ -141,7 +142,7 @@ impl CryptoBox<Ristretto> for VersionedCryptoBox {
         key: &<Ristretto as Kex>::Private,
         footer: &GenericArray<u8, Self::FooterSize>,
         buffer: &mut [u8],
-    ) -> Result<bool, Error> {
+    ) -> Result<CtDecryptResult, Error> {
         // Note: When generic_array is upreved, this can be tidier using this:
         // https://docs.rs/generic-array/0.14.1/src/generic_array/sequence.rs.html#302-320
         // For now we have to split as a slice, then convert back to Generic Array.
@@ -190,7 +191,7 @@ mod test {
                         algo.decrypt(&a, &ciphertext).expect("decryption failed!");
                     assert_eq!(plaintext.len(), decrypted.len());
                     assert_eq!(plaintext, &&decrypted[..]);
-                    assert_eq!(success, true);
+                    assert!(bool::from(success));
                 }
             }
         });
@@ -211,12 +212,8 @@ mod test {
             for plaintext in &[&plaintext1[..], &plaintext2[..]] {
                 for _reps in 0..50 {
                     let ciphertext = algo.encrypt(&mut rng, &a_pub, plaintext).unwrap();
-                    let decrypted = algo.decrypt(&not_a, &ciphertext);
-                    if decrypted.is_err() {
-                        assert_eq!(decrypted, Err(Error::MacFailed));
-                    } else {
-                        assert_eq!(decrypted.unwrap().0, false);
-                    }
+                    let (success, _decrypted) = algo.decrypt(&not_a, &ciphertext).unwrap();
+                    assert!(!bool::from(success));
                 }
             }
         });