Fixed seed variable concatenation pointer.

[stroebeljc]

Description

This request fixes a minor issue with random number generation in CTR_DRBG (issue #3819). The current implementation places the nonce value at the same starting location as the just retrieved seed value. However, the seed size is still incremented, resulting in a section of zeroes the same length as the nonce at the end of the seed.

This fix just moves the nonce to where it is supposed to be in the concatenated seed.

Fixes #3819

Status

READY

Backports needed?

NO - the feature with the problem is not present in any of the LTS branches.

Steps to test or reproduce

The self-test was updated to include nonce concatenation and expected results.

[mpg]

Hi @stroebeljc and thanks for your contribution! Indeed the current behaviour of the code does no seem intentional: overwriting entropy with other entropy does not make sense, and is in direct contradiction with the documentation of mbedtls_ctr_drbg_seed() which clearly states that the outputs of the two calls to f_entropy are concatenated. Thanks for spotting that!

I'm taking the liberty of editing the PR description to add that backports are not needed, as the feature is not present in any of the LTS branches. Note: the feature was introduced in ARMmbed/mbed-crypto#305 and had this bug from the start.

[mpg]

The fix looks absolutely good to me, so I'm going to focus on what's not there :)

• I think this deserves a ChangeLog entry as a Bugfix, can you add one? (See ChangeLog.d/00README.md` for how to do that.)
• I'm not fully convinced that this can't be tested so I think it's worth at least discussing that point a bit more. In tests/suites/test_suite_ctr_drbg.function we use a "fixed" entropy callback, mbedtls_test_entropy_func for several purposes including tracking how much "entropy" was consumed and validating output against test vectors. I think the same approach should work here. IMO the questions are: (1) can we find test vectors from a trusted source that covers the desired length? and (2) if not, would it still be worth adding a test based on values generated after your patch, just to avoid any regression in the future?

Could you look a bit more into how this could bit tested and let me know what you think? (Also, once less people are on holiday, a second team member is going to review this as well and might have opinions or suggestions on this too.)

[stroebeljc]

I took a look at NIST DRBG test vectors to see if the CTR_CRBG algorithm is working as expected. Found that the self test routine was not complete and correct, so I updated the method and its data to verify proper operation with actual NIST test vectors.

[mpg]

This is looking very good! I'm just requesting a comment in the self-test function documenting the origin of the test data.

[mpg]

Thanks for the reference! Looks all good to me now.

[mpg]

Now the next step is for another team member to review, which is unlikely to happen this week as most of the team is on leave, but hopefully some time next week.

[gilles-peskine-arm]

Oops. Thanks for finding and fixing this! I'm mostly happy but I'd like some minor documentation improvements.

I'd have preferred to keep the self-tests as they were and improve the unit tests, but since you've already changed the unit tests, let's go with that. I've filed a request for unit tests separately. We shouldn't have things that are only validated through self-tests and not through specific unit tests, but it'll do for now.

[gilles-peskine-arm]

LGTM apart from the changelog entry.

[mpg]

LGTM