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.

Sign up for GitHub

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

Jump to bottom

Fix CCM mode encryption when length of message > 256 #12364

Merged
merged 2 commits into from
Jan 8, 2020
Merged

Fix CCM mode encryption when length of message > 256 #12364

merged 2 commits into from
Jan 8, 2020

Conversation

mtausig
Copy link
Contributor

@mtausig mtausig commented Oct 3, 2019

Contribution description

Encryption/Decryption using CCM mode did not work when the message had a size that exceeded 256. This patch fixes it

Testing procedure

I was unable to find a public test vector for messages of large size. I have therefore created a new one manually and validated it against two other crypto libraries (BouncyCastle and pycrpytodome)

Issues/PRs references

Fixes part of issue #8107

@mtausig
Copy link
Contributor Author

mtausig commented Oct 3, 2019

Note: This is based on the branch of pull request #12362 to avoid conflicts later on. I will rebase it once that PR is merged.

@aabadie aabadie added Area: crypto Area: Cryptographic libraries Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Oct 3, 2019
fjmolinas
fjmolinas reviewed Dec 17, 2019
View reviewed changes
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find a text vector either so lets go with your manually generated test vector. Other than that just one minor comment where I think an assert could be good.

sys/crypto/modes/ccm.c
@@ -37,7 +37,8 @@ static inline int min(int a, int b)
int ccm_compute_cbc_mac(cipher_t *cipher, const uint8_t iv[16],
const uint8_t *input, size_t length, uint8_t *mac)
{
uint8_t offset, block_size, mac_enc[16] = { 0 };
uint8_t block_size, mac_enc[16] = { 0 };
uint32_t offset;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this limit the message length to 2^32 and isn't the message limit 2^8L so 2^64? I think its OK not to expect messages longer than 2^32 (which is already huge!) buy maybe this should be checked and asserted?

I think the doc should be updated with your changes in #12362 as well but this could be left for a follow up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I change length from size_t to uint32_t? That would make the size limitation obvious and lead to a better defined situation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I think this is something with public API, but I'll handle it in a separate PR. Lets get this fix in, lets wait for #12362 to get in and then we will go with this one as well.

@mtausig mtausig force-pushed the ccm_msglen branch 2 times, most recently from 1f4e154 to fe0beda Compare January 7, 2020 20:27
@fjmolinas
Copy link
Contributor

@mtausig this one needs rebase now.

@mtausig
Copy link
Contributor Author

mtausig commented Jan 7, 2020

Rebased to master

@fjmolinas fjmolinas added CI: run tests If set, CI server will run tests on hardware for the labeled PR Area: build system Area: Build system labels Jan 8, 2020
@fjmolinas
Copy link
Contributor

Ran test locally:

Help: Press s to start test, r to print it is ready
READY
s
START
main(): This is RIOT! (Version: 2020.01-devel-1598-g4428-pr-12364)
..................................
OK (34 tests)

fjmolinas
fjmolinas approved these changes Jan 8, 2020
View reviewed changes
Copy link
Contributor

@fjmolinas fjmolinas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK, tested locally and murdock will do the rest. I would like to see the implementation restrictions reflected in the API, but I would rather leave that for another PR.

@fjmolinas fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 8, 2020
@fjmolinas
Copy link
Contributor

Failing tests seem an issue with the board connected to murodck, locally I can't reproduce:

BUILD_IN_DOCKER=1 RIOT_CI_BUILD=1 BOARD=nrf52dk make -C tests/msg_send_receive/ clean all flash test -j3

Connect to serial port /dev/riot/tty-nrf52dk
Welcome to pyterm!
Type '/exit' to exit.
:kwHelp: Press s to start test, r to print it is ready
Help: Press s to start test, r to print it is ready
Help: Press s to start test, r to print it is ready
READY
s
START
main(): This is RIOT! (Version: buildtest)
Incremented counters to 1 and 1
Incremented counters to 2 and 2
Incremented counters to 3 and 3
Incremented counters to 4 and 4
Incremented counters to 5 and 5
Incremented counters to 6 and 6
Incremented counters to 7 and 7
Incremented counters to 8 and 8
Incremented counters to 9 and 9
Incremented counters to 10 and 10
Test successful.

make: Leaving directory '/home/francisco/workspace/RIOT3/tests/msg_send_receive'

Re-triggering build.

@fjmolinas fjmolinas added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jan 8, 2020
@aabadie
Copy link
Contributor

aabadie commented Jan 8, 2020

All green here!

@aabadie aabadie merged commit a018df6 into RIOT-OS:master Jan 8, 2020
@fjmolinas
Copy link
Contributor

@mtausig thanks for the fix!

@fjmolinas fjmolinas mentioned this pull request Jan 12, 2020
Merged
@fjmolinas fjmolinas added this to the Release 2020.01 milestone Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fjmolinas fjmolinas fjmolinas approved these changes

Assignees
No one assigned
Labels
Area: build system Area: Build system Area: crypto Area: Cryptographic libraries CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2020.01
Development

Successfully merging this pull request may close these issues.
3 participants
@mtausig @fjmolinas @aabadie