remove unnecessary Result-wrapping in `crypto.rs` #474

cosmicexplorer · 2022-06-27T07:36:03Z

Broken out from #287.

Separate CTR and CBC encryption methods in ctr and cbc submodules.
Define pub const sizes for array lengths.
Require fixed-size arrays to en/decryption methods to remove EncryptionError and one case of DecryptionError.
Rename methods to aes_256_* and aes_256_ctr_hmac_* for consistency.

TODO

Figure out how to apply this comment: Add docstrings for 7 more Rust API modules #287 (comment).

jrose-signal · 2022-07-20T19:26:51Z

These aren't pub for a reason: if someone wants to use these operations, they should get them directly from the RustCrypto crates. These are pretty much just convenience wrappers because of how they're used within the protocol. (The hardcoded "first ten bytes as a MAC" is something that a generalized library probably wouldn't hardcode.)

I think you're right about EncryptionError, though. Someone has to check that these keys are the right length, but it can happen a level up. And then I think DecryptionError collapses down to a struct instead of a one-case enum (not that it matters so much for an implementation detail).

cosmicexplorer · 2022-09-09T22:11:24Z

These are pretty much just convenience wrappers because of how they're used within the protocol.

Ok, so in light of this I've changed it to pub(crate) mod crypto; in lib.rs, removed the doctests, and re-focused this PR on just removing unnecessary Results by enforcing fixed-size slices where possible.

However, on top of that, I've also kept in:

the cbc and ctr module organization,
some docstrings for parts that I thought weren't immediately clear,
new en/decryption test cases using Rng::gen() (vs the existing test cases which only check a single literal hex key).

I can revert any of that as desired.

jrose-signal · 2022-09-15T22:11:40Z

rust/protocol/src/crypto.rs

+/// bad ciphertext: {0}
+///
+/// Either the input is malformed, or the MAC doesn't match on decryption.
+///
+/// These cases should not be distinguished; message corruption can cause either problem.
+#[derive(Debug, Display, Error)]
+#[ignore_extra_doc_attributes]
+pub struct DecryptionError(pub &'static str);


Nitpick: For some reason displaydoc on structs weirds me out; maybe it's because enum cases often get abbreviated docs anyway in a way that types don't. Mind switching to thiserror-style for the Display representation instead?

jrose-signal · 2022-09-15T22:12:21Z

rust/protocol/src/crypto.rs

-    BadCiphertext(&'static str),
-}
+// TODO: Could be nice to have a type-safe library for manipulating units of bytes safely.
+const BITS_PER_BYTE: usize = std::mem::size_of::<u8>() * 8;


Rust guarantees that size_of::<u8>() is 1, so I think it's fine to just write 8 here. (The named constant still makes sense though.)

jrose-signal · 2022-09-15T22:14:48Z

rust/protocol/src/crypto.rs

-    Ok(ctext)
-}
+/// Use AES-256 in CTR mode.
+pub mod ctr {


I don't know if I really see the point of this. Yes, there's a little more organization, but "CTR" and "CBR" don't imply "AES-256" by themselves, and they're all just convenience wrappers for the session implementation in any case.

jrose-signal · 2022-09-15T22:17:34Z

rust/protocol/src/crypto.rs

-}
+    /// Length in bytes of the [`Hmac`] key used for [`aes_256_ctr_hmac_encrypt`] and
+    /// [`aes_256_ctr_hmac_decrypt`].
+    pub const MAC_KEY_LENGTH: usize = 80 / BITS_PER_BYTE;


The MAC key isn't 80 bits; only the MAC itself is. The key can in theory be any length and in practice we use a 32-byte key.

jrose-signal · 2022-09-15T22:20:43Z

rust/protocol/src/crypto.rs

+            let key: [u8; AES_256_KEY_SIZE] =
+                hex::decode("603DEB1015CA71BE2B73AEF0857D77811F352C073B6108D72D9810A30914DFF4")
+                    .expect("valid hex")
+                    .try_into()
+                    .expect("correct array size");


The hex_literal crate used elsewhere in the workspace can check these conditions at compile time; if you're adding more tests it might be a good time to switch over (incrementally).

jrose-signal · 2022-09-15T22:22:59Z

rust/protocol/src/lib.rs

@@ -24,7 +24,7 @@

 mod address;
 mod consts;
-mod crypto;
+pub(crate) mod crypto;


This doesn't need to be changed, right?

jrose-signal · 2022-09-15T22:24:31Z

rust/protocol/src/sealed_sender.rs

-            &sender_eph_keys.mac_key,
-        )
-        .expect("just generated these keys, they should be correct");
+            array_ref![&sender_eph_keys.mac_key, 0, crypto::ctr::MAC_KEY_LENGTH],


…and this is where the MAC key is incorrectly truncated, and our lack of "decrypting this existing message with this session state" tests has come back to bite us.

jrose-signal · 2022-09-15T22:28:07Z

rust/protocol/src/session_cipher.rs

-        Err(crypto::DecryptionError::BadKeyOrIv) => {
-            log::warn!(
-                "{} session state corrupt for {}",
-                current_or_previous,
-                remote_address,
-            );
-            return Err(SignalProtocolError::InvalidSessionStructure(
-                "invalid receiver chain message keys",
-            ));
-        }


This is the change that convinced me this is a good idea: at first I thought "what happens if those fields are corrupt"? But they can't be, because we already check that when getting the message keys out of the session state, and that wasn't in the type system anywhere.

jrose-signal · 2022-09-15T22:29:15Z

rust/protocol/src/group_cipher.rs

            log::error!(
-                "incoming sender key state corrupt for {}, distribution ID {}, chain ID {}",
-                sender,
+                "outgoing sender key state corrupt for distribution ID {}",


Copy/paste error: "incoming" got switched for "outgoing" here and below.

jrose-signal · 2022-09-15T22:30:33Z

rust/protocol/src/group_cipher.rs

+    let cipher_key: [u8; crypto::AES_256_KEY_SIZE] =
+        message_keys.cipher_key().try_into().map_err(|_| {
+            log::error!(
+                "outgoing sender key state corrupt for distribution ID {}",
+                distribution_id,
+            );
+            SignalProtocolError::InvalidSenderKeySession { distribution_id }
+        })?;
+    let iv: [u8; crypto::AES_NONCE_SIZE] = message_keys.iv().try_into().map_err(|_| {
+        log::error!(
+            "outgoing sender key state corrupt for distribution ID {}",
+            distribution_id,
+        );
+        SignalProtocolError::InvalidSenderKeySession { distribution_id }
+    })?;
+    let ciphertext = crypto::cbc::aes_256_cbc_encrypt(plaintext, &cipher_key, &iv);


This duplication is a little annoying, but I can't see a great way to clean it up. Do you think it's worth putting that error generation into a local helper?

stale · 2023-02-04T03:56:51Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale · 2023-02-11T04:16:04Z

This issue has been closed due to inactivity.

cosmicexplorer mentioned this pull request Jun 27, 2022

module documentation tracking issue #467

Open

11 tasks

cosmicexplorer force-pushed the document-crypto-module branch 3 times, most recently from 9e79b45 to 78efa92 Compare June 27, 2022 08:12

cosmicexplorer marked this pull request as draft June 27, 2022 08:40

cosmicexplorer marked this pull request as ready for review June 27, 2022 08:41

cosmicexplorer marked this pull request as draft June 27, 2022 08:41

cosmicexplorer marked this pull request as ready for review July 18, 2022 22:37

cosmicexplorer added a commit to cosmicexplorer/libsignal that referenced this pull request Jul 19, 2022

[signalapp#474] add docstrings and doctests to crypto.rs

8f29bf9

cosmicexplorer force-pushed the document-crypto-module branch from 78efa92 to 8f29bf9 Compare July 19, 2022 00:02

cosmicexplorer added a commit to cosmicexplorer/libsignal that referenced this pull request Jul 19, 2022

[signalapp#474] add docstrings and doctests to crypto.rs

2bc4257

cosmicexplorer added a commit to cosmicexplorer/libsignal that referenced this pull request Jul 19, 2022

[signalapp#474] add docstrings and doctests to crypto.rs

fdaaad4

cosmicexplorer added a commit to cosmicexplorer/libsignal that referenced this pull request Sep 9, 2022

[signalapp#474] add docstrings and doctests to crypto.rs

fbcaa0e

cosmicexplorer force-pushed the document-crypto-module branch from 8f29bf9 to fbcaa0e Compare September 9, 2022 20:00

cosmicexplorer added a commit to cosmicexplorer/libsignal that referenced this pull request Sep 9, 2022

[signalapp#474] add docstrings and doctests to crypto.rs

0a1bc67

cosmicexplorer added a commit to cosmicexplorer/libsignal that referenced this pull request Sep 9, 2022

[signalapp#474] remove unnecessary Result-wrapping from crypto.rs

d71fe76

cosmicexplorer force-pushed the document-crypto-module branch from fbcaa0e to d71fe76 Compare September 9, 2022 21:59

cosmicexplorer changed the title ~~add docstrings and doctests to crypto.rs~~ remove unnecessary Result-wrapping in crypto.rs Sep 9, 2022

[signalapp#474] remove unnecessary Result-wrapping from crypto.rs

e144dca

cosmicexplorer force-pushed the document-crypto-module branch from d71fe76 to e144dca Compare September 9, 2022 22:08

jrose-signal reviewed Sep 15, 2022

View reviewed changes

stale bot added the stale label Feb 4, 2023

stale bot closed this Feb 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove unnecessary Result-wrapping in `crypto.rs` #474

remove unnecessary Result-wrapping in `crypto.rs` #474

cosmicexplorer commented Jun 27, 2022 •

edited

Loading

jrose-signal commented Jul 20, 2022

cosmicexplorer commented Sep 9, 2022 •

edited

Loading

jrose-signal Sep 15, 2022

jrose-signal Sep 15, 2022

jrose-signal Sep 15, 2022

jrose-signal Sep 15, 2022

jrose-signal Sep 15, 2022

jrose-signal Sep 15, 2022

jrose-signal Sep 15, 2022

jrose-signal Sep 15, 2022

jrose-signal Sep 15, 2022

jrose-signal Sep 15, 2022

stale bot commented Feb 4, 2023

stale bot commented Feb 11, 2023

remove unnecessary Result-wrapping in crypto.rs #474

remove unnecessary Result-wrapping in crypto.rs #474

Conversation

cosmicexplorer commented Jun 27, 2022 • edited Loading

TODO

jrose-signal commented Jul 20, 2022

cosmicexplorer commented Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Feb 4, 2023

stale bot commented Feb 11, 2023

remove unnecessary Result-wrapping in `crypto.rs` #474

remove unnecessary Result-wrapping in `crypto.rs` #474

cosmicexplorer commented Jun 27, 2022 •

edited

Loading

cosmicexplorer commented Sep 9, 2022 •

edited

Loading