Adding traits for symmetric ciphers#44
Conversation
# Conflicts: # QUALITY_AND_STYLE.md # crypto/core/src/traits.rs
| #[cfg(std)] | ||
| /// A one-shot API to encrypt some plaintext with the given key. | ||
| /// This function returns the ciphertext as a Vec<u8>, and therefore is only available when compiling with std. | ||
| /// Returns a tuple containing the initialization data and the ciphertext. | ||
| /// This is not available if building for no_std. |
There was a problem hiding this comment.
Made a similar comment in another PR, but std isn't a built-in cfg. nothing in this repo defines it (no Cargo [features], .cargo/config.toml, build.rs, or rustflags), so #[cfg(std)] is always false and encrypt/decrypt/aead_encrypt/aead_decrypt are silently stripped from the trait. Use #[cfg(feature = "std")] and add [features] std = [] (probably default = ["std"]) to crypto/core/Cargo.toml, or drop the Vec APIs.
There was a problem hiding this comment.
There was a problem hiding this comment.
@ounsworth I think to unblock, we'll merge this PR the way it is (but add a TODO), and then retroactively patch with #49
| SecurityStrength::_192bit, | ||
| SecurityStrength::_256bit, | ||
| ]; | ||
| for ss in security_strengths.iter() { |
There was a problem hiding this comment.
set_security_strength calls drop_hazardous_operations() on every success and rejects raising strength without the flag (key_material.rs: lines 406–455).
Since allow_hazardous_operations() is called only once before the loop, the 2nd iteration (None → _112bit) returns HazardousOperationNotPermitted and this .unwrap() panics. It also rejects strengths above the key length, so KEY_LEN < 32 panics regardless.
Recommend that you re-enable hazardous ops each iteration and bound tested strengths by KEY_LEN*8. (Same pattern in the BlockCipher and AEAD tests.)
There was a problem hiding this comment.
Well spotted.
Actually, I've already solved this problem on release/0.1.2alpha via #40 .
Once this PR gets rebased on top of that (or merged into it), this problem will go away.
| SecurityStrength::_192bit, | ||
| SecurityStrength::_256bit, | ||
| ]; | ||
| for ss in security_strengths.iter() { |
There was a problem hiding this comment.
Similar to other PR. This loop varies key strength, you are using mac_key in encrypt_out(&mac_key,...).
| let mac_key = | ||
| KeyMaterial::<KEY_LEN>::from_bytes_as_type(&DUMMY_SEED_512[..KEY_LEN], KeyType::MACKey) | ||
| .unwrap(); | ||
| match C::encrypt_out(&mac_key, msg, &mut ct) { |
There was a problem hiding this comment.
If we want to test the wrong key check, wouldn't we want to use aead_encrypt_out?
| } | ||
| Err(SymmetricCipherError::DecryptionFailed) => { /* also ok */ } | ||
| _ => panic!("Unexpected error"), | ||
| }; |
There was a problem hiding this comment.
For Authenticated Encryption with Associated Data (AEAD), modifying ciphertext with unchanged tag must also fail a tag check. Accepting Ok(_) lets a cipher that never authenticate changed ciphertext pass.
Recommend Err(AEADTagCheckFailed) here, to match AAD- and tag-tamper cases below.
| ciphertext: &[u8], | ||
| tag: &[u8; TAG_LEN], | ||
| ) -> Result<Vec<u8>, SymmetricCipherError>; | ||
| /// A one-shot API to decrypt some ciphertext with the given key. |
There was a problem hiding this comment.
tag: &[u8] drops compile-time length guarantees that aead_encrypt_out and aead_decrypt uses [u8, TAG_LEN] and violates the "prefer &[u8; N]" rule. Also allows tag-truncation forgeries if an implementer compares tag.len() bytes.
Recommend we change to tag: &[u8; TAG_LEN]
| /// typically the ciphertext is the same length as the plaintext, but some ciphers may have an expansion factor or require | ||
| /// extra space for a nonce or tag. | ||
| /// Returns a tuple containing the initialization data and the number of bytes written to the plaintext buffer. | ||
| fn aead_decrypt_out( |
There was a problem hiding this comment.
tag: &[u8] drops compile-time length guarantees that aead_encrypt_out and aead_decrypt uses [u8, TAG_LEN] and violates the "prefer &[u8; N]" rule. Also allows tag-truncation forgeries if an implementer compares tag.len() bytes.
Recommend we change to tag: &[u8; TAG_LEN]
| /// In order for these one-shot APIs to be usable securely in all contexts, the init data will be generated | ||
| /// securely by the block cipher implementation and returned along with the ciphertext, and there is no API for the | ||
| /// user to provide the init data. If you require this functionality, see the documentation for the underlying implementation. | ||
| pub trait BlockCipher<const KEY_LEN: usize, const INIT_DATA_LEN: usize, const BLOCK_LEN: usize>: |
There was a problem hiding this comment.
Design Question: Encryption has do_encrypt_final/_out but decryption has no do_decrypt_final.
Padding modes need a decrypt-side finalization to strip/validate padding. Was this intentional, or should we add it?
There was a problem hiding this comment.
I thought I had added one. Maybe I just thought about adding one. Yes. We will certainly need one.
| ) -> Result<usize, SymmetricCipherError>; | ||
| } | ||
|
|
||
| /// The basic functions of an Authenticated Encryption with Addititional Data cipher. |
There was a problem hiding this comment.
Design question: This forces every AEAD to expose the unauthenticated one-shot encrypt_out/decrypt_out (tag dropped). Problematic for a "fool-proof" API. Should AEAD inherit these, and what are their semantics on an AEAD type?
There was a problem hiding this comment.
You're making an assumption that that'll be unauthenticated 😉
Couldn't an AEAD implement the one-shot encrypt_out by sticking the tag at the end of the ciphertext, and have the corresponding decrypt_out expect that? Like, just silently do the correct AEAD tag stuff without even telling the user that you're doing it.
2 participants
This is prep work for @officialfrancismendoza 's ASCON branch.
These are a first-pass at these traits, and will almost certainly be refined as Francis works through ASCON, and subsequently AES.
Francis, feel free to merge to
release/0.1.2alphaand rebase your ASCON branch on top.