CTR_DRBG: grab a nonce from the entropy source if needed #305

2019-10-22T14:12:18-05:00

gilles-peskine-arm commented

2019-10-22 14:12:18 -05:00

(Migrated from github.com)

CTR_DRBG grabs as many bytes of entropy for the initial seeding as for reseeds. This is not compliant with NIST SP 800-90A when the entropy length is too small to cover both the entropy itself and a nonce, unless the caller passes a nonce in the custom parameter.

Fix this by making a second call to the entropy function to grab enough entropy for a random nonce (half the entropy length, per SP 800-90A).

To maximize backward compatibility, do not make a second call to the entropy function if the entropy length is sufficient to cover a nonce. Thus the only combination of boolean compile-time option that leads to making a second call to the entropy function is:

!MBEDTLS_CTR_DRBG_USE_AES_128 && (!MBEDTLS_SHA512_C || MBEDTLS_ENTROPY_FORCE_SHA256)

An alternative design choice would be to always grab a nonce whose length is half the entropy length, like the HMAC_DRBG module does. This would be a simpler design but I preferred this solution which only adds a few bytes of code and which has less risk of breaking someone's workflow.

Test run on a merge of this PR and #291 to get coverage of relevant compilation settings that are currently not covered: https://jenkins-internal.mbed.com/job/mbedtls-psa-release-new/592/ → passed except for some Mbed OS jobs which are also failing in this PR.

Some commits in this PR are minor bugs which I've fixed in the backports of #299 (https://github.com/ARMmbed/mbedtls/pull/2894 and https://github.com/ARMmbed/mbedtls/pull/2895):

The fixup commits for “xxx_DRBG: support set_entropy_len() before seed()” in this PR are documentation updates that I missed in the last commit of #299. The ones in ctr_drbg.h and hmac_drbg.h needs to be backported, and I did so in the backport PRs which have not been merged yet. The one in ctr_drbg.c was already present in a suitably modified form in the 2.16 backport of #299 and is not applicable to 2.7.
"Note that mbedtls_ctr_drbg_seed() must not be called twice" is a follow-up to #287 which I added to the backports of #299.
"Fix CTR_DRBG benchmark" fixes a bug that this PR revealed. I backported this fix in the backports of #299 and side-ported it in the Mbed OS benchmark in https://github.com/ARMmbed/mbed-os-example-tls/pull/262.

The rest of the PR is a behavior change that should not be backported, and some very minor test improvements that do not need to be backported.

Internal ref: IOTCRYPT-180

CTR_DRBG grabs as many bytes of entropy for the initial seeding as for reseeds. This is not compliant with NIST SP 800-90A when the entropy length is too small to cover both the entropy itself and a nonce, unless the caller passes a nonce in the `custom` parameter. Fix this by making a second call to the entropy function to grab enough entropy for a random nonce (half the entropy length, per SP 800-90A). To maximize backward compatibility, do not make a second call to the entropy function if the entropy length is sufficient to cover a nonce. Thus the only combination of boolean compile-time option that leads to making a second call to the entropy function is: ``` !MBEDTLS_CTR_DRBG_USE_AES_128 && (!MBEDTLS_SHA512_C || MBEDTLS_ENTROPY_FORCE_SHA256) ``` An alternative design choice would be to always grab a nonce whose length is half the entropy length, like the HMAC_DRBG module does. This would be a simpler design but I preferred this solution which only adds a few bytes of code and which has less risk of breaking someone's workflow. Test run on a merge of this PR and #291 to get coverage of relevant compilation settings that are currently not covered: https://jenkins-internal.mbed.com/job/mbedtls-psa-release-new/592/ → passed except for some Mbed OS jobs which are also failing in this PR. Some commits in this PR are minor bugs which I've fixed in the backports of #299 (https://github.com/ARMmbed/mbedtls/pull/2894 and https://github.com/ARMmbed/mbedtls/pull/2895): * The fixup commits for “xxx_DRBG: support set_entropy_len() before seed()” in this PR are documentation updates that I missed in the last commit of #299. The ones in `ctr_drbg.h` and `hmac_drbg.h` needs to be backported, and I did so in the backport PRs which have not been merged yet. The one in `ctr_drbg.c` was already present in a suitably modified form in the 2.16 backport of #299 and is not applicable to 2.7. * "Note that mbedtls_ctr_drbg_seed() must not be called twice" is a follow-up to #287 which I added to the backports of #299. * "Fix CTR_DRBG benchmark" fixes a bug that this PR revealed. I backported this fix in the backports of #299 and side-ported it in the Mbed OS benchmark in https://github.com/ARMmbed/mbed-os-example-tls/pull/262. The rest of the PR is a behavior change that should not be backported, and some very minor test improvements that do not need to be backported. Internal ref: IOTCRYPT-180

k-stachowiak (Migrated from github.com) reviewed

2019-10-28 10:36:47 -05:00

k-stachowiak (Migrated from github.com) left a comment

I would like to ask for clarification of two issues I have pointed out in the comments. In general the PR looks good though.

library/ctr_drbg.c

					
				@ -381,15 +416,33 @@ exit:

				    return( ret );

				}

Becoming static, this no longer needs the mbedtls_ prefix. Is it supposed to be left like this e.g. for maintaining the ABI compatibility even for the internal API?

Becoming static, this no longer needs the `mbedtls_` prefix. Is it supposed to be left like this e.g. for maintaining the ABI compatibility even for the internal API?

tests/suites/test_suite_ctr_drbg.function

					
				@ -191,0 +216,4 @@

				    TEST_ASSERT( test_offset_idx - last_idx >= byte_strength );

				exit:

				    mbedtls_ctr_drbg_free( &ctx );

test_offset_idx doesn't seem to be updated anywhere in this test case. Therefore, neither this, nor the last assertion operate on anything depending on the runtime other than the function argument.

`test_offset_idx` doesn't seem to be updated anywhere in this test case. Therefore, neither this, nor the last assertion operate on anything depending on the runtime other than the function argument.

gilles-peskine-arm (Migrated from github.com) reviewed

2019-10-28 10:40:05 -05:00

library/ctr_drbg.c

					
				@ -381,15 +416,33 @@ exit:

				    return( ret );

				}

It's an internal function, so it doesn't need an mbedtls_xxx prefix, but I think using the prefix makes the code easier to read. For example you can search for mbedtls_ctr_drbg_reseed (in the file, or in git grep) and it'll find mbedtls_ctr_drbg_reseed_internal.

It's an internal function, so it doesn't _need_ an `mbedtls_xxx` prefix, but I think using the prefix makes the code easier to read. For example you can search for `mbedtls_ctr_drbg_reseed` (in the file, or in `git grep`) and it'll find `mbedtls_ctr_drbg_reseed_internal`.

gilles-peskine-arm (Migrated from github.com) reviewed

2019-10-28 10:40:20 -05:00

tests/suites/test_suite_ctr_drbg.function

					
				@ -191,0 +216,4 @@

				    TEST_ASSERT( test_offset_idx - last_idx >= byte_strength );

				exit:

				    mbedtls_ctr_drbg_free( &ctx );

It's a global variable. mbedtls_test_entropy_func updates it.

It's a global variable. `mbedtls_test_entropy_func` updates it.

gilles-peskine-arm commented

2019-10-28 11:00:12 -05:00

(Migrated from github.com)

Looking at the test results, many Mbed OS jobs are failing. This isn't just known bugs or ordinary flakiness of some boards on the CI: all the benchmark jobs are failing on K64F with an error that's directly related to this PR (https://jenkins-internal.mbed.com/job/mbed-crypto-pr/job/PR-305-merge/4/execution/node/3923/log/), and all the CY8CKIT jobs are failing (not just the known issue with IAR) for a reason I don't understand.

k-stachowiak (Migrated from github.com) approved these changes

2019-10-29 04:05:48 -05:00

k-stachowiak (Migrated from github.com) left a comment

Thanks for addressing my comments! I have also reviewed the two new commits. Still looks good.

gilles-peskine-arm commented

2019-10-29 08:35:09 -05:00

(Migrated from github.com)

CI failures:

The benchmark jobs are failing on Mbed OS. This is because the Mbed OS benchmark application has the same bug that I fixed in this PR. https://github.com/ARMmbed/mbed-os-example-tls/pull/262 fixes it (CI run: https://jenkins-internal.mbed.com/job/mbedtls-psa-release-new/599/ → benchmark passes everywhere, but some runs of hashing and authcrypt are failing on CY8CKIT_062_WIFI_BT).
getting-started-CY8CKIT_062_WIFI_BT-IAR fails. This is a known issue that existed before this PR.
Some other CY8CKIT_062_WIFI_BT jobs are failing, but only on -merge, not on -head: authcrypt-CY8CKIT_062_WIFI_BT-ARM, authcrypt-CY8CKIT_062_WIFI_BT-ARM, authcrypt-CY8CKIT_062_WIFI_BT-ARM, authcrypt-CY8CKIT_062_WIFI_BT-IAR, authcrypt-CY8CKIT_062_WIFI_BT-IAR, authcrypt-CY8CKIT_062_WIFI_BT-IAR, authcrypt-CY8CKIT_062_WIFI_BT-IAR, hashing-CY8CKIT_062_WIFI_BT-IAR, hashing-CY8CKIT_062_WIFI_BT-IAR, hashing-CY8CKIT_062_WIFI_BT-IAR, authcrypt-CY8CKIT_062_WIFI_BT-ARM, authcrypt-CY8CKIT_062_WIFI_BT-ARM, authcrypt-CY8CKIT_062_WIFI_BT-GCC_ARM, hashing-CY8CKIT_062_WIFI_BT-ARM, hashing-CY8CKIT_062_WIFI_BT-ARM. I don't understand what's going on there.

CI failures: * The benchmark jobs are failing on Mbed OS. This is because the Mbed OS benchmark application has the same bug that I fixed in this PR. https://github.com/ARMmbed/mbed-os-example-tls/pull/262 fixes it (CI run: https://jenkins-internal.mbed.com/job/mbedtls-psa-release-new/599/ → benchmark passes everywhere, but some runs of hashing and authcrypt are failing on CY8CKIT_062_WIFI_BT). * `getting-started-CY8CKIT_062_WIFI_BT-IAR` fails. This is a known issue that existed before this PR. * Some other `CY8CKIT_062_WIFI_BT` jobs are failing, but only on [-merge](https://jenkins-internal.mbed.com/blue/organizations/jenkins/mbed-crypto-pr/detail/PR-305-merge/6/pipeline), not on [-head](https://jenkins-internal.mbed.com/blue/organizations/jenkins/mbed-crypto-pr/detail/PR-305-head/5/pipeline): [authcrypt-CY8CKIT_062_WIFI_BT-ARM](https://jenkins-internal.mbed.com/job/mbed-crypto-pr/job/PR-305-merge/6/execution/node/3455/log/), [authcrypt-CY8CKIT_062_WIFI_BT-ARM](https://jenkins-internal.mbed.com/job/mbed-crypto-pr/job/PR-305-merge/6/execution/node/3915/log/), [authcrypt-CY8CKIT_062_WIFI_BT-ARM](https://jenkins-internal.mbed.com/job/mbed-crypto-pr/job/PR-305-merge/6/execution/node/3926/log/), [authcrypt-CY8CKIT_062_WIFI_BT-IAR](https://jenkins-internal.mbed.com/job/mbed-crypto-pr/job/PR-305-merge/6/execution/node/3868/log/), [authcrypt-CY8CKIT_062_WIFI_BT-IAR](https://jenkins-internal.mbed.com/job/mbed-crypto-pr/job/PR-305-merge/6/execution/node/3868/log/), [authcrypt-CY8CKIT_062_WIFI_BT-IAR](https://jenkins-internal.mbed.com/job/mbed-crypto-pr/job/PR-305-merge/6/execution/node/3920/log/), [authcrypt-CY8CKIT_062_WIFI_BT-IAR](https://jenkins-internal.mbed.com/job/mbed-crypto-pr/job/PR-305-merge/6/execution/node/3931/log/), [hashing-CY8CKIT_062_WIFI_BT-IAR](https://jenkins-internal.mbed.com/job/mbed-crypto-pr/job/PR-305-merge/6/execution/node/3867/log/), [hashing-CY8CKIT_062_WIFI_BT-IAR](https://jenkins-internal.mbed.com/job/mbed-crypto-pr/job/PR-305-merge/6/execution/node/3919/log/), [hashing-CY8CKIT_062_WIFI_BT-IAR](https://jenkins-internal.mbed.com/job/mbed-crypto-pr/job/PR-305-merge/6/execution/node/3930/log/), [authcrypt-CY8CKIT_062_WIFI_BT-ARM](https://jenkins-internal.mbed.com/job/mbed-crypto-pr/job/PR-305-merge/6/execution/node/5728/log/), [authcrypt-CY8CKIT_062_WIFI_BT-ARM](https://jenkins-internal.mbed.com/job/mbed-crypto-pr/job/PR-305-merge/6/execution/node/6712/log/), [authcrypt-CY8CKIT_062_WIFI_BT-GCC_ARM](https://jenkins-internal.mbed.com/job/mbed-crypto-pr/job/PR-305-merge/6/execution/node/5655/log/), [hashing-CY8CKIT_062_WIFI_BT-ARM](https://jenkins-internal.mbed.com/job/mbed-crypto-pr/job/PR-305-merge/6/execution/node/5616/log/), [hashing-CY8CKIT_062_WIFI_BT-ARM](https://jenkins-internal.mbed.com/job/mbed-crypto-pr/job/PR-305-merge/6/execution/node/6081/log/). I don't understand what's going on there.

gilles-peskine-arm commented

2019-10-29 09:07:45 -05:00

(Migrated from github.com)

The non-reproducible CY8CKIT_062_WIFI_BT failures look like one board is non-responsive and other boards are working. There are enough passes to show that this PR is ok.

Patater commented

2019-10-29 12:02:51 -05:00

(Migrated from github.com)

CI has at least one bad Cypress board in it causing tests to not complete successfully. I manually tested our mbed-os-example-mbed-crypto/getting-started example on CY8CKIT_062_WIFI_BT with GCC_ARM and the merge result of this PR with development. This run went well.

With ARMC6, the example run hung after printing the following, which is a known issue:

Import an AES key...	Imported a key
Sign a message...

This is due to the version of ARMC6 (Component: ARM Compiler 6.10.1 Tool: armclang [5d143200]) having an optimizer bug. Using a custom build profile to set cflags to -O1, the example run completes successfully.

CI has at least one bad Cypress board in it causing tests to not complete successfully. I manually tested our mbed-os-example-mbed-crypto/getting-started example on `CY8CKIT_062_WIFI_BT` with `GCC_ARM` and the merge result of this PR with `development`. This run went well. With `ARMC6`, the example run hung after printing the following, which is a known issue: ``` Import an AES key... Imported a key Sign a message... ``` This is due to the version of `ARMC6` (`Component: ARM Compiler 6.10.1 Tool: armclang [5d143200]`) having an optimizer bug. Using a custom build profile to set cflags to `-O1`, the example run completes successfully.

👍 1

dgreen-arm (Migrated from github.com) approved these changes

2019-10-30 08:07:57 -05:00

dgreen-arm (Migrated from github.com) left a comment

PR looks good to me

yanesca commented

2019-11-07 05:49:32 -06:00

(Migrated from github.com)

@gilles-peskine-arm What is the meaning of the fixup! prefix in the commit messages? (I just find it confusing, because I usually mark commits like that if I later want to rebase and fixup some other commits with them.)

@gilles-peskine-arm What is the meaning of the `fixup!` prefix in the commit messages? (I just find it confusing, because I usually mark commits like that if I later want to rebase and fixup some other commits with them.)

gilles-peskine-arm commented

2019-11-07 06:19:43 -06:00

(Migrated from github.com)

@yanesca It's a git convention and feature indicating that the commit is an amendment to the previous commit with the same title minus the fixup! prefix. This convention is built into the --autosquash option of git rebase.

Normally a fixup is only in work in progress and shouldn't be pushed: you should rebase and squash the fixup before pushing. But here the defective commit was already present in the history, so the amendment can't be squashed.

@yanesca It's a git convention and feature indicating that the commit is an amendment to the previous commit with the same title minus the `fixup! ` prefix. This convention is built into the [`--autosquash` option of `git rebase`](https://git-scm.com/docs/git-rebase#Documentation/git-rebase.txt---autosquash). Normally a fixup is only in work in progress and shouldn't be pushed: you should rebase and squash the fixup before pushing. But here the defective commit was already present in the history, so the amendment can't be squashed.

👍 1

Sign in to join this conversation.

No reviewers

k-stachowiak

dgreen-arm