Backport 2.7: Allow xxx_drbg_set_entropy_len before xxx_drbg_seed #2895
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
needs-review
Contributor
Author
|
While making ARMmbed/mbed-crypto#305, I discovered some documentation that I'd failed to update. https://github.com/gilles-peskine-arm/mbedtls/tree/drbg-set_entropy_len-doc_cleanup-2.7 contains fixup commits for these missing updates, and I rewrote the history of this branch to squash the fixups. |
mbedtls_hmac_drbg_seed() always set the entropy length to the default, so a call to mbedtls_hmac_drbg_set_entropy_len() before seed() had no effect. Change this to the more intuitive behavior that set_entropy_len() sets the entropy length and seed() respects that and only uses the default entropy length if there was no call to set_entropy_len().
Move the definitions of mbedtls_ctr_drbg_seed_entropy_len() and mbedtls_ctr_drbg_seed() to after they are used. This makes the code easier to read and to maintain.
mbedtls_ctr_drbg_seed() always set the entropy length to the default, so a call to mbedtls_ctr_drbg_set_entropy_len() before seed() had no effect. Change this to the more intuitive behavior that set_entropy_len() sets the entropy length and seed() respects that and only uses the default entropy length if there was no call to set_entropy_len(). The former test-only function mbedtls_ctr_drbg_seed_entropy_len() is no longer used, but keep it for strict ABI compatibility.
gilles-peskine-arm
force-pushed
the
drbg-set_entropy_len-2.7
branch
from
b4802d7 to
093aa51
Compare
| return( ret ); | ||
| } | ||
|
|
||
| int mbedtls_ctr_drbg_seed( mbedtls_ctr_drbg_context *ctx, |
Contributor
There was a problem hiding this comment.
This function lacks the needed documentation now.
Contributor
Author
There was a problem hiding this comment.
What documentation? If you mean the comments relating the implementation to the NIST standard that are present in the mbed-crypto and 2.16 PRs, they were added after 2.7 and never backported, and backporting them would be out of scope here.
AndrzejKurek
suggested changes
Oct 24, 2019
Contributor
AndrzejKurek
left a comment
AndrzejKurek
left a comment
There was a problem hiding this comment.
Please update the documentation.
AndrzejKurek
previously approved these changes
Oct 25, 2019
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. 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().
gilles-peskine-arm
force-pushed
the
drbg-set_entropy_len-2.7
branch
from
9f5e7ed to
cc4c69f
Compare
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. 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
drbg-set_entropy_len-2.7
branch
from
cc4c69f to
4c575c0
Compare
k-stachowiak
approved these changes
Oct 29, 2019
AndrzejKurek
approved these changes
Nov 4, 2019
gilles-peskine-arm
added
approved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
The functions
mbedtls_ctr_drbg_set_entropy_len()andmbedtls_hmac_drbg_set_entropy_len()were only supported after the call tombedtls_ctr_drbg_seed()andmbedtls_hmac_drbg_seed(), so the initial seeding always grabbed the amount of entropy defined at compile time (MBEDTLS_CTR_DRBG_ENTROPY_LENfor CTR_DRBG, calculated from the hash algorithm for HMAC_DRBG). There was no good reason for that and the documentation was not clear prior to ARMmbed/mbed-crypto#287. Change the API to be intuitive: you can callset_entropy_len()afterinit(), and it will influence both the initial seeding (if you call it beforeseed()) and subsequent reseeds.Keep the test-only function
mbedtls_ctr_drbg_seed_entropy_lenfor strict ABI compatibility.Backport of ARMmbed/mbed-crypto#299, plus some follow-up fixes from ARMmbed/mbed-crypto#305.