Feat: Implement ASCON Lightweight Cryptographic Libraries - #21
Conversation
|
@officialfrancismendoza I see this. On my todo list! |
29dadd5 to
7120e4a
Compare
…d build workspace (bcgit#45)
… much easier review
…Updated summary.md as well
…ulting from changes in core that I have to build around
937be59 to
0f15f1f
Compare
…llible (guards absorb after squeeze), AEAD conformed to main SymmetricCipher and AEADCipher in release/0.1.2.alpha, use Suspendable for hash/XOF/CXOF and SuspendableKeyed for AEAD (bcgit#21)
…llible (guards absorb after squeeze), AEAD conformed to main SymmetricCipher and AEADCipher in release/0.1.2.alpha, use Suspendable for hash/XOF/CXOF and SuspendableKeyed for AEAD (bcgit#21)
|
@officialfrancismendoza, a couple things on this one so far:
|
There was a problem hiding this comment.
First review:
This PR is currently failing the rustfmt check. Just run $ cargo fmt in the root and commit again.
The Crucible project only has tests for ML-KEM and ML-DSA. So does not apply here.
There are ascon vectors in both Wycheproof and bc-test-data, so you should be able to take the test harnesses that we have in, for example, ml-dsa and port that over for ascon.
(I will continue with a code review, though I admit that +3,900 is a lot to review, so I will have to do this in chunks.)
…sts' of github.com:officialfrancismendoza/bc-rust into officialfrancismendoza-feature/officialfrancismendoza/45-githu
…a/15-ASCON' of github.com:officialfrancismendoza/bc-rust
| @@ -0,0 +1,262 @@ | |||
| # ASCON Implementation & Testing — Work Summary | |||
There was a problem hiding this comment.
This file seems maybe like it is your personal notes that were checked in by accident? I don't see any long-term value in including this file in the source code.
| //! [`bouncycastle_utils::secret::Secret`] wrappers, so they are scrubbed with volatile writes | ||
| //! when the value is dropped. The hash/XOF states are likewise `Secret`-wrapped. | ||
| //! - **Decryption release:** never release decrypted plaintext until finalization returns `Ok`; an | ||
| //! `Err(AuthenticationFailed)` means the ciphertext or tag was tampered with. |
There was a problem hiding this comment.
I like this point and I think it should be expanded. I'm thinking something like:
Decryption tag check failure: a ciphertext decryption whose finalization returns an
Err(AuthenticationFailed)should be considered to be tampered with and the entire plaintext should be rejected. For example, if the plaintext being decrypted is large enough that it must be processed by the application in a streaming fashion, the application should have a way to cancel the operation or transaction with an error if ASCON finalization returns an AuthenticationFailed error.
| //! h.update_bytes(b"hello "); | ||
| //! h.update_bytes(b"world"); | ||
| //! let mut out = [0u8; 32]; | ||
| //! h.do_final_into(&mut out); |
There was a problem hiding this comment.
The rest of the library uses do_final_out() for this pattern. I actually kinda like _into instead of _out, but one way or another this should be made consistent with the rest of the library.
If you think that _into is the better name, than why don't you open a github issue to propose that change, and then create a PR against 0.1.3alpha.
| //! ``` | ||
| //! use bouncycastle_ascon::ascon_aead128::AsconAead128; | ||
| //! | ||
| //! let key = [0u8; 16]; |
There was a problem hiding this comment.
Why is this not a KeyMaterial?
| //! | ||
| //! let mut ct = vec![0u8; plaintext.len() + 16]; // ciphertext || 16-byte tag | ||
| //! let n = AsconAead128::encrypt(&key, &nonce, Some(ad), plaintext, &mut ct); | ||
| //! ct.truncate(n); |
There was a problem hiding this comment.
This API pattern seems extremely weird to me.
First the user has to create a mut vec of the correct size plaintext.len() + 16.
Then the user has to hand that in to the encrypt function.
Then the user has to truncate it because it might actually be shorter than plaintext.len() + 16 ?
I might expect some acrobatics like that in no_std where the user has to hand in a [u8; N] of the correct size, but the whole point of Vec's is to avoid making the user deal with array lengths.
If we're using vec here, then can't we hide this inside the function call and -> Vec<u8> that we size correctly?
| /// Algorithm name for Ascon-XOF128. | ||
| pub const ASCON_XOF128_NAME: &str = "Ascon-XOF128"; | ||
| /// Algorithm name for Ascon-CXOF128. | ||
| pub const ASCON_CXOF128_NAME: &str = "Ascon-CXOF128"; |
There was a problem hiding this comment.
All the ASCON variants should impl traits::Algorithm and maybe traits::HashAlgParams?
| //! These replace the external `arrayref` crate so that this crate carries no third-party runtime | ||
| //! dependencies (per the project's QUALITY_AND_STYLE rules). All callers pass slices that are at | ||
| //! least 8 bytes long at the given offset, so `copy_from_slice` is infallible by construction and | ||
| //! no fallible conversion is involved. |
There was a problem hiding this comment.
Claude likes to leave comments like these explaining its reasoning. That's fine for you to understand what it's done, but should be deleted before committing.
| fn pad(i: usize) -> u64 { | ||
| debug_assert!(i < 8); | ||
| 0x01u64 << (i * 8) | ||
| } |
There was a problem hiding this comment.
I assume that this function should correspond to Algorithm 2 in SP.800-232.
I would like you to use a commenting style similar to what I have done in ML-DSA to make it clear how our code corresponds with the sample code in SP.800-232.
For example:
https://github.com/bcgit/bc-rust/blob/main/crypto/mldsa/src/aux_functions.rs#L15
Moreover, for larger functions, I actually comment line-by-line with the corresponding line numbers in the FIPS sample algorithms to make the correspondence super clear, especially if our code is skipping steps or doing steps out-of-order.
Some examples:
https://github.com/bcgit/bc-rust/blob/main/crypto/mldsa/src/aux_functions.rs#L435
https://github.com/bcgit/bc-rust/blob/main/crypto/mldsa/src/mldsa.rs#L929
This also provides a way to note any deviations that we are making from the FIPS, which will make certification easier.
Example:
https://github.com/bcgit/bc-rust/blob/main/crypto/mldsa/src/mldsa_keys.rs#L665
But actually, looking closer, this does not appear to be the same as Algorithm 2, this is generating the padding string 1 || 0^j but not performing the concatenation. So then it is confusing to give it the same name as Alg 2.
Possibly, this indicates that in fact this code does not correspond directly to the FIPS sample algorithms and we should do a refactor so that it does?
| 0x00FFFFFFFFFFFFFF | ||
| } else { | ||
| 0x00FFFFFFFFFFFFFF >> (56 - final_bits) | ||
| }; |
There was a problem hiding this comment.
This function exists verbatim in both ascon_xof128.rs and ascon_cxof128.rs. That means that some de-duplication should be done.
I would suggest doing some re-structuring of this code to better match the structure suggested by SP.800-232:
- Have an
aux_functions.rsthat corresponds to SP.800-232 section 2.1 which includes Algorithm 1 parse(X, r) and Algorithm 2 pad(X, r), and then also serves as a logical place to put other code that will be common to the various parts of ACSON. - Maybe create
permutation.rsthat corresponds with SP.800-232 and contains the implementations of 𝐴𝑠𝑐𝑜𝑛-𝑝[8] and 𝐴𝑠𝑐𝑜𝑛-𝑝[12]. I would expect to see this as a single parametrized function to avoid duplicate code, reduce code-review burden, and reduce number of tests needed.
| let n = cipher.encrypt_update(plaintext, ciphertext); | ||
| let written = cipher.encrypt_finalize(&mut ciphertext[n..]); | ||
| Ok((nonce, n + written)) | ||
| } |
There was a problem hiding this comment.
I assume this encrypt_out function is supposed to correspond to Algorithm 3 Ascon-AEAD128.enc(𝐾, 𝑁 , 𝐴, 𝑃 ) from SP.800-232?
A few things that catch my eye on a first look:
- This should match exactly the API of Algorithm 3, including the name
encenc_outinstead ofencrypt/encrypt_out. I would also make the names of the inputsK, N, A, Pmatch exactly. - A header comment in the same style as, for example, this indicating that this is the implementation of Algorithm 3.
- Algorithm 3 has 4 inputs, but this only has 3. I see that you are not allowing the user to provide the nonce, but instead you are forcing them to use our
HashDRBG_SHA512::new_from_os()(via theSelf::fresh_nonce()call). This will be problematic, for example, in environments that can't use our RNG and need to connect in for example a hardward RNG. I think we should match exactly the API of FIPS Algorithm 3. If you want, you could do
N: Option<&[u8; NONCE_LEN]>(or maybe that should be a KeyMaterial?) and if it's None then we can generate it for the user. I've done similar patterns in ML-DSA / ML-KEM.
You should also provide anenc_rngsimilar to what I've done with MLKEM.encaps_rng that takes an&mut dyn RNGand uses that for sourcing entropy. - The name of the constant
CRYPTO_KEYBYTESis confusing. I think this should beKEY_LENto be consistent with other similar constants across the library (I use "LEN" for a length in bytes, and "SIZE" for lengths in bits. This is described in the QUALITY_AND_STYLE.md. Same goes forCRYPTO_ABYTES. - At a quick glance, the body of this function does not look anything like the the body of Algorithm 3; I think because you have chopped up Algorithm 3 into a new(), update(), final() streaming pattern, which is fine, but you'll need to heavily comment all of the involved functions to make the correspondence with the FIPS Algorithms clear.
- By the way, the rest of the library uses
_final()not_finalize(). - Algorithm 3 returns Output: ciphertext 𝐶, 128-bit tag 𝑇. You are returning the nonce, and it doesn't look like you're returning the tag at all. This doesn't make sense to me.
Description
Implementation of NIST SP 800-232 ASCON lightweight cryptography standards for constrained devices (#22), including:
Note: due to the important considerations of embedded systems, this PR must follow, where applicable, utilization of non-heap memory structures (for example: [u8, CONST_SIZE] instead of vec) to respect their compute and RAM limitations.
Type of Change
A full list of detailed changes are outlined in the crate's summary.md file.
Testing
cargo test -p bouncycastle-asconto test ASCON crate and associated KAT test casescargo test --workspacecargo mutants --package bouncycastle-asconfor mutant testsImportant TBDs for testing: