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 Failure in Cryptocell tests on Montgomrery curve #11512

Merged
merged 1 commit into from Oct 15, 2019

Conversation

@RonEld
Copy link
Contributor

RonEld commented Sep 18, 2019

Description

After the addition of ECDH tests for curve 25519, they failed for Cryptocell, because of different endianity of the input keys and outpu calulated secret. This fix changes the input and output to correct endianity as expected by Mbed TLS and Cryptocell.

This was tested with test_suite_ecdh on target NRF52840_DK

Pull request type

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

Reviewers

@ARMmbed/mbed-os-crypto

Release Notes

Change the order of the input keys and output secret given and
returned from the CC API, to address correct endianity.
@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Sep 18, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

ciarmcom commented Sep 18, 2019

@RonEld, thank you for your changes.
@ARMmbed/mbed-os-crypto @ARMmbed/mbed-os-maintainers please review.

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Sep 24, 2019

@ARMmbed/mbed-os-crypto Please review

@adbridge

This comment has been minimized.

Copy link
Contributor

adbridge commented Sep 27, 2019

@ARMmbed/mbed-os-crypto Please review

bump

Copy link
Contributor

k-stachowiak left a comment

It looks like the general algorithm has been maintained, but I don't understand one of the conversions' direction. @RonEld could you please clarify that for me?

ecdhParams->privKey, CURVE_25519_KEY_SIZE ,
ecdhParams->pubKey, CURVE_25519_KEY_SIZE ,
&ecdhParams->kgTempData ) );
if ( ret != 0 )
{
goto cleanup;
}
ret = convert_CrysError_to_mbedtls_err(
CRYS_COMMON_ConvertLswMswWordsToMsbLsbBytes( secret,

This comment has been minimized.

Copy link
@k-stachowiak

k-stachowiak Sep 30, 2019

Contributor

Shouldn't this conversion go the other way? We have converted the private and public key to the MsbLsbBytes so that CRYS_ECMONT_Scalarmult() can operate on them, but I would expect the output temp_buf to still be in that encoding. If we now want to operate on it further, outside the Cryptocell world, I'd expect the temp_buf to be converted to the LswMswWords again here.

This comment has been minimized.

Copy link
@RonEld

RonEld Oct 2, 2019

Author Contributor

But this is what mbedtls_mpi_read_binary() does in line 282.
Note that this operation was added after the test vector in our tests failed, so the output of this function is now as expected outside the Cryptocell world

This comment has been minimized.

Copy link
@k-stachowiak

k-stachowiak Oct 2, 2019

Contributor

Thanks for the clarification 👍

@adbridge adbridge added needs: CI and removed needs: review labels Oct 7, 2019
@adbridge

This comment has been minimized.

Copy link
Contributor

adbridge commented Oct 7, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Oct 7, 2019

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_exporter
@adbridge

This comment has been minimized.

Copy link
Contributor

adbridge commented Oct 7, 2019

Looks like a CI license issue so restarting

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Oct 7, 2019

Test run: FAILED

Summary: 2 of 11 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_cloud-client-test
  • jenkins-ci/mbed-os-ci_exporter
@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Oct 7, 2019

Test run: FAILED

Summary: 1 of 11 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_exporter
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 14, 2019

Exporters restarted

@0xc0170 0xc0170 merged commit f05f03f into ARMmbed:master Oct 15, 2019
25 checks passed
25 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+24 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8688 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8464B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.