Code for AES-CCM in Hacspec Rust #139

tanmay2004 · 2021-08-25T13:10:29Z

I have worked on the Rust implementation of the AES-CCM algorithm. I've created both the encryption and decryption functions and they can be found in ccm_mode.rs. I've used the specification provided here. The document also provides a few tests at the end which I've included in test_ccm_mode.rs.

Currently, the decryption function is slightly incomplete. The doc asks to RETURN INVALID in a few cases but I didn't know how to do that in Rust. Hopefully, the rest of it is satisfactory.

franziskuskiefer

Thanks for the PR!
Before starting the review of the code we need to fix a couple things.

Please add the new crate to the top level workspace (the Cargo.toml in the project root). Otherwise it won't work.
The code as is doesn't compile. After adding the crate to the workspace it will be tested on CI and we would see this here.

error[E0609]: no field `1` on type `hacspec_aes::Block`
   --> examples/aes-ccm/src/ccm_mode.rs:108:56
    |
108 |     let mut y_curr = (aes128_encrypt_block(key, bloc)).1;
    |                                                        ^ help: a field with a similar name exists: `0`

Let's make sure this actually builds and typechecks before we start looking at the code.

tanmay2004 · 2021-08-29T11:32:30Z

@franziskuskiefer I think I've fixed the issue now. I added aes-ccm to the master Cargo.toml. Also, my code failed on line 108 because I had an older version of aes.rs on my system. I updated it and my code, so it is working now. Please review.

franziskuskiefer

Ok this fixes the build. But we still need to get the typechecker running on CI and the spec passing it. To do that you have to add it to the typecheck_exacmples.sh. Please also run the typechecker locally before to find any issues. There are a number of issues. I mentioned some in this review.
Ping me if you still have issues running the typechecker locally.

Cargo.toml

examples/aes-ccm/src/ccm_mode.rs

Remove while loops

franziskuskiefer

Thanks! We're getting there :)
I didn't look at the actual code yet for correctness. Just code quality and hacspec issues so far.

Please add comments to functions (at the top and within) to explain what the functions are doing.
All the as integer casts need comments explaining why they are safe, or (better) have checks somewhere that they are safe.
I filed Add bitwise operations on Seq to typechecker #143 to track the bitwise operation issue.
To improve test coverage can you add the wycheproof test vectors? You can look at the aes128-gcm test for a template how to read a json test vector like this. https://github.com/google/wycheproof/blob/master/testvectors/aes_ccm_test.json

examples/aes-ccm/src/ccm_mode.rs

franziskuskiefer

Looks like I forgot to hit submit here.
This is only a partial review but there are some todos in here. Please address them while I look at the rest.

franziskuskiefer · 2021-09-07T10:58:25Z

examples/aes-ccm/src/ccm_mode.rs

+use hacspec_aes::*;
+
+pub type AesCcmResult = Result<ByteSeq, u8>;
+pub const INVALID: u8 = 0u8;


I know this isn't done in gcm but you can use enums for errors, which is a little nicer. Something like

#[derive(Debug)] pub enum AesCcmError { Invalid } pub type AesCcmResult = Result<ByteSeq, AesCcmError>;

franziskuskiefer · 2021-09-07T10:59:05Z

examples/aes-ccm/src/ccm_mode.rs

@@ -0,0 +1,210 @@
+use hacspec_lib::*;


Please add a comment on the top with a short description and link to the spec you used (NIST)

franziskuskiefer · 2021-09-07T11:01:20Z

examples/aes-ccm/src/ccm_mode.rs

+pub type AesCcmResult = Result<ByteSeq, u8>;
+pub const INVALID: u8 = 0u8;
+
+fn format_func(a: &ByteSeq, n: &ByteSeq, p: &ByteSeq, t: usize, alen: usize, nlen: usize, plen: usize) -> ByteSeq {


This function is doing a lot without explaining anything. Please add some comments.

franziskuskiefer · 2021-09-07T11:02:12Z

examples/aes-ccm/src/ccm_mode.rs

+
+    k = k + plen;
+
+    for _t in 0..16 {


I think I asked for the comments here before.

franziskuskiefer · 2021-09-07T11:04:07Z

examples/aes-ccm/src/ccm_mode.rs

+}
+
+pub fn encrypt_ccm(a: ByteSeq, n: ByteSeq, pay: ByteSeq, key: Key128, tlen: usize) -> ByteSeq {
+    let key_copy = key.clone();


You don't need to have the copy here. Just clone when calling the function that needs a clone.

It wasn't working that way, because I'm using the key multiple times in a loop for encrypting each block one by one. I had tried many variations of the cloning for the key and the tests ran successfully but the typechecker failed for all my attempts other than the way I've done it right now.

franziskuskiefer · 2021-09-07T11:05:00Z

examples/aes-ccm/src/ccm_mode.rs

+    (s0, s)
+}
+
+pub fn encrypt_ccm(a: ByteSeq, n: ByteSeq, pay: ByteSeq, key: Key128, tlen: usize) -> ByteSeq {


Single character variable names are always a bad idea. Pleas use variable names that tell the reader what they are looking at.

franziskuskiefer · 2021-09-07T11:05:38Z

examples/aes-ccm/src/ccm_mode.rs

+
+pub fn encrypt_ccm(a: ByteSeq, n: ByteSeq, pay: ByteSeq, key: Key128, tlen: usize) -> ByteSeq {
+    let key_copy = key.clone();
+    let plen = pay.len();


So here you get the lengths that you convert later to u8. But you never check that they actually fit in u8. All casts to smaller types require explicit checks. (The typechecker also tells you where they are and warns you about them.)

franziskuskiefer

Generally looks good. The most important points are still the readability, i.e. adding comments and using speaking variable names.
I'll see that we get the typechecker fixed for this to pass.

franziskuskiefer · 2021-09-17T10:52:58Z

examples/aes-ccm/src/ccm_mode.rs

+pub type AesCcmResult = Result<ByteSeq, u8>;
+pub const INVALID: u8 = 0u8;
+
+fn format_func(a: &ByteSeq, n: &ByteSeq, p: &ByteSeq, t: usize, alen: usize, nlen: usize, plen: usize) -> ByteSeq {


Why do you pass in the lengths here? You can just get them from the ByteSeq input.
I'd expect this to be something like

fn format_func(aad: &ByteSeq, nonce: &ByteSeq, plaintext: &ByteSeq, tag_len: usize) -> ByteSeq {

franziskuskiefer · 2021-09-17T11:14:04Z

examples/aes-ccm/tests/test_ccm_mode.rs

+            continue;
+        };
+        if testGroup.tagSize < 32 {
+            println!("MAC lengths < 32 are not allowed for AES CCM.");


When they are not allowed this shouldn't happen, right? So can you just add an assert?

franziskuskiefer · 2021-09-17T11:23:14Z

examples/aes-ccm/src/ccm_mode.rs

+    b
+}
+
+fn get_t(b: &ByteSeq, key: Key128, num: usize) -> ByteSeq {


Using num here for tlen is pretty confusing.

franziskuskiefer · 2021-10-25T17:03:01Z

Hi @tanmay2004 what's the state of this PR? I think most typechecker related issues that we ran into here should be fixed by now.

Add files via upload

3cb8cf0

franziskuskiefer self-requested a review August 25, 2021 14:34

franziskuskiefer assigned tanmay2004 Aug 25, 2021

franziskuskiefer added the spec 📜 A new hacspec label Aug 25, 2021

franziskuskiefer suggested changes Aug 25, 2021

View reviewed changes

tanmay2004 added 2 commits August 29, 2021 15:25

Update Cargo.toml

4e97067

Update ccm_mode.rs

40644ae

tanmay2004 requested a review from franziskuskiefer August 29, 2021 11:32

franziskuskiefer suggested changes Aug 30, 2021

View reviewed changes

tanmay2004 added 10 commits August 30, 2021 14:03

Update ccm_mode.rs

fd78f72

Update test_ccm_mode.rs

09908ad

Add aes-ccm

a2ded10

Update ccm_mode.rs

1d05f7b

Remove while loops

Remove try_into usage

348cf27

Update ccm_mode.rs

ad86b35

Remove break statements

40ce5e9

Update ccm_mode.rs

29e22be

Merge branch 'master' into master

65cf75e

Update ccm_mode.rs

2d4d4b2

tanmay2004 requested a review from franziskuskiefer September 1, 2021 12:16

franziskuskiefer mentioned this pull request Sep 1, 2021

Add bitwise operations on Seq to typechecker #143

Closed

franziskuskiefer suggested changes Sep 1, 2021

View reviewed changes

tanmay2004 added 5 commits September 2, 2021 21:50

Update ccm_mode.rs

c883a93

Update typecheck_examples.sh

804c169

Merge branch 'master' into master

a639218

Update test_ccm_mode.rs

1344fe6

Fix decrypt_ccm

2ac25ef

tanmay2004 requested a review from franziskuskiefer September 4, 2021 08:34

Add wycheproof tests

691b1a3

tanmay2004 added 2 commits September 7, 2021 09:49

Use usize more

2313593

Update Cargo.toml

4454f18

franziskuskiefer reviewed Sep 16, 2021

View reviewed changes

franziskuskiefer reviewed Sep 17, 2021

View reviewed changes

franziskuskiefer mentioned this pull request Sep 17, 2021

Type inference error with external types #152

Open

Merge branch 'master' into master

49a3e48

franziskuskiefer marked this pull request as draft February 15, 2022 13:41

franziskuskiefer added the inactive Status: Inactive and waiting on the author. label Oct 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code for AES-CCM in Hacspec Rust #139

Code for AES-CCM in Hacspec Rust #139

tanmay2004 commented Aug 25, 2021

franziskuskiefer left a comment

tanmay2004 commented Aug 29, 2021

franziskuskiefer left a comment

franziskuskiefer left a comment

franziskuskiefer left a comment

franziskuskiefer Sep 7, 2021

franziskuskiefer Sep 7, 2021

franziskuskiefer Sep 7, 2021

franziskuskiefer Sep 7, 2021

franziskuskiefer Sep 7, 2021

tanmay2004 Sep 16, 2021 •

edited

franziskuskiefer Sep 7, 2021

franziskuskiefer Sep 7, 2021

franziskuskiefer left a comment

franziskuskiefer Sep 17, 2021

franziskuskiefer Sep 17, 2021

franziskuskiefer Sep 17, 2021

franziskuskiefer commented Oct 25, 2021

Code for AES-CCM in Hacspec Rust #139

Are you sure you want to change the base?

Code for AES-CCM in Hacspec Rust #139

Conversation

tanmay2004 commented Aug 25, 2021

franziskuskiefer left a comment

Choose a reason for hiding this comment

tanmay2004 commented Aug 29, 2021

franziskuskiefer left a comment

Choose a reason for hiding this comment

franziskuskiefer left a comment

Choose a reason for hiding this comment

franziskuskiefer left a 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

tanmay2004 Sep 16, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franziskuskiefer left a 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

franziskuskiefer commented Oct 25, 2021

tanmay2004 Sep 16, 2021 •

edited