Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix AES-CCM* to work with a_len up to 0xfeff and m_len up to 0xffff #1170

Merged
merged 14 commits into from Jan 9, 2020

Conversation

@simonduq
Copy link
Member

simonduq commented Jan 4, 2020

This PR fixes the AES-CCM* library for payloads/headers beyond 240 bytes (current implem is for up to 255 but ends in infinite loop for m_len greater than 240).

The new limit for a_len is 0xfeff and for m_len 0xffff.
This is useful for e.g. use of AES-CCM in upper layers, or for 802.15.4g with larger frames.
Also adds relevant unit tests.

This is contributed by Yanzi Networks AB (www.yanzi.se).

Copy link
Member

yatch left a comment

Congrats!! I'm looking forward to using this update with large frames 👍 Leaving minor comments.

@@ -42,64 +42,68 @@
#include "lib/aes-128.h"
#include <string.h>

/* see RFC 3610 */
#define CCM_STAR_AUTH_FLAGS(Adata, M) ((Adata ? (1u << 6) : 0) | (((M - 2u) >> 1) << 3) | 1u)
/* As per RFC 3610. M == 2 (m_len is two bytes long) */

This comment has been minimized.

Copy link
@yatch

yatch Jan 4, 2020

Member

Here, M == 2 should be L == 2??

[Section 2, RFC 3610]

       Name  Description                               Size    Encoding
       ----  ----------------------------------------  ------  --------
       M     Number of octets in authentication field  3 bits  (M-2)/2
       L     Number of octets in length field          3 bits  L-1

This comment has been minimized.

Copy link
@yatch

yatch Jan 4, 2020

Member

And, M is always 2 in this implementation, which I'd mention here as well.

This comment has been minimized.

Copy link
@simonduq

simonduq Jan 6, 2020

Author Member

Here, M == 2 should be L == 2??

Good point, fixing the comment

And, M is always 2 in this implementation, which I'd mention here as well.

Hmm I don't think so: M is the MIC len, right?

This comment has been minimized.

Copy link
@yatch

yatch Jan 7, 2020

Member

I guess I was confused. Ignore the second point. Sorry for that (>_<)

uint8_t i;

set_iv(x, CCM_STAR_AUTH_FLAGS(a_len, mic_len), nonce, m_len);

This comment has been minimized.

Copy link
@yatch

yatch Jan 4, 2020

Member

Would it be nicer to check if the given mic_len is valid?

[Section 2, RFC 3610]

   For the generic CCM mode there are two parameter choices.  The first
   choice is M, the size of the authentication field.  .... (snip) ...
   Valid values are 4, 6, 8, 10, 12, 14, and 16 octets.  The second

This comment has been minimized.

Copy link
@simonduq

simonduq Jan 6, 2020

Author Member

agree

This comment has been minimized.

Copy link
@simonduq

simonduq Jan 6, 2020

Author Member

done

#include <stdint.h>

int hexconv_hexlify(const uint8_t *data, int data_len,
char *text, int text_size);

This comment has been minimized.

Copy link
@yatch

yatch Jan 4, 2020

Member

Indentation

This comment has been minimized.

Copy link
@simonduq

simonduq Jan 6, 2020

Author Member

fixed

@@ -0,0 +1,2 @@
To generate test vectors, run:
./generate-test-vectors.py > test-vectors.c

This comment has been minimized.

Copy link
@yatch

yatch Jan 4, 2020

Member

Add

pip install pycryptodome

or something to resolve the dependency.

This comment has been minimized.

Copy link
@simonduq

simonduq Jan 6, 2020

Author Member

Did that. Also, switched to python3

This comment has been minimized.

Copy link
@yatch

yatch Jan 7, 2020

Member

👍 I confirmed the code actually was able to run with Python 2 and Python 3 :-)

#include <stdio.h>
#include <stdbool.h>

#define MICLEN 8

This comment has been minimized.

Copy link
@yatch

yatch Jan 4, 2020

Member

Can we have tests for MICLEN == 4 and MICLEN == 16, which can be used in IEEE 802.15.4 as well (Table 9-4, IEEE 802.15.4-2015)

if [ -z $TIMEOUT ]; then
printf "FAIL (%u seconds)\n" $SECONDS | tee -a $BASENAME.testlog;
else
printf "FAIL (%u seconds timeout)\n" $TIMEOUT | tee -a $BASENAME.testlog;

This comment has been minimized.

Copy link
@yatch

yatch Jan 4, 2020

Member

This works perfectly... I confirmed with the commit of 84e07a9. This is great!

-- Starting test 07-aes-ccm
* clean ...                                                           OK   (6 seconds)
* compile ...                                                         OK   (20 seconds)
* start ./test-aesccm.native ...                                      OK   (0/30 seconds)
* run ./test-aesccm.native ...                                        FAIL (120 seconds timeout)
* run ./test-aesccm.native ...                                        SKIP
-- Killing background jobs
kill -9 1827
../utils.sh: line 41:  1827 Killed                  $TEST &> $RUNLOG

This comment has been minimized.

Copy link
@simonduq

simonduq Jan 6, 2020

Author Member

Oh yes we've been using that one intensively :D

This comment has been minimized.

Copy link
@simonduq

simonduq Jan 6, 2020

Author Member

Though just spotted a buggy log output: the SKIP line should have been for "check" not for "run". Committed a fix for that

This comment has been minimized.

Copy link
@yatch

yatch Jan 7, 2020

Member

👍

@yatch
yatch approved these changes Jan 7, 2020
Copy link
Member

yatch left a comment

Looks good to me. I will wait for a couple of days before merging this.

if [ -z $TIMEOUT ]; then
printf "FAIL (%u seconds)\n" $SECONDS | tee -a $BASENAME.testlog;
else
printf "FAIL (%u seconds timeout)\n" $TIMEOUT | tee -a $BASENAME.testlog;

This comment has been minimized.

Copy link
@yatch

yatch Jan 7, 2020

Member

👍

@simonduq

This comment has been minimized.

Copy link
Member Author

simonduq commented Jan 8, 2020

Thanks. Just pushed another minor thing.

@yatch
yatch approved these changes Jan 9, 2020
@yatch yatch merged commit b991985 into contiki-ng:develop Jan 9, 2020
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.