CTR_DRBG: grab a nonce from the entropy source if needed #305
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gilles-peskine-arm
added
bug
gilles-peskine-arm
force-pushed
the
ctr_drbg-grab_nonce_from_entropy-set_nonce_length
branch
from
204e45f to
79dde0f
Compare
This was referenced Oct 23, 2019
Closed
Closed
Update a comment that referred to a now-removed function.
Remove a comment that documented a now-removed restriction.
No semantic change.
Add a new function mbedtls_ctr_drbg_set_nonce_len() which configures the DRBG instance to call f_entropy a second time during the initial seeding to grab a nonce. The default nonce length is 0, so there is no behavior change unless the user calls the new function.
Add a new function mbedtls_ctr_drbg_set_nonce_len() which configures the DRBG instance to call f_entropy a second time during the initial seeding to grab a nonce. The default nonce length is 0, so there is no behavior change unless the user calls the new function.
Test mbedtls_ctr_drbg_set_nonce_len (good cases only, which is in keeping with the coverage of other functions).
No behavior change. Prepare for a future version that will set the entropy nonce length to a nonzero value by default.
Change the default entropy nonce length to be nonzero in some cases. Specifically, the default nonce length is now set in such a way that the entropy input during the initial seeding always contains enough entropy to achieve the maximum possible security strength per NIST SP 800-90A given the key size and entropy length. If MBEDTLS_CTR_DRBG_ENTROPY_LEN is kept to its default value, mbedtls_ctr_drbg_seed() now grabs extra entropy for a nonce if MBEDTLS_CTR_DRBG_USE_128_BIT_KEY is disabled and either MBEDTLS_ENTROPY_FORCE_SHA256 is enabled or MBEDTLS_SHA512_C is disabled. If MBEDTLS_CTR_DRBG_USE_128_BIT_KEY is enabled, or if the entropy module uses SHA-512, then the default value of MBEDTLS_CTR_DRBG_ENTROPY_LEN does not require a second call to the entropy function to achieve the maximum security strength. This choice of default nonce size guarantees NIST compliance with the maximum security strength while keeping backward compatibility and performance high: in configurations that do not require grabbing more entropy, the code will not grab more entropy than before.
The default entropy nonce length is either zero or nonzero depending on the desired security strength and the entropy length. The implementation calculates the actual entropy nonce length from the actual entropy length, and therefore it doesn't need a constant that indicates the default entropy nonce length. A portable application may be interested in this constant, however. And our test code could definitely use it. Define a constant MBEDTLS_CTR_DRBG_ENTROPY_NONCE_LEN and use it in test code. Previously, test_suite_ctr_drbg had knowledge about the default entropy nonce length built in and test_suite_psa_crypto_init failed. Now both use MBEDTLS_CTR_DRBG_ENTROPY_NONCE_LEN. This change means that the test ctr_drbg_entropy_usage no longer validates that the default entropy nonce length is sensible. So add a new test that checks that the default entropy length and the default entropy nonce length are sufficient to ensure the expected security strength.
gilles-peskine-arm
force-pushed
the
ctr_drbg-grab_nonce_from_entropy-set_nonce_length
branch
from
79dde0f to
6997166
Compare
gilles-peskine-arm
added
the
needs: work
|
Looking at the test results, many Mbed OS jobs are failing. This isn't just known bugs or ordinary flakiness of some boards on the CI: all the benchmark jobs are failing on K64F with an error that's directly related to this PR (https://jenkins-internal.mbed.com/job/mbed-crypto-pr/job/PR-305-merge/4/execution/node/3923/log/), and all the CY8CKIT jobs are failing (not just the known issue with IAR) for a reason I don't understand. |
You can't reuse a CTR_DRBG context without free()ing it and re-init()ing. This generally happened to work, but was never guaranteed. It could have failed with alternative implementations of the AES module because mbedtls_ctr_drbg_seed() calls mbedtls_aes_init() on a context which is already initialized if mbedtls_ctr_drbg_seed() hasn't been called before, plausibly causing a memory leak. Calling free() and seed() with no intervening init fails when MBEDTLS_THREADING_C is enabled and all-bits-zero is not a valid mutex representation. So add the missing free() and init().
You can't reuse a CTR_DRBG context without free()ing it and re-init()ing it. This generally happened to work, but was never guaranteed. It could have failed with alternative implementations of the AES module because mbedtls_ctr_drbg_seed() calls mbedtls_aes_init() on a context which is already initialized if mbedtls_ctr_drbg_seed() hasn't been called before, plausibly causing a memory leak. Since the addition of mbedtls_ctr_drbg_set_nonce_len(), the second call to mbedtls_ctr_drbg_seed() uses a nonsensical value as the entropy nonce length. Calling free() and seed() with no intervening init fails when MBEDTLS_THREADING_C is enabled and all-bits-zero is not a valid mutex representation.
gilles-peskine-arm
force-pushed
the
ctr_drbg-grab_nonce_from_entropy-set_nonce_length
branch
from
07ebf57 to
bd326f9
Compare
k-stachowiak
approved these changes
Oct 29, 2019
|
The non-reproducible CY8CKIT_062_WIFI_BT failures look like one board is non-responsive and other boards are working. There are enough passes to show that this PR is ok. |
gilles-peskine-arm
removed
the
needs: work
|
CI has at least one bad Cypress board in it causing tests to not complete successfully. I manually tested our mbed-os-example-mbed-crypto/getting-started example on With This is due to the version of |
Patater
removed
the
needs: review
|
@gilles-peskine-arm What is the meaning of the |
|
@yanesca It's a git convention and feature indicating that the commit is an amendment to the previous commit with the same title minus the Normally a fixup is only in work in progress and shouldn't be pushed: you should rebase and squash the fixup before pushing. But here the defective commit was already present in the history, so the amendment can't be squashed. |
gilles-peskine-arm
added a commit
to gilles-peskine-arm/mbed-crypto
that referenced
this pull request
Nov 15, 2019
* ARMmbed#292: Make psa_close_key(0) and psa_destroy_key(0) succeed * ARMmbed#299: Allow xxx_drbg_set_entropy_len before xxx_drbg_seed * ARMmbed#259: Check `len` against buffers size upper bound in PSA tests * ARMmbed#288: Add ECDSA tests with hash and key of different lengths * ARMmbed#305: CTR_DRBG: grab a nonce from the entropy source if needed * ARMmbed#316: Stop transactions from being reentrant * ARMmbed#317: getting_started: Make it clear that keys are passed in * ARMmbed#314: Fix pk_write with EC key to use a constant size for the private value * ARMmbed#298: Test a build without any asymmetric cryptography * ARMmbed#284: Fix some possibly-undefined variable warnings * ARMmbed#315: Define MBEDTLS_PK_SIGNATURE_MAX_SIZE * ARMmbed#318: Finish side-porting commits from mbedtls-restricted that missed the split
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CTR_DRBG grabs as many bytes of entropy for the initial seeding as for reseeds. This is not compliant with NIST SP 800-90A when the entropy length is too small to cover both the entropy itself and a nonce, unless the caller passes a nonce in the
customparameter.Fix this by making a second call to the entropy function to grab enough entropy for a random nonce (half the entropy length, per SP 800-90A).
To maximize backward compatibility, do not make a second call to the entropy function if the entropy length is sufficient to cover a nonce. Thus the only combination of boolean compile-time option that leads to making a second call to the entropy function is:
An alternative design choice would be to always grab a nonce whose length is half the entropy length, like the HMAC_DRBG module does. This would be a simpler design but I preferred this solution which only adds a few bytes of code and which has less risk of breaking someone's workflow.
Test run on a merge of this PR and #291 to get coverage of relevant compilation settings that are currently not covered: https://jenkins-internal.mbed.com/job/mbedtls-psa-release-new/592/ → passed except for some Mbed OS jobs which are also failing in this PR.
Some commits in this PR are minor bugs which I've fixed in the backports of #299 (Mbed-TLS/mbedtls#2894 and Mbed-TLS/mbedtls#2895):
ctr_drbg.handhmac_drbg.hneeds to be backported, and I did so in the backport PRs which have not been merged yet. The one inctr_drbg.cwas already present in a suitably modified form in the 2.16 backport of Allow xxx_drbg_set_entropy_len before xxx_drbg_seed #299 and is not applicable to 2.7.The rest of the PR is a behavior change that should not be backported, and some very minor test improvements that do not need to be backported.
Internal ref: IOTCRYPT-180