gh-ARMmbed/mbed-crypto

Watch

Fork 0

DRBG documentation improvements #287

Merged

gilles-peskine-arm merged 19 commits from ctr_drbg-doc-nist-crypto into development

2019-10-11 09:57:46 -05:00

gilles-peskine-arm commented

2019-10-03 08:50:19 -05:00

(Migrated from github.com)

git diff upstream-public/pr/2860  -- include/mbedtls/{ctr,hmac}_drbg.h

I am working on a separate pull request for the following changes (separate PR because it was getting really messy):

mbedtls_hmac_drbg_seed() and mbedtls_ctr_drbg_seed() overrode the entropy length in the DRBG context, so calling mbedtls_hmac_drbg_set_entropy_len() or mbedtls_ctr_drbg_set_entropy_len() before calling xxx_seed() had no effect. Change the xxx_seed() functions to respect the entropy length set with xxx_set_entropy_len().
In CTR_DRBG, systematically grab a nonce from the entropy source. This removes the difficulty of using the module in a NIST-compliant way.

Backports: 2.16 in https://github.com/ARMmbed/mbedtls/pull/2860, 2.7 in https://github.com/ARMmbed/mbedtls/pull/2878.

Internal ref: IOTCRYPT-180, IOTCRYPT-457

Improve the documentation of the CTR_DRBG and HMAC_DRBG modules. This part is a straightfoward forward-port of https://github.com/ARMmbed/mbedtls/pull/2860: the 2.16 and 2.19 versions of `hmac_drbg.h` and `ctr_drbg.h` were near-identical and I changed them in identical ways. ``` git diff upstream-public/pr/2860 -- include/mbedtls/{ctr,hmac}_drbg.h ``` I am working on a separate pull request for the following changes (separate PR because it was getting really messy): 1. `mbedtls_hmac_drbg_seed()` and `mbedtls_ctr_drbg_seed()` overrode the entropy length in the DRBG context, so calling `mbedtls_hmac_drbg_set_entropy_len()` or `mbedtls_ctr_drbg_set_entropy_len()` before calling `xxx_seed()` had no effect. Change the `xxx_seed()` functions to respect the entropy length set with `xxx_set_entropy_len()`. 1. In CTR_DRBG, systematically grab a nonce from the entropy source. This removes the difficulty of using the module in a NIST-compliant way. Backports: 2.16 in https://github.com/ARMmbed/mbedtls/pull/2860, 2.7 in https://github.com/ARMmbed/mbedtls/pull/2878. Internal ref: IOTCRYPT-180, IOTCRYPT-457

k-stachowiak (Migrated from github.com) reviewed

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

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

No description provided.

I have only found one potential editing issue (unless I've got the sentence wrong).

k-stachowiak (Migrated from github.com) reviewed

2019-10-07 06:30:58 -05:00

gilles-peskine-arm commented

2019-10-07 09:03:08 -05:00

(Migrated from github.com)

CI failed in the Windows 2013 job. The failure is the CTR_DRBG selftest, The reason the selftest started failing is that I moved MBEDTLS_CTR_DRBG_USE_128_BIT_KEY to a different section in config.pl, so it got included in config.pl full. The selftest program has been failing when MBEDTLS_CTR_DRBG_USE_128_BIT_KEY is enabled ever since we introduced this option (#290) but we didn't realize until now because we were never enabling this option on the CI (#289).

I've excluded MBEDTLS_CTR_DRBG_USE_128_BIT_KEY from config.pl full. Fixing #289 and #290 is out of scope of this PR.

CI failed in the Windows 2013 job. The failure is the CTR_DRBG selftest, The reason the selftest started failing is that I moved `MBEDTLS_CTR_DRBG_USE_128_BIT_KEY` to a different section in `config.pl`, so it got included in `config.pl full`. The `selftest` program has been failing when `MBEDTLS_CTR_DRBG_USE_128_BIT_KEY` is enabled ever since we introduced this option (#290) but we didn't realize until now because we were never enabling this option on the CI (#289). I've excluded `MBEDTLS_CTR_DRBG_USE_128_BIT_KEY` from `config.pl full`. Fixing #289 and #290 is out of scope of this PR.

gilles-peskine-arm commented

2019-10-07 12:06:41 -05:00

(Migrated from github.com)

The merge job passed except for check-generated-files.sh in TLS which is a known issue with a fix pending.

The head job failed because of some Jenkins wonkiness that's unrelated to the PR.

The merge job passed except for `check-generated-files.sh` in TLS which is a known issue with a fix pending. The head job failed because of some Jenkins wonkiness that's unrelated to the PR.

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

2019-10-08 01:04:03 -05:00

AndrzejKurek (Migrated from github.com) reviewed

2019-10-11 04:58:12 -05:00

include/mbedtls/ctr_drbg.h

What's the difference between #MBEDTLS_CTR_DRBG_USE_128_BIT_KEY and \c MBEDTLS_CTR_DRBG_USE_128_BIT_KEY? Since I can see that both of these versions are used.

What's the difference between `#MBEDTLS_CTR_DRBG_USE_128_BIT_KEY` and `\c MBEDTLS_CTR_DRBG_USE_128_BIT_KEY`? Since I can see that both of these versions are used.

AndrzejKurek (Migrated from github.com) reviewed

2019-10-11 05:06:03 -05:00

include/mbedtls/ctr_drbg.h

Didn't you mention previously that in such case the entropy len has to be at least 24?

128 bits if AES-128 is used (\c MBEDTLS_CTR_DRBG_USE_128_BIT_KEY enabled)

and #MBEDTLS_CTR_DRBG_ENTROPY_LEN is set to 24 or more (which is

always the case unless it is explicitly set to a different value

in config.h).

Didn't you mention previously that in such case the entropy len has to be at least 24? > 128 bits if AES-128 is used (\c MBEDTLS_CTR_DRBG_USE_128_BIT_KEY enabled) > * and #MBEDTLS_CTR_DRBG_ENTROPY_LEN is set to 24 or more (which is > * always the case unless it is explicitly set to a different value > * in config.h).

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

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

include/mbedtls/ctr_drbg.h

#MACRO typesets MACRO and links to the definition of MACRO if MACRO is defined, but typesets #MACRO if MACRO is undefined, so it isn't good for preprocessor symbols that may or may not be defined. Hence the use of \c MACRO for symbols that may or may not be defined.

`#MACRO` typesets `MACRO` and links to the definition of `MACRO` if `MACRO` is defined, but typesets `#MACRO` if `MACRO` is undefined, so it isn't good for preprocessor symbols that may or may not be defined. Hence the use of `\c MACRO` for symbols that may or may not be defined.

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

2019-10-11 06:19:06 -05:00

include/mbedtls/ctr_drbg.h

24 for the initial seeding, 16 for subsequent reseeds. This function only influences subsequent reseeds. MBEDTLS_CTR_DRBG_ENTROPY_LEN influences both.

24 for the initial seeding, 16 for subsequent reseeds. This function only influences subsequent reseeds. `MBEDTLS_CTR_DRBG_ENTROPY_LEN` influences both.

AndrzejKurek (Migrated from github.com) reviewed

2019-10-11 09:01:38 -05:00

include/mbedtls/ctr_drbg.h

Shouldn't it be \c MBEDTLS_CTR_DRBG_USE_128_BIT_KEY everywhere then? Or will doxygen ignore blocks that won't get compiled?

Shouldn't it be `\c MBEDTLS_CTR_DRBG_USE_128_BIT_KEY` everywhere then? Or will doxygen ignore blocks that won't get compiled?

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

2019-10-11 09:08:09 -05:00

include/mbedtls/ctr_drbg.h

It can be #MBEDTLS_CTR_DRBG_USE_128_BIT_KEY here because this comment is surrounded by #ifdef MBEDTLS_CTR_DRBG_USE_128_BIT_KEY.

It can be `#MBEDTLS_CTR_DRBG_USE_128_BIT_KEY` here because this comment is surrounded by `#ifdef MBEDTLS_CTR_DRBG_USE_128_BIT_KEY`.

AndrzejKurek (Migrated from github.com) approved these changes

2019-10-11 09:43:16 -05:00

gilles-peskine-arm commented

2019-10-11 09:57:01 -05:00

(Migrated from github.com)

CI ok. PR approved, near-identical PR for 2.16 approved, 2.7 backport approved. This is good to merge.

[CI ok](https://github.com/ARMmbed/mbed-crypto/pull/287#issuecomment-539110867). PR approved, near-identical PR for 2.16 approved, 2.7 backport approved. This is good to merge.