Support set *_drbg reseed interval before seed

[gavacq]

Description

Fixes #2927

Summary of change

mbedtls_hmac_drbg_seed() and mbedtls_ctr_drbg_seed() were setting the entropy context reseed_interval to the default value of 10 000 even when set_reseed_interval() had been called prior to seed(). The documentation did not describe the API behaving this way, and it doesn't make sense that seed() would remove any prior preference for reseed_interval so this behavior has been fixed. Now, the value set by set_reseed_interval() will persist through after a call to seed().

Impact of changes

This change would break any application that

• set the reseed_interval and expected it to be reset to the default value once they called seed()
• set the reseed_interval and was unknowingly using the default value for the lifetime of the DRBG object

The first situation could be dangerous if the set interval was very high and the PRNG was never reseeded. It is an unlikely scenario however because a user that understood the prior behavior would probably not design their application in this way anyways.

In the second situation, this change could expose a vulnerability in the user's application, so this could provide them with a net benefit.

Migration actions required

None

Documentation

• Comments have been updated.
• Some formatting was done to align comment indentation.

---

Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

---

Test results

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[x] Tests / results supplied as part of this PR

Modifies *_drbg_entropy_usage() so that set_reseed_interval() is called before seed(). This would fail before this patch.

[danh-arm]

Hi @gacquroff . Thanks for your contribution.

It looks like this patch breaks our doxygen generation:

******************************************************************

* check_doxygen_warnings: Check: doxygen warnings (builds the documentation) 

* Wed Jun  3 16:37:05 UTC 2020

******************************************************************

/var/lib/build/include/mbedtls/ctr_drbg.h:306: warning: Invalid list item found

FAIL

^^^^check_doxygen_warnings: Check: doxygen warnings (builds the documentation): tests/scripts/doxygen.sh -> 1^^^^

Could you please fix this before we review? Regards, Dan.

[gilles-peskine-arm]

For some reason (network glitch?) the Travis log wasn't linked automatically.

[gavacq]

Travis CI build passed, but I'm not sure about the PR-3393-head TLS Testing failure. Should I be able to see the log from that?

[ronald-cron-arm]

Hi @gacquroff, the PR-3393-head TLS Testing failure was just a CI glitch (a repo cloning failed) thus from the CI point of view it is all good now. Thanks for reverting the comment changes.

[gavacq]

Hi @ronald-cron-arm, no problem. Let me know if I need to fix any other CI bugs.

[gilles-peskine-arm]

FreeBSD failed again in a re-run, but it isn't the fault of this PR. We'll need a clean run on FreeBSD before merging, but I don't expect any problems.

[gilles-peskine-arm]

I noticed a missing link in the documentation. Other than that, looks good to me, thanks.

[gavacq]

hmac_drbg_no_reseed() test was failing before this change due to the reseed_interval not set to a non zero value after hmac_drbg_free(). This caused the subsquent mbedtls_hmac_drbg_random_with_add() to reseed, failing the test.

@ronald-cron-arm I would argue that we should not support using the DRBG after free() but before another init(), since free() destroys the context mutex. However we don't say this anywhere in the documentation. What do you think?

[ronald-cron-arm]

Sorry for this late reply. I've checked with @gilles-peskine-arm and in fact the test is right and thus shouldn't be changed. mbedtls_hmac_drbg_free() should reset the context to a state that's equivalent to its state after the initial call to mbedtls_hmac_drbg_init(). One could argue that is more a reset than a free probably but that's the convention across mbedtls interfaces. The same apply to ctr_drbg. If this is ok with you, while at it, you are welcome to update the documentation of the free() functions to clarify their behavior. It would be much appreciated but no worries if you don't have time to do it.

[gavacq]

I think this 'reset' convention does not apply to the MBEDTLS_THREADING_C mutex as after mbedtls_hmac_drbg_free the mutex handle is NULL. I added mbedtls_mutex_init after mbedtls_platform_zeroize in mbedtls_hmac_drbg_free in the latest patch. Likewise with ctr_drbg. This would be consistent with 'resetting context to state after initial call to mbedtls_hmac_drbg_init'

[gavacq]

@ronald-cron-arm Are those test failures another CI glitch? I'm happy to clean up anything else here.

[danh-arm]

The CI failed due to this having the "needs: work" label. I've removed and re-triggered now.

[ronald-cron-arm]

The free() functions have to be fixed (see #3393 (comment)).

[gilles-peskine-arm]

Please add a changelog entry file in ChangeLog.d. Suggested content:

Bugfix
   * In CTR_DRBG and HMAC_DRBG, don't reset the reseed interval in seed().
     Fixes #2927.

[gilles-peskine-arm]

Please add a newline at the end of the file (Unix text encoding).

[ronald-cron-arm]

Thanks a lot for this. Apart from the CI issue pointed out by @gilles-peskine-arm this looks good to me. I've checked that by reverting the changes in ctr_drbg.c and hmac_drbg.c the non-regression tests are falling as expected and where expected. Looking forward for the backports so we can merge this PR.

[gavacq]

See backports:
2.16: #3938
2.7: #3937