Test MBEDTLS_CTR_DRBG_USE_128_BIT_KEY #291
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gilles-peskine-arm
added
bug
This was referenced Oct 9, 2019
gilles-peskine-arm
removed
the
needs: preceding PR
gilles-peskine-arm
force-pushed
the
ctr_drbg-test_aes_128
branch
from
b1f1d4a to
067f1a3
Compare
|
New Jenkins run: https://jenkins-internal.mbed.com/job/mbed-crypto-pr/job/PR-291-merge/12/ → PASS |
gilles-peskine-arm
added
needs: ci
gilles-peskine-arm
added
needs: work
|
I'll rebase again :( |
In the CTR_DRBG module, add selftest data for when MBEDTLS_CTR_DRBG_USE_128_BIT_KEY is enabled. I generated the test data by running our own code. This is ok because we have other tests that ensure that the algorithm is implemented correctly. This makes programs/self/selftest pass when MBEDTLS_CTR_DRBG_USE_128_BIT_KEY is enabled.
The test suites should always run self-tests for all enabled features. Otherwise we miss failing self-tests in CI runs, because we don't always run the selftest program independently. There was one spurious dependency to remove: MBEDTLS_CTR_DRBG_USE_128_BIT_KEY for ctr_drbg, which was broken but has now been fixed.
This is a variant toggle, not an extra feature, so it should be tested separately. We test most of the effect of MBEDTLS_ENTROPY_FORCE_SHA256 (namely, using SHA-256 in the entropy module) when we test the library with the SHA512 module disabled (which we do at least via depends-hashes.pl). This commit removes testing of the MBEDTLS_ENTROPY_FORCE_SHA256 option itself, which should be added separately.
The size of the seedfile used by the entropy module when MBEDTLS_ENTROPY_NV_SEED is enabled is 32 byte when MBEDTLS_ENTROPY_FORCE_SHA256 is enabled or MBEDTLS_SHA512_C is disabled, and 64 bytes otherwise. A larger seedfile is ok on entry (the code just grabs the first N bytes), but a smaller seedfile is not ok. Therefore, if you run a component with a 32-byte seedfile and then a component with a 64-byte seedfile, the second component fails in the unit tests (up to test_suite_entropy which erases the seedfile and creates a fresh one). This is ok up to now because we only enable MBEDTLS_ENTROPY_NV_SEED together with MBEDTLS_ENTROPY_FORCE_SHA256. But it prevents enabling MBEDTLS_ENTROPY_NV_SEED without MBEDTLS_ENTROPY_FORCE_SHA256. To fix this, unconditionally create a seedfile before each component.
Test MBEDTLS_CTR_DRBG_USE_128_BIT_KEY and MBEDTLS_ENTROPY_FORCE_SHA256 together and separately.
gilles-peskine-arm
force-pushed
the
ctr_drbg-test_aes_128
branch
from
067f1a3 to
592f591
Compare
gilles-peskine-arm
added
needs: ci
|
Rebased. Previous version in https://github.com/gilles-peskine-arm/mbed-crypto/tree/ctr_drbg-test_aes_128-2 |
gilles-peskine-arm
added
needs: review
AndrzejKurek
approved these changes
Nov 25, 2019
piotr-now
reviewed
Dec 13, 2019
| static const unsigned char result_nopr[16] = | ||
| { 0x6c, 0x25, 0x27, 0x95, 0xa3, 0x62, 0xd6, 0xdb, | ||
| 0x90, 0xfd, 0x69, 0xb5, 0x42, 0x9, 0x4b, 0x84 }; | ||
| #else /* MBEDTLS_CTR_DRBG_USE_128_BIT_KEY */ |
There was a problem hiding this comment.
Don't you think it would be more readable if there was added one extra line before and one after #else? Just as in the line 681?
piotr-now
approved these changes
Dec 13, 2019
gilles-peskine-arm
added a commit
to gilles-peskine-arm/mbed-crypto
that referenced
this pull request
Feb 3, 2020
Previously in d875285: * ARMmbed#333: Streamline PSA key type encodings: prepare * ARMmbed#323: Initialise return values to an error Previously in dbcb442: * ARMmbed#291: Test MBEDTLS_CTR_DRBG_USE_128_BIT_KEY * ARMmbed#334: Fix some pylint warnings Previously in ceceedb: * ARMmbed#348: Bump version to Mbed TLS 2.20.0 and crypto SO version to 4 * ARMmbed#354: Fix incrementing pointer instead of value In this commit: * ARMmbed#349: Fix minor defects found by Coverity * ARMmbed#179: Add option to build SHA-512 without SHA-384 * ARMmbed#327: Implement psa_hash_compute and psa_hash_compare * ARMmbed#330: Streamline PSA key type and curve encodings
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Test
MBEDTLS_CTR_DRBG_USE_128_BIT_KEYproperly. Fix #289. Fix #290.Follow-up of #287.
Needs backports: to 2.16 (almost identical); to 2.7 (only for the
config.handall.shchanges that are aboutMBEDTLS_ENTROPY_FORCE_SHA256).