Fixes to cryptobox API (#712)

* Fixes to cryptobox API Recent testing has shown that sometimes, errors are mishandled when using the cryptobox API. The problem is: (1) Mac check failure is returned using a `bool`, but this doesn't have the `#[must_use]` annotation that `core::Result` does. (2) Cryptobox itself was still using the `Error::MacFailed` which was supposed to go away. (3) Cryptobox itself was not using xoloki's CtAead trait, it was still using the variable time thing, and only the return codes were changing. The solution proposed is: (1) Introduce CtDecryptResult type which is a new-type wrapper around `subtle::Choice` which has the must-use annotation. Put this in `mc-crypto-ct-aead` crate and use it. Previously I was worried to use `subtle::Choice` as an API type, but now we have doen that elsewhere and it hasn't caused churn or problems. (2) Make cryptobox return CtDecryptResult instead of bool as it had been doing, and nuke the `MacFailed` error variant which should be gone now. Make cryptobox also use the same version of `aead` crate that `mc-crypto-ct-aead` crate is exporting. (3) Make cryptobox actually use the constant-time aes-gcm. Besides these issues, while updating the fog hint code that uses this, we noticed: (4) The fog hint decryption code which used cryptobox is needlessly complicated, uses an RNG for no real reason, and introduces unnecessary types with special implementations of subtle trait. Proposed change: (4) We have now made it completely determinsitic, which is better for testability, and documented that it doesn't make changes if decryption fails, which is all that is needed. Then the caller can decide what value the output parameter should have in that case. We deleted the unnecessary types and functions. * update patch in consensus enclave trusted also * lock file * make plaintext array in fog hint pub again (it is used in tests in internal) * Update crypto/box/src/versioned.rs Co-authored-by: James Cape <jamescape777@gmail.com> * Update crypto/box/src/versioned.rs Co-authored-by: James Cape <jamescape777@gmail.com> Co-authored-by: James Cape <jamescape777@gmail.com>
mobilecoinfoundation · Feb 10, 2021 · 29f11c8 · 29f11c8
1 parent 388213e
commit 29f11c8
Show file tree

Hide file tree

Showing 15 changed files with 154 additions and 153 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/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" }
 
diff --git a/consensus/enclave/trusted/Cargo.lock b/consensus/enclave/trusted/Cargo.lock
diff --git a/consensus/enclave/trusted/Cargo.toml b/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]
diff --git a/crypto/box/Cargo.toml b/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" }

diff --git a/crypto/box/src/fixed_buffer.rs b/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

diff --git a/crypto/box/src/hkdf_blake2b_aes_128_gcm.rs b/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));
     }
 }
 

diff --git a/crypto/box/src/hkdf_box.rs b/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

diff --git a/crypto/box/src/lib.rs b/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);
                 }
             }
         });

diff --git a/crypto/box/src/traits.rs b/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>

diff --git a/crypto/box/src/versioned.rs b/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));
                 }
             }
         });