Skip to content

Backport to Mbedtls 2.16: Support set *_drbg reseed interval before seed #3938

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
ronald-cron-arm merged 2 commits into from
Dec 7, 2020
Merged

Backport to Mbedtls 2.16: Support set *_drbg reseed interval before seed #3938

ronald-cron-arm merged 2 commits into from
Dec 7, 2020

Conversation

@gavacq
Copy link
Contributor

@gavacq gavacq commented Dec 3, 2020

Backport of #3393.

gavacq added 2 commits December 3, 2020 13:30
@gavacq
Support set *_drbg reseed interval before seed
77cb30c 
mbedtls_ctr_drbg_set_reseed_interval() and
mbedtls_hmac_drbg_set_reseed_interval() can now be called before
their seed functions and the reseed_interval value will persist.
Previously it would be overwritten with the default value.

*_drbg_reseed_interval is now set in init() and free().

mbedtls_ctr_drbg_free() and mbedtls_hmac_drbg_free() now
reset the drbg context to the state immediately after init().

Tests:
- Added test to check that DRBG reseeds when reseed_counter
reaches reseed_interval, if reseed_interval set before seed
and reseed_interval is less than MBEDTLS_*_DRBG_RESEED_INTERVAL.

Signed-off-by: gacquroff <gavina352@gmail.com>
@gavacq
Add changelog entry file for bugfix 2927
07d1f47 
Signed-off-by: gacquroff <gavina352@gmail.com>
@gavacq
Copy link
Contributor Author

gavacq commented Dec 3, 2020

I did have a problem running local tests for this patch, but it seemed unrelated to my change.
Failed to open test file: ./test_suite_asn1parse.datax
This also occured for a few other test datasets.

@gavacq gavacq mentioned this pull request Dec 3, 2020
Merged
@gilles-peskine-arm gilles-peskine-arm added bug Community needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review component-crypto Crypto primitives and low-level interfaces labels Dec 3, 2020
@gilles-peskine-arm
Copy link
Contributor

gilles-peskine-arm commented Dec 3, 2020

Failed to open test file: ./test_suite_asn1parse.datax

I guess you had built the project, then you changed to a 2.16 branch, then you ran make clean, then you built again. 2.16 doesn't have test_suite_asn1parse, and make clean deletes all .datax files but only deletes known executables, so test_suite_asn1parse was left behind. make test runs all the files called test_suite_* with no extension, it doesn't have a list of known test suites. The remedy is to run make clean before changing branches (which after several years of working on Mbed TLS I still forget regularly), or to delete the spurious files manually. Anyhow, it doesn't indicate a problem with the code.

@gavacq
Copy link
Contributor Author

gavacq commented Dec 3, 2020 •
edited
Loading

I guess you had built the project, then you changed to a 2.16 branch, then you ran make clean, then you built again.

That's exactly what I did, hopefully I remember for next time 😃

gilles-peskine-arm
gilles-peskine-arm approved these changes Dec 3, 2020
View reviewed changes
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a faithful backport of #3393. The patch is identical except for expected differences (in CTR_DRBG, reseed_counter marks that the entropy nonce length is not set explicitly) due to ARMmbed/mbed-crypto#305 which was added after 2.16.

@gavacq
Copy link
Contributor Author

gavacq commented Dec 3, 2020

This is a faithful backport of #3393. The patch is identical except for expected differences (in CTR_DRBG, reseed_counter marks that the entropy nonce length is not set explicitly) due to ARMmbed/mbed-crypto#305 which was added after 2.16.

I will add this info into future PRs. Should git blame to reference patch like you did.

ronald-cron-arm
ronald-cron-arm approved these changes Dec 7, 2020
View reviewed changes
Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me as well.

@ronald-cron-arm ronald-cron-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Dec 7, 2020
@ronald-cron-arm ronald-cron-arm merged commit 3f35b87 into Mbed-TLS:mbedtls-2.16 Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gilles-peskine-arm gilles-peskine-arm gilles-peskine-arm approved these changes

@ronald-cron-arm ronald-cron-arm ronald-cron-arm approved these changes

Assignees

No one assigned

Labels

approved Design and code approved - may be waiting for CI or backports bug component-crypto Crypto primitives and low-level interfaces

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@gavacq @gilles-peskine-arm @amadvance @ronald-cron-arm