Unexpected reset of DRBG reseed interval

[juppman]

Description

• Type: Feature Request
• Priority: Major

---

Enhancement\Feature Request

Consider the following scenario

1. mbedtls_ctr_drbg_init is called to initialize the DRBG context
2. mbedtls_ctr_drbg_set_reseed_interval is called to specify a custom reseed interval
3. mbedtls_ctr_drbg_seed is called to perform initial seeding

From my point of view, this seems like a proper procedure and should result in the custom reseed interval being used. But currently mbedtls_ctr_drbg_seed resets the reseed interval to the default value.

I have found no indication in the API documentation informing a developer about this behavior.

Justification - why does the library need this feature?
Since reseed interval could be considered security critical, it should be hard to end up using another value than intended.

Suggested enhancement
Either avoid resetting reseed interval in the seed function or add proper documentation/instructions for the developer that integrates with mbedTLS.

[gilles-peskine-arm]

I agree, the library code should have been written in such a way that set_reseed_interval before seed works.

I recently fixed a similar bug for set_entropy_len (ARMmbed/mbed-crypto#299). We should make a similar fix for the reseed interval. At least in this case the workaround of calling set_reseed_interval after seed works.

This applies to HMAC_DRBG as well.

[gavacq]

I'd like to work on this issue, but since this is my first time contributing to open source software, please be patient and let me know what my PR is missing.
First, is there any way to see if there is a PR for this issue already? I searched in issues and pull requests with is:open 2927 but didn't see anything.

[yanesca]

Hi @gacquroff, thank you for considering to contribute to Mbed TLS!

Github automatically adds links to PRs that reference this issue. Since there is no such link added, there is no PR addressing this issue yet.

[gilles-peskine-arm]

Thanks @gacquroff ! If you haven't already done so, please read our coding standards.

For this bug fix, here are a few things I can think of:

• Please ensure that the behavior of a “sensible” application doesn't change. What “sensible” means is to some extent a value judgement. We never change documented behavior if we can help it, but we do sometimes change undocumented behavior.
• Please make sure to update the documentation where relevant.
• Please add a non-regression test if at all possible.
• Some style mistakes may cause tests/scripts/check-files.py to complain with a comprehensible error message, or tests/scripts/check-names.sh to complain with some incomprehensible output. The most common gotcha with check-names.sh is that all lines in a multiline comment after the first must start with space followed by *: * in column 0 is no good.

[gavacq]

@yanesca Thanks!

@gilles-peskine-arm Is a non-regression test equivalent to a regression test? I've seen conflicting information.

[gilles-peskine-arm]

Is a non-regression test equivalent to a regression test?

I mean a test that fails before fixing the bug, and passes once the bug is fixed. I can't think of another meaning for either “regression test” or “non-regression test”. I've seen both expressions in common use.

[gavacq]

Thanks for clarifying! I've seen regression test meaning "a test that ensures there isnt a regression, i.e. passes before the patch and passes after", so that's why I was confused.