gh-ARMmbed/mbed-crypto

Watch

Allow xxx_drbg_set_entropy_len before xxx_drbg_seed #299

Merged

gilles-peskine-arm merged 3 commits from drbg-set_entropy_len into development

2019-10-18 09:39:04 -05:00

gilles-peskine-arm commented

2019-10-11 11:17:37 -05:00

(Migrated from github.com)

The functions mbedtls_ctr_drbg_set_entropy_len() and mbedtls_hmac_drbg_set_entropy_len() were only supported after the call to mbedtls_ctr_drbg_seed() and mbedtls_hmac_drbg_seed(), so the initial seeding always grabbed the amount of entropy defined at compile time (MBEDTLS_CTR_DRBG_ENTROPY_LEN for CTR_DRBG, calculated from the hash algorithm for HMAC_DRBG). There was no good reason for that and the documentation was not clear prior to https://github.com/ARMmbed/mbed-crypto/pull/287. Change the API to be intuitive: you can call set_entropy_len() after init(), and it will influence both the initial seeding (if you call it before seed()) and subsequent reseeds.

A side benefit is that this removes the need for the test-only function mbedtls_ctr_drbg_seed_entropy_len.

Upon reflection, the ways this would break existing applications are:

An application that called xxx_set_entropy_len() before xxx_drbg_seed and expected it to do nothing. That's no more likely than an application that called xxx_set_entropy_len() and expected this to set the entropy length throughout the lifetime of the DRBG object, and is currently unknowingly grabbing the default amount of entropy instead.
An application that called mbedtls_ctr_drbg_seed_entropy_len despite it being marked as for internal use only.
So I think this should be backported, perhaps with a backward compatibility wrapper mbedtls_ctr_drbg_seed_entropy_len for the sake of linkers that may have performed some cross-module inlining across a shared library call (hopefully that doesn't happen, but maybe we can spare a few bytes of code size to play it safe).

A step on the path to IOTCRYPT-180.

The functions `mbedtls_ctr_drbg_set_entropy_len()` and `mbedtls_hmac_drbg_set_entropy_len()` were only supported after the call to `mbedtls_ctr_drbg_seed()` and `mbedtls_hmac_drbg_seed()`, so the initial seeding always grabbed the amount of entropy defined at compile time (`MBEDTLS_CTR_DRBG_ENTROPY_LEN` for CTR_DRBG, calculated from the hash algorithm for HMAC_DRBG). There was no good reason for that and the documentation was not clear prior to https://github.com/ARMmbed/mbed-crypto/pull/287. Change the API to be intuitive: you can call `set_entropy_len()` after `init()`, and it will influence both the initial seeding (if you call it before `seed()`) and subsequent reseeds. A side benefit is that this removes the need for the test-only function `mbedtls_ctr_drbg_seed_entropy_len`. Upon reflection, the ways this would break existing applications are: * An application that called `xxx_set_entropy_len()` before `xxx_drbg_seed` and expected it to do nothing. That's no more likely than an application that called `xxx_set_entropy_len()` and expected this to set the entropy length throughout the lifetime of the DRBG object, and is currently unknowingly grabbing the default amount of entropy instead. * An application that called `mbedtls_ctr_drbg_seed_entropy_len` despite it being marked as for internal use only. So I think this should be backported, perhaps with a backward compatibility wrapper `mbedtls_ctr_drbg_seed_entropy_len` for the sake of linkers that may have performed some cross-module inlining across a shared library call (hopefully that doesn't happen, but maybe we can spare a few bytes of code size to play it safe). A step on the path to IOTCRYPT-180.

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

2019-10-15 10:16:04 -05:00

AndrzejKurek (Migrated from github.com) approved these changes

2019-10-17 06:39:17 -05:00

gilles-peskine-arm commented

2019-10-17 10:40:06 -05:00

(Migrated from github.com)

CI status: the failures are from tests on one embedded board that looks like an instability with this board affecting other jobs and the ABI compatibility check reporting that mbedtls_ctr_drbg_seed_entropy_len has disappeared as expected. Since the affected board doesn't interact with the entropy seeding differently from other boards, we can merge without a status on this board. So CI is ok for merging.

[CI status](https://jenkins-internal.mbed.com/job/mbed-crypto-pr/job/PR-299-merge/4/flowGraphTable/): the failures are from tests on one embedded board that looks like an instability with this board affecting other jobs and the ABI compatibility check reporting that `mbedtls_ctr_drbg_seed_entropy_len` has disappeared as expected. Since the affected board doesn't interact with the entropy seeding differently from other boards, we can merge without a status on this board. So CI is ok for merging.

gilles-peskine-arm commented

2019-10-18 09:28:01 -05:00

(Migrated from github.com)

CI passed except for ABI checking (as expected) and getting-started-CY8CKIT_062_WIFI_BT-IAR which fails with the same error (looks like a stack overflow) on development. Ok to merge.

[CI passed](https://jenkins-internal.mbed.com/blue/organizations/jenkins/mbed-crypto-pr/detail/PR-299-merge/6/pipeline) except for ABI checking ([as expected](https://github.com/ARMmbed/mbed-crypto/pull/299#issuecomment-543234790)) and `getting-started-CY8CKIT_062_WIFI_BT-IAR` which fails with the same error (looks like a stack overflow) on `development`. Ok to merge.

gavacq (Migrated from github.com) reviewed

2020-02-16 18:19:42 -06:00

library/ctr_drbg.c

Hi @gilles-peskine-arm, is there any reason you didin't use ctr_drbg_set_entropy_len() here? Maybe you wanted to keep some level of method independency.

Hi @gilles-peskine-arm, is there any reason you didin't use `ctr_drbg_set_entropy_len()` here? Maybe you wanted to keep some level of method independency.

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

2020-02-17 12:31:24 -06:00

library/ctr_drbg.c

I can't think of a strong reason one way or the other. Semantically, I want to set the entropy_len field to a certain value. mbedtls_ctr_drbg_set_entropy_len() does that, but so does an assignment. An assignment is simpler than calling a function. Arguably, calling a function is better in case the function changes. But changes to do what? If it changes to add some validation, this validation might not be what is needed at this point inside mbedtls_ctr_drbg_seed. And if the meaning of the field changes, there'll be other changes needed in the code, so who knows what would need to change in mbedtls_ctr_drbg_seed. In terms of code size, the assignment is a couple of bytes smaller than a function call. So I have a very slight preference for the direct assignment. But only very slight. I don't remember thinking about it explicitly at the time.

I can't think of a strong reason one way or the other. Semantically, I want to set the `entropy_len` field to a certain value. `mbedtls_ctr_drbg_set_entropy_len()` does that, but so does an assignment. An assignment is simpler than calling a function. Arguably, calling a function is better in case the function changes. But changes to do what? If it changes to add some validation, this validation might not be what is needed at this point inside `mbedtls_ctr_drbg_seed`. And if the meaning of the field changes, there'll be other changes needed in the code, so who knows what would need to change in `mbedtls_ctr_drbg_seed`. In terms of code size, the assignment is a couple of bytes smaller than a function call. So I have a very slight preference for the direct assignment. But only very slight. I don't remember thinking about it explicitly at the time.

gavacq (Migrated from github.com) reviewed

2020-02-23 17:29:52 -06:00

library/ctr_drbg.c

Thanks for the perspective.

Thanks for the perspective.

Sign in to join this conversation.

No reviewers

No labels

good first issue

HEADER FORMATTING ISSUE

Jira status: AWAITING REVIEW

Jira status: CLOSED

Jira status: DONE

Jira status: IN PROGRESS

Jira status: IN REVIEW

Jira status: OPEN

Jira status: REOPENED

Jira status: RESOLVED

mirror internally

needs: backports

needs: design review

needs: preceding PR

open for community contribution

Product Backlog

ready for merge

type: enhancement

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gh-ARMmbed/mbed-crypto!299

No description provided.