Skip to content

Fix issues in Cryptocell 310 cc_internal discovered by On Target Testing #8797

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
Merged
merged 1 commit into from
Nov 30, 2018
Merged

Fix issues in Cryptocell 310 cc_internal discovered by On Target Testing #8797

merged 1 commit into from
Nov 30, 2018

Conversation

RonEld
Copy link
Contributor

@RonEld RonEld commented Nov 19, 2018

Description

Return MBEDTLS_ERR_ECP_INVALID_KEY when Cryptocell returns
CRYS_ECPKI_BUILD_KEY_INVALID_PRIV_KEY_SIZE_ERROR
or CRYS_ECPKI_BUILD_KEY_INVALID_PUBL_KEY_SIZE_ERROR.
This was discovered after new negative tests were added to the tests.

Pull request type

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

@0xc0170 0xc0170 requested a review from a team November 19, 2018 13:08
@0xc0170
Copy link
Contributor

0xc0170 commented Nov 26, 2018

@RonEld @ARMmbed/mbed-os-crypto Review please. 2 lines fix, should be quick

@yanesca
Copy link
Contributor

yanesca commented Nov 26, 2018 •
edited
Loading

@RonEld Could you please explain why is this change necessary? What are those negative tests testing? Which functions return these error codes and in what situation?

@RonEld
Copy link
Contributor Author

RonEld commented Nov 26, 2018

@yanesca The failed tests are the "ECDSA zero private parameter" tests, which CC returned CRYS_ECPKI_BUILD_KEY_INVALID_PRIV_KEY_SIZE_ERROR, which needed to be translated to MBEDTLS_ERR_ECP_INVALID_KEY.

@yanesca
Copy link
Contributor

yanesca commented Nov 26, 2018

Could you please add this information to the commit message?

@RonEld
Copy link
Contributor Author

RonEld commented Nov 26, 2018

@yanesca OK, but isn't the current commit message enough?

Fix issues in CC310 cc_internal discovered by OTT
c948eaa 
Return `MBEDTLS_ERR_ECP_INVALID_KEY` when Cryptocell returns
`CRYS_ECPKI_BUILD_KEY_INVALID_PRIV_KEY_SIZE_ERROR`
or `CRYS_ECPKI_BUILD_KEY_INVALID_PUBL_KEY_SIZE_ERROR`,
When the key size is invalid. Found by the "ECDSA zero private parameter"
tests.
@RonEld RonEld force-pushed the cryptocell_ecc_errors_alt_fixes branch from a447f6c to c948eaa Compare November 26, 2018 13:09
@RonEld
Copy link
Contributor Author

RonEld commented Nov 26, 2018

@yanesca I amended the commit message with this information

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 26, 2018

@RonEld Assuming only fix for 5.11?

@RonEld
Copy link
Contributor Author

RonEld commented Nov 26, 2018

@0xc0170 If you mean that this PR is ready for review, then yes

@yanesca
Copy link
Contributor

yanesca commented Nov 26, 2018

@RonEld I thought that the original commit message wasn't enough, because it didn't give context to the change. Now I can ask you about the context, but if somebody later reads your commit this might not be an option and even it is, it is uncomfortable for both of you.

yanesca
yanesca reviewed Nov 26, 2018
View reviewed changes
Copy link
Contributor

@yanesca yanesca left a comment

Choose a reason for hiding this comment

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

Thanks @RonEld, it looks good to me!

(I asked for a second review, and will approve after it is done.)

gilles-peskine-arm
gilles-peskine-arm reviewed Nov 26, 2018
View reviewed changes
features/cryptocell/FEATURE_CRYPTOCELL310/cc_internal.c
@@ -143,6 +143,8 @@ int convert_CrysError_to_mbedtls_err( CRYSError_t Crys_err )
case CRYS_ECPKI_GEN_KEY_INVALID_PRIVATE_KEY_PTR_ERROR:
case CRYS_ECPKI_EXPORT_PUBL_KEY_INVALID_PUBL_KEY_DATA_ERROR:
case CRYS_ECPKI_BUILD_KEY_INVALID_PRIV_KEY_DATA_ERROR:
case CRYS_ECPKI_BUILD_KEY_INVALID_PRIV_KEY_SIZE_ERROR:
case CRYS_ECPKI_BUILD_KEY_INVALID_PUBL_KEY_SIZE_ERROR:
Copy link

Choose a reason for hiding this comment

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

What about CRYS_ECPKI_BUILD_KEY_INVALID_PUBL_KEY_DATA_ERROR and other CRYS_ECPKI_.*_INVALID_.*_KEY_.*_ERROR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add these, but since these aren't returned in any of our tests, there will be a full list of tests to add.

Copy link

Choose a reason for hiding this comment

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

Ok, so good enough for now.

@bulislaw
Copy link
Member

bulislaw commented Nov 26, 2018 •
edited
Loading

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).

@cmonr cmonr added the risk: A label Nov 26, 2018
@cmonr
Copy link
Contributor

cmonr commented Nov 26, 2018

CI started.

@cmonr cmonr added risk: G and removed risk: A labels Nov 27, 2018
@RonEld
Copy link
Contributor Author

RonEld commented Nov 27, 2018

@0xc0170 The CI failure doesn't seem to be related to the change.
Could you please restart CI?

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 27, 2018

That is correct, there was a bug in CI that was recently fixed.

As we got some important features to land still, can this be postponed to 5.11.1 ? I'll leave it for 5.11rc1 but won't restart CI until some other PR are ready/or CI is free

@0xc0170 0xc0170 added risk: A and removed risk: G labels Nov 27, 2018
@RonEld
Copy link
Contributor Author

RonEld commented Nov 27, 2018

As we got some important features to land still, can this be postponed to 5.11.1 ?

I think so. Since this PR is basically translating one error to a different error, for the sake of tests passing, I think we can postpone to 5.11.1 . If it won't make it to 5.11 , I will work on adding the updates as commented by @gilles-peskine-arm

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 27, 2018

Thanks @RonEld , that helps.

Moved to 5.11.1

@0xc0170 0xc0170 removed the risk: A label Nov 27, 2018
@RonEld
Copy link
Contributor Author

RonEld commented Nov 29, 2018

Fo rthe record: I have been looking at other error codes that can be returned from the underlying driver, related to key, and couldn't find those that can be returned by th API that is called in ecdh and ecdsa, that is likely to be returned. ( It doesn't mean there aren't any...)

@cmonr
Copy link
Contributor

cmonr commented Nov 29, 2018

java.nio.file.FileSystemException: /builds/ws/mbed-os-ci_build-ARM: No space left on device

Will restart when able.

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 30, 2018

CI restarted

@mbed-ci
Copy link

mbed-ci commented Nov 30, 2018

Test run: SUCCESS

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

@cmonr cmonr merged commit 79da14d into ARMmbed:master Nov 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cmonr cmonr cmonr approved these changes

@yanesca yanesca yanesca approved these changes

@gilles-peskine-arm gilles-peskine-arm gilles-peskine-arm approved these changes

Assignees
No one assigned
Labels
release-version: 5.11.1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@RonEld @0xc0170 @yanesca @bulislaw @cmonr @mbed-ci @gilles-peskine-arm