Skip to content

DeviceKey: Fix random key doesn't generate with custom entropy source #11725

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

Merged
merged 1 commit into from
Oct 24, 2019

Conversation

ccli8
Copy link
Contributor

@ccli8 ccli8 commented Oct 22, 2019

Description

This PR tries to fix random key doesn't generate with custom entropy source in device_key. Originally, when DEVICE_TRNG is defined, MBEDTLS_ENTROPY_HARDWARE_ALT will also be defined accordingly to provide entropy source. This is fine for targets supporting TRNG. However, for targets without TRNG, it is also possible to provide non-TRNG entropy source solution via the define MBEDTLS_ENTROPY_HARDWARE_ALT. Related discussion can be found at #11680

Pull request type

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

…E_ALT

Originally, when DEVICE_TRNG is defined, MBEDTLS_ENTROPY_HARDWARE_ALT will also be defined
accordingly to provide entropy source. This is fine for targets supporting TRNG. However, for
targets without TRNG, it is also possible to provide non-TRNG entropy source solution via the
define MBEDTLS_ENTROPY_HARDWARE_ALT. Related discussion can be found at:

ARMmbed#11680
@ciarmcom
Copy link
Member

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

@VeijoPesonen
Copy link
Contributor

@ARMmbed/mbed-os-crypto are you fine with this?

@yanesca
Copy link
Contributor

yanesca commented Oct 22, 2019

Yes, I think this should be fine:

  • MBEDTLS_ENTROPY_HARDWARE_ALT is the same mechanism we register TRNGs. The user can use the same mechanism to register something as an entropy source that is weaker than a TRNG. (This might or might not be a security issue depending on the user's threat model.) This was possible before and works without a functional problem in most of the cases. This is the only function that we know of that does not work with user defined MBEDTLS_ENTROPY_HARDWARE_ALT.

  • There is room for improvement regarding security, but I think that any such improvement is out of scope for this PR.

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 23, 2019

@VeijoPesonen we should be good to proceed here?

Copy link
Contributor

@VeijoPesonen VeijoPesonen left a comment

Choose a reason for hiding this comment

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

Approved securitywise by @yanesca. Looks also good to me.

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 23, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Oct 23, 2019

Test run: SUCCESS

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

@0xc0170 0xc0170 merged commit acf576a into ARMmbed:master Oct 24, 2019
@ccli8 ccli8 deleted the nuvoton_devicekey_entropy_alt branch October 24, 2019 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants