Allow xxx_drbg_set_entropy_len before xxx_drbg_seed #299
Conversation
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(). This removes the need for the test-only function mbedtls_ctr_drbg_seed_entropy_len(). Just call mbedtls_ctr_drbg_set_entropy_len() followed by mbedtls_ctr_drbg_seed(), it works now.
gilles-peskine-arm
added
bug
gilles-peskine-arm
removed
the
needs: review
|
CI status: the failures are from tests on one embedded board that looks like an instability with this board affecting other jobs and the ABI compatibility check reporting that |
|
CI passed except for ABI checking (as expected) and |
* 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
| @@ -415,7 +409,8 @@ int mbedtls_ctr_drbg_seed_entropy_len( | |||
| ctx->f_entropy = f_entropy; | |||
| ctx->p_entropy = p_entropy; | |||
|
|
|||
| ctx->entropy_len = entropy_len; | |||
| if( ctx->entropy_len == 0 ) | |||
There was a problem hiding this comment.
Hi @gilles-peskine-arm, is there any reason you didin't use ctr_drbg_set_entropy_len() here? Maybe you wanted to keep some level of method independency.
There was a problem hiding this comment.
I can't think of a strong reason one way or the other. Semantically, I want to set the entropy_len field to a certain value. mbedtls_ctr_drbg_set_entropy_len() does that, but so does an assignment. An assignment is simpler than calling a function. Arguably, calling a function is better in case the function changes. But changes to do what? If it changes to add some validation, this validation might not be what is needed at this point inside mbedtls_ctr_drbg_seed. And if the meaning of the field changes, there'll be other changes needed in the code, so who knows what would need to change in mbedtls_ctr_drbg_seed. In terms of code size, the assignment is a couple of bytes smaller than a function call. So I have a very slight preference for the direct assignment. But only very slight. I don't remember thinking about it explicitly at the time.
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 #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.A side benefit is that this removes the need for the test-only function
mbedtls_ctr_drbg_seed_entropy_len.Upon reflection, the ways this would break existing applications are:
xxx_set_entropy_len()beforexxx_drbg_seedand expected it to do nothing. That's no more likely than an application that calledxxx_set_entropy_len()and expected this to set the entropy length throughout the lifetime of the DRBG object, and is currently unknowingly grabbing the default amount of entropy instead.mbedtls_ctr_drbg_seed_entropy_lendespite it being marked as for internal use only.So I think this should be backported, perhaps with a backward compatibility wrapper
mbedtls_ctr_drbg_seed_entropy_lenfor the sake of linkers that may have performed some cross-module inlining across a shared library call (hopefully that doesn't happen, but maybe we can spare a few bytes of code size to play it safe).A step on the path to IOTCRYPT-180.