Fix issues in Cryptocell 310 ccm_alt discovered by On Target Testing #8704

RonEld · 2018-11-11T09:47:21Z

Description

Fix issues discovered by On Target Testing on the NRF52840_DK platform:
~~1. Check add len is not too big.~~
2. Fix memory overflow, by adding a local buffer for the tag result.
3. Return MBEDTLS_ERR_CCM_AUTH_FAILED where needed.
4. Add unsupported functions for CCM*.
5. Zeroize output buffer, upon authentication faliure.
6. Use mbedtls_platform_zeroize() instead of mbedtls_zeroize()

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

RonEld · 2018-11-11T09:48:54Z

Note: This PR will conflict with #8643

0xc0170 · 2018-11-12T08:57:42Z

Note: This PR will conflict with #8643

What order they should be integrated? I assume first 8643 and this one rebased afterwards?

RonEld · 2018-11-12T09:27:23Z

@0xc0170 Actually they are not dependent on each other.
The conflict will be because #8643 renames error codes returned by the function, while this PR, returns the previous error code, but added some conditions, in the same changed lines.

This PR fixes exiting bugs, while 8643 resolves future issues ( after newer version of Mbed TLS will be merged)
In summary, I would first integrate this PR, and after that rebase 9643
Note that 9643 is not a blocker, as merging Mbed TLS should not break build, as it was first suspected, and was the incentive for that PR

adbridge

There appear to be camelcase variables in this code. This contravenes the mbed-os coding guidelines....

0xc0170 · 2018-11-15T07:38:41Z

There appear to be camelcase variables in this code. This contravenes the mbed-os coding guidelines....

From my review, those are the one from SDK, not our functions, so should be good. @RonEld Please recheck

0xc0170 · 2018-11-15T07:39:24Z

@ARMmbed/mbed-os-crypto Please review the latest update

RonEld · 2018-11-15T08:22:30Z

I wil change all the camelcase variables that are changeable:
CrysRet -> crys_ret
CC_Mac_Res -> cc_mac_res

RonEld · 2018-11-15T09:45:11Z

@0xc0170 @adbridge I have renamed the camelcase variable.
I have also modified the header files and remove the function declarations, as they are now not needed to be declared in the alternative headers

yanesca · 2018-11-15T10:27:03Z

@RonEld The first commit fixes 6 unrelated issues. Could you please split it up to 6 different commits?

0xc0170 · 2018-11-15T16:52:36Z

Updated, @yanesca please rereview

RonEld · 2018-11-15T16:57:43Z

@yanesca I rebased to split the first commit to 6 different commits.
I also amended the "Additional fix for ccm_alt from On Target Testings" commit with the same fix on the encryption path, which was lost among all the merges \ rebases.
I also amended the style fixes commit with removing whitespaces before the opening parenthesis.

yanesca · 2018-11-16T09:21:19Z

Thank you very much, I'll have a look at it as soon as I can!

RonEld · 2018-11-16T10:25:10Z

@yanesca Thanks!
I've been thinking. The function mbedtls_ccm_setkey(), returns MBEDTLS_ERR_AES_FEATURE_UNAVAILABLE for every key size that is not 128 bits long, because Cryptocell310 only supports this key size. I think it is worth returning MBEDTLS_ERR_CCM_BAD_INPUT in invalid key sizes ( such as 224), and return the MBEDTLS_ERR_AES_FEATURE_UNAVAILABLE for 192 and 256 bits size keys. What do you think? If you agree, I will work on this first thing next week, but I don't think it should block this PR

yanesca · 2018-11-16T10:44:25Z

I agree, that would be a nice improvement!

0xc0170 · 2018-11-26T13:24:38Z

@RonEld Please also rebase with the fixing the review above

Once done, we'll start CI

RonEld · 2018-11-26T13:28:23Z

@0xc0170 Sure, no problem.
Since the fixes are style changes, would you like me to squash the fixes with c314242 ?

0xc0170 · 2018-11-26T13:30:55Z

Since the fixes are style changes, would you like me to squash the fixes with c314242 ?

It could be.

1. Change camelcase variables to Mbed OS style. 2. Remove functions declarations from the `_alt` header, since they are now added from the module header regardless whether an alternative implementation exists. 3. Remove the `extern "c"` declaration from the `_alt` headers. 4. Remove whitespaces before opening parenthesis. 5. Fix alignment of function parameters. 6. Fix indentations. 7. Limit lines to 80 characters.

Return `MBEDTLS_ERR_AES_FEATURE_UNAVAILABLE` only for valid key lengths, that are not supported by Cryptocell 310. For other key sizes, return `MBEDTLS_ERR_CCM_BAD_INPUT`

Replace the module specific errors with the `MBEDTLS_ERR_PLATFORM_XXX` errors.

RonEld · 2018-11-26T14:23:54Z

@0xc0170 I have rebased to resolve conflicts, and put fixes from review from @dgreen-arm in 1b34927

yanesca · 2018-11-26T14:38:37Z

@RonEld do you still have the branch before the rebase? Can you post a link to it so that I can compare?

bulislaw · 2018-11-26T14:53:31Z

All the PRs need to be engineering ready (marked as "needs: CI") by the end of the day (Austin time). Otherwise it won't make 5.11 and will need to come in the next release (5.12 for features, 5.11.1 for fixes and new platforms).

RonEld · 2018-11-26T15:04:24Z

@yanesca UnfortunatelyI don't.. I am working on making a reference branch

RonEld · 2018-11-26T15:11:28Z

@yanesca I believe the branch before the rebase and style fixes from @dgreen-arm is in https://github.com/RonEld/mbed-os/tree/ccm_alt_reference_brach

yanesca

I have reviewed the rebase and I am happy with the changes.

(@RonEld thanks for putting up a reference branch!)

mbed-ci · 2018-11-27T01:52:12Z

Test run: SUCCESS

Summary: 4 of 4 test jobs passed
Build number : 6
Build artifacts
Build logs

cmonr · 2018-11-27T02:03:19Z

@ARMmbed/mbed-os-maintainers IF we're ignoring the jenkins_ci/exporter result (the export nodes are currently completely out of space and failing), then this PR is good to come in.

0xc0170 · 2018-11-27T08:19:29Z

Thanks @cmonr for heads-up! This is ready for merge

0xc0170 added the needs: review label Nov 12, 2018

0xc0170 requested a review from a team November 12, 2018 08:58

adbridge requested changes Nov 14, 2018

View reviewed changes

adbridge added needs: work release-version: 5.10.4 and removed needs: review labels Nov 14, 2018

RonEld added needs: review and removed needs: work labels Nov 15, 2018

adbridge approved these changes Nov 15, 2018

View reviewed changes

RonEld force-pushed the cryptocell_ccm_alt_fixes branch 5 times, most recently from 77d64ef to ce86f0f Compare November 15, 2018 16:27

0xc0170 approved these changes Nov 15, 2018

View reviewed changes

0xc0170 requested a review from a team November 15, 2018 16:53

cmonr removed the release-version: 5.10.4 label Nov 15, 2018

0xc0170 added the release-version: 5.11.0-rc1 label Nov 26, 2018

Ron Eldor added 3 commits November 26, 2018 15:32

Return MBEDTLS_ERR_CCM_BAD_INPUT on invalid key

5ed3a64

Return `MBEDTLS_ERR_AES_FEATURE_UNAVAILABLE` only for valid key lengths, that are not supported by Cryptocell 310. For other key sizes, return `MBEDTLS_ERR_CCM_BAD_INPUT`

Add the platform generic errors

5e0223f

Replace the module specific errors with the `MBEDTLS_ERR_PLATFORM_XXX` errors.

RonEld force-pushed the cryptocell_ccm_alt_fixes branch from b6b1e49 to 5e0223f Compare November 26, 2018 14:23

dgreen-arm approved these changes Nov 26, 2018

View reviewed changes

RonEld added needs: review and removed needs: work labels Nov 26, 2018

yanesca approved these changes Nov 26, 2018

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels Nov 26, 2018

cmonr added the risk: A label Nov 26, 2018

cmonr added risk: G and removed risk: A labels Nov 27, 2018

0xc0170 added ready for merge and removed needs: CI labels Nov 27, 2018

0xc0170 merged commit 86915d9 into ARMmbed:master Nov 27, 2018

0xc0170 removed the ready for merge label Nov 27, 2018

cmonr removed the risk: G label Nov 27, 2018

Fix issues in Cryptocell 310 ccm_alt discovered by On Target Testing #8704

Fix issues in Cryptocell 310 ccm_alt discovered by On Target Testing #8704

Uh oh!

Conversation

RonEld commented Nov 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Uh oh!

RonEld commented Nov 11, 2018

Uh oh!

0xc0170 commented Nov 12, 2018

Uh oh!

RonEld commented Nov 12, 2018

Uh oh!

adbridge left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Nov 15, 2018

Uh oh!

0xc0170 commented Nov 15, 2018

Uh oh!

RonEld commented Nov 15, 2018

Uh oh!

RonEld commented Nov 15, 2018

Uh oh!

yanesca commented Nov 15, 2018

Uh oh!

0xc0170 commented Nov 15, 2018

Uh oh!

RonEld commented Nov 15, 2018

Uh oh!

yanesca commented Nov 16, 2018

Uh oh!

RonEld commented Nov 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yanesca commented Nov 16, 2018

Uh oh!

0xc0170 commented Nov 26, 2018

Uh oh!

RonEld commented Nov 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Nov 26, 2018

Uh oh!

RonEld commented Nov 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yanesca commented Nov 26, 2018

Uh oh!

bulislaw commented Nov 26, 2018

Uh oh!

RonEld commented Nov 26, 2018

Uh oh!

RonEld commented Nov 26, 2018

Uh oh!

yanesca left a comment

Choose a reason for hiding this comment

Uh oh!

mbed-ci commented Nov 27, 2018

Test run: SUCCESS

Uh oh!

cmonr commented Nov 27, 2018

Uh oh!

0xc0170 commented Nov 27, 2018

Uh oh!

Uh oh!

RonEld commented Nov 11, 2018 •

edited

Loading

RonEld commented Nov 16, 2018 •

edited

Loading

RonEld commented Nov 26, 2018 •

edited

Loading

RonEld commented Nov 26, 2018 •

edited

Loading