Fixes to cryptobox API #712

cbeck88 · 2021-02-09T20:14:21Z

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.
We also made the fog hint code zeroize the temporary buffers,
per cryptobox documentation.

Cargo.toml

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.

cbeck88 · 2021-02-09T20:21:26Z

crypto/box/src/hkdf_blake2b_aes_128_gcm.rs

@@ -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)?;


2 bugs here: 1 still using Error::MacFailed, 2 not using the CtAead interface

this was issue (3)

cbeck88 · 2021-02-09T20:28:39Z

crypto/ct-aead/src/traits.rs

+/// This wraps the value Choice::from(true) when decryption succeeded, and Choice::from(false) otherwise.
+#[must_use = "The result of constant time decryption should not be discarded"]
+#[derive(Copy, Clone, Debug)]
+pub struct CtDecryptResult(pub Choice);


this is solution to issue (1) (ignoring the mac check result)

cbeck88 · 2021-02-09T20:29:05Z

transaction/core/src/fog_hint.rs

    u8,
    Diff<EncryptedFogHintSize, <VersionedCryptoBox as CryptoBox<Ristretto>>::FooterSize>,
 >;

-#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
-pub struct Plaintext(PlaintextArray);


this thing did not need to exist, so it is gone (issue 4)

cbeck88 · 2021-02-09T20:30:13Z

transaction/core/src/fog_hint.rs

    #[inline(never)]
    pub fn ct_decrypt(
        ingest_server_private_key: &RistrettoPrivate,
        ciphertext: &EncryptedFogHint,
        output: &mut Self,
    ) -> Choice {
-        let mut rng = McRng::default();
-        let mut plaintext = Plaintext::default();
-        let default_pubkey = RistrettoPublic::from_random(&mut rng);


this default pubkey is removed, we just use the current value of output instead

cbeck88 · 2021-02-09T21:12:49Z

crypto/box/src/traits.rs

    fn decrypt_in_place_detached(
        &self,
        key: &KexAlgo::Private,
        footer: &GenericArray<u8, Self::FooterSize>,
        buffer: &mut [u8],
-    ) -> Result<bool, Error>;
+    ) -> Result<CtDecryptResult, Error>;


this (and similar changes) is solution to (2)

eranrund

Looks good as far as I can tell. Would love if @jcape could also take a look.

Cargo.toml

cbeck88 · 2021-02-09T23:07:21Z

I guess I should develop the uprev PR and get passing deploy, will work on that soonish

cbeck88 · 2021-02-09T23:26:58Z

confirmed that when upreving in internal repo we see this:

   Compiling fog-view-protocol v0.10.0 (/tmp/mobilenode/src/fog/view/protocol)
error: unused `mc_crypto_ct_aead::traits::CtDecryptResult` that must be used
  --> src/fog/view/protocol/src/user_private.rs:73:9
   |
73 | /         VersionedCryptoBox::default()
74 | |             .decrypt_in_place(self.get_view_key(), &mut payload)
75 | |             .map_err(TxOutRecoveryError::DecryptionFailed)?;
   | |____________________________________________________________^
   |
   = note: `-D unused-must-use` implied by `-D warnings`
   = note: The result of constant time decryption should not be discarded

…ternal)

cbeck88 · 2021-02-10T00:29:08Z

crypto/box/src/traits.rs

+        &self,
+        key: &KexAlgo::Private,
+        cryptogram: &[u8],
+    ) -> Result<(CtDecryptResult, Vec<u8>), Error> {


there's a good argument that this should be made to return Zeroizing<Vec<u8>>, that would mean that all the callers in SDKs etc. dont need to explicitly zeroize this. zeroize is pretty lightweight crate that I think most things already rely on via transaction-core

this passed deploy, and i decided to punt on zeroize for now because i can't find a good solution for zeroizing generic array. I posted a pull request here:

fizyk20/generic-array#112

jcape

LGTM, have some suggestions.

General question: if the bytes in the fog hint are not actually a valid ristretto point (i.e. decompression fails), do we still need to produce output in constant-time? If so, then the output.public_key assignment is simply pushing the non-constant-timeness of that (bad) hint into later code.

crypto/box/src/versioned.rs

Co-authored-by: James Cape <jamescape777@gmail.com>

cbeck88 · 2021-02-10T03:08:04Z

the way it works in ingest enclave impl is:
first, a FogHint is initialized with a random valid ristretto public
then, ct decrypt is used with that value as the output, so in case of decryption failure, the key we get is random, and we return the choice anyways, in case fog enclave wants to do something more with the success value (although i think in latest revisions, it doesn't do anything with it)

`mc-transaction-core` should be as portable as possible, but it had a dependency on `mc-crypto-rand` which is not as portable. Getting an RNG is very hard to do portably -- the approach we use in `mc-crypto-rand` is like, use the standard library approach and the getrandom crate, which still requires an OS, or if you have RDRAND instruction, use that, and selecting between these alternatives is what `mc-crypto-rand` does basically. But this will not work on embedded devices that don't have an OS or intel chips. It just happens to work for all our current enclave phone and server targets. But it may pose a portability problem if someone wanted to build `mc-transaction-core` for a more restricted device. Fortunately the only thing that was creating an RNG directly in `mc-transaction-core`, was the code in the `FogHint` which we removed in mobilecoinfoundation#712 So at this point `mc-transaction-core` only needs `mc-crypto-rand` for tests. Moving it to dev-dependency helps make the transaction core more portable.

`mc-transaction-core` should be as portable as possible, but it had a dependency on `mc-crypto-rand` which is not as portable. Getting an RNG is very hard to do portably -- the approach we use in `mc-crypto-rand` is like, use the standard library approach and the getrandom crate, which still requires an OS, or if you have RDRAND instruction, use that, and selecting between these alternatives is what `mc-crypto-rand` does basically. But this will not work on embedded devices that don't have an OS or intel chips. It just happens to work for all our current enclave phone and server targets. But it may pose a portability problem if someone wanted to build `mc-transaction-core` for a more restricted device. Fortunately the only thing that was creating an RNG directly in `mc-transaction-core`, was the code in the `FogHint` which we removed in #712 So at this point `mc-transaction-core` only needs `mc-crypto-rand` for tests. Moving it to dev-dependency helps make the transaction core more portable.

cbeck88 requested review from jcape, eranrund, sugargoat and a team February 9, 2021 20:14

cbeck88 commented Feb 9, 2021

View reviewed changes

Cargo.toml Show resolved Hide resolved

cbeck88 force-pushed the fixups_to_cryptobox branch from d6a5362 to 285bcf4 Compare February 9, 2021 20:20

cbeck88 commented Feb 9, 2021

View reviewed changes

eranrund approved these changes Feb 9, 2021

View reviewed changes

Cargo.toml Show resolved Hide resolved

update patch in consensus enclave trusted also

2d99389

cbeck88 added 2 commits February 9, 2021 16:40

lock file

21abc9a

make plaintext array in fog hint pub again (it is used in tests in in…

759ae29

…ternal)

cbeck88 commented Feb 10, 2021

View reviewed changes

jcape approved these changes Feb 10, 2021

View reviewed changes

crypto/box/src/versioned.rs Outdated Show resolved Hide resolved

crypto/box/src/versioned.rs Outdated Show resolved Hide resolved

cbeck88 and others added 2 commits February 9, 2021 17:55

Update crypto/box/src/versioned.rs

0af0bff

Co-authored-by: James Cape <jamescape777@gmail.com>

Update crypto/box/src/versioned.rs

894193f

Co-authored-by: James Cape <jamescape777@gmail.com>

cbeck88 merged commit 29f11c8 into mobilecoinfoundation:master Feb 10, 2021

cbeck88 deleted the fixups_to_cryptobox branch February 10, 2021 04:20

cbeck88 mentioned this pull request Feb 10, 2021

move mc-crypto-rand to be a dev-dependency of mc-transaction-core #717

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes to cryptobox API #712

Fixes to cryptobox API #712

cbeck88 commented Feb 9, 2021 •

edited

cbeck88 Feb 9, 2021

cbeck88 Feb 9, 2021

cbeck88 Feb 9, 2021

cbeck88 Feb 9, 2021

cbeck88 Feb 9, 2021

cbeck88 Feb 9, 2021

eranrund left a comment

cbeck88 commented Feb 9, 2021

cbeck88 commented Feb 9, 2021

cbeck88 Feb 10, 2021 •

edited

cbeck88 Feb 10, 2021

jcape left a comment

cbeck88 commented Feb 10, 2021

Fixes to cryptobox API #712

Fixes to cryptobox API #712

Conversation

cbeck88 commented Feb 9, 2021 • edited

cbeck88 Feb 9, 2021

Choose a reason for hiding this comment

cbeck88 Feb 9, 2021

Choose a reason for hiding this comment

cbeck88 Feb 9, 2021

Choose a reason for hiding this comment

cbeck88 Feb 9, 2021

Choose a reason for hiding this comment

cbeck88 Feb 9, 2021

Choose a reason for hiding this comment

cbeck88 Feb 9, 2021

Choose a reason for hiding this comment

eranrund left a comment

Choose a reason for hiding this comment

cbeck88 commented Feb 9, 2021

cbeck88 commented Feb 9, 2021

cbeck88 Feb 10, 2021 • edited

Choose a reason for hiding this comment

cbeck88 Feb 10, 2021

Choose a reason for hiding this comment

jcape left a comment

Choose a reason for hiding this comment

cbeck88 commented Feb 10, 2021

cbeck88 commented Feb 9, 2021 •

edited

cbeck88 Feb 10, 2021 •

edited