DRBG documentation improvements #287

gilles-peskine-arm · 2019-10-03T13:50:19Z

Improve the documentation of the CTR_DRBG and HMAC_DRBG modules. This part is a straightfoward forward-port of Mbed-TLS/mbedtls#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):

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 Mbed-TLS/mbedtls#2860, 2.7 in Mbed-TLS/mbedtls#2878.

Internal ref: IOTCRYPT-180, IOTCRYPT-457

* State explicit whether several numbers are in bits or bytes. * Clarify whether buffer pointer parameters can be NULL. * Explain the value of constants that are dependent on the configuration.

Document that a derivation function is used. Document the security strength of the DRBG depending on the compile-time configuration and how it is set up. In particular, document how the nonce specified in SP 800-90A is set. Mention how to link the ctr_drbg module with the entropy module.

Improve the formatting and writing of the documentation based on what had been done for CTR_DRBG. Document the maximum size and nullability of some buffer parameters.

NIST and many other sources call it a "personalization string", and certainly not "device-specific identifiers" which is actually somewhat misleading since this is just one of many things that might go into a personalization string.

Explain how MBEDTLS_CTR_DRBG_ENTROPY_LEN is set next to the security strength statement, rather than giving a partial explanation (current setting only) in the documentation of MBEDTLS_CTR_DRBG_ENTROPY_LEN.

Separate the cases that achieve a 128-bit strength and the cases that achieve a 256-bit strength.

In particular, don't use #MBEDTLS_xxx on macros that are undefined in some configurations, since this would be typeset with a literal '#'.

It's an on/off feature, so it should be listed in version_features.

k-stachowiak

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

k-stachowiak · 2019-10-04T13:52:05Z

include/mbedtls/hmac_drbg.h

+ *                      mbedtls_hmac_drbg_set_entropy_len().
+ *
+ * \note                The default entropy length is the security strength
+ *                      (converted from bits to bytes). You can override


Minor: This sentence is probably missing something like "You can override it with..."

The documentation of CTR_DRBG erroneously claimed that mbedtls_ctr_drbg_set_entropy_len() had an impact on the initial seeding. This is in fact not the case: mbedtls_ctr_drbg_seed() forces the initial seeding to grab MBEDTLS_CTR_DRBG_ENTROPY_LEN bytes of entropy. Fix the documentation and rewrite the discussion of the entropy length and the security strength accordingly.

The documentation of HMAC_DRBG erroneously claimed that mbedtls_hmac_drbg_set_entropy_len() had an impact on the initial seeding. This is in fact not the case: mbedtls_hmac_drbg_seed() forces the entropy length to its chosen value. Fix the documentation.

This is a variant toggle, not an extra feature, so it should be tested separately.

gilles-peskine-arm · 2019-10-07T14:03:08Z

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 · 2019-10-07T17:06:41Z

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.

AndrzejKurek · 2019-10-11T09:58:12Z

include/mbedtls/ctr_drbg.h

+/**< The key size in bytes used by the cipher.
+ *
+ * Compile-time choice: 16 bytes (128 bits)
+ * because #MBEDTLS_CTR_DRBG_USE_128_BIT_KEY is enabled.


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.

#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.

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

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 · 2019-10-11T10:06:03Z

include/mbedtls/ctr_drbg.h

+ *                        to achieve a 256-bit strength.
+ *                      - When using AES-128
+ *                        (\c MBEDTLS_CTR_DRBG_USE_128_BIT_KEY is enabled)
+ *                        \p len must be at least 16 (in bytes)


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).

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

gilles-peskine-arm · 2019-10-11T14:57:01Z

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

* ARMmbed#272: Insert doxygen comments on old algorithms so they appear in PSA documentation * ARMmbed#285: SE driver: make persistent data work * ARMmbed#279: Include IANA reference in the definition of ECC curves and DH groups * ARMmbed#287: DRBG documentation improvements * ARMmbed#297: Fix int overflow in mbedtls_asn1_get_int (Credit to OSS-Fuzz)

CTR_DRBG documentation clarifications

08875d4

* State explicit whether several numbers are in bits or bytes. * Clarify whether buffer pointer parameters can be NULL. * Explain the value of constants that are dependent on the configuration.

gilles-peskine-arm added bug Something isn't working enhancement New feature or request needs: review The pull request is ready for review. This generally means that it has no known issues. labels Oct 3, 2019

This was referenced Oct 3, 2019

Backport 2.7: DRBG documentation improvements Mbed-TLS/mbedtls#2878

Merged

Backport 2.16: DRBG documentation improvements Mbed-TLS/mbedtls#2860

Merged

gilles-peskine-arm added 15 commits October 4, 2019 11:09

CTR_DRBG: Document the maximum size of some parameters

944bc58

CTR_DRBG: improve the discussion of entropy length vs strength

6fdf0b3

More CTR_DRBG documentation improvements and clarifications

ec51dd1

HMAC_DRBG documentation improvements clarifications

74efcd2

Improve the formatting and writing of the documentation based on what had been done for CTR_DRBG. Document the maximum size and nullability of some buffer parameters.

HMAC_DRBG: improve the documentation of the entropy length

3457b5e

Consistently use \c NULL and \c 0

10f16ac

Do note that xxx_drbg_random functions reseed with PR enabled

2d8f069

HMAC_DRBG: note that the initial seeding grabs entropy for the nonce

5d9fd07

CTR_DRBG: make it easier to understand the security strength

017778e

Explain how MBEDTLS_CTR_DRBG_ENTROPY_LEN is set next to the security strength statement, rather than giving a partial explanation (current setting only) in the documentation of MBEDTLS_CTR_DRBG_ENTROPY_LEN.

CTR_DRBG: Improve the explanation of security strength

2884ba3

Separate the cases that achieve a 128-bit strength and the cases that achieve a 256-bit strength.

CTR_DRBG: more consistent formatting and wording

d0c64c8

In particular, don't use #MBEDTLS_xxx on macros that are undefined in some configurations, since this would be typeset with a literal '#'.

Move MBEDTLS_CTR_DRBG_USE_128_BIT_KEY to the correct section

1540e5b

It's an on/off feature, so it should be listed in version_features.

Add a note about CTR_DRBG security strength to config.h

7e27936

gilles-peskine-arm force-pushed the ctr_drbg-doc-nist-crypto branch from 5fe1885 to fa2bea9 Compare October 4, 2019 09:16

gilles-peskine-arm added needs: work The pull request needs rework before it can be merged. and removed needs: review The pull request is ready for review. This generally means that it has no known issues. labels Oct 4, 2019

k-stachowiak previously approved these changes Oct 4, 2019

View reviewed changes

gilles-peskine-arm added 2 commits October 4, 2019 18:33

gilles-peskine-arm dismissed k-stachowiak’s stale review via 77d4457 October 4, 2019 16:34

gilles-peskine-arm force-pushed the ctr_drbg-doc-nist-crypto branch from fa2bea9 to 77d4457 Compare October 4, 2019 16:34

gilles-peskine-arm removed the bug Something isn't working label Oct 4, 2019

gilles-peskine-arm added needs: review The pull request is ready for review. This generally means that it has no known issues. and removed needs: work The pull request needs rework before it can be merged. labels Oct 4, 2019

gilles-peskine-arm changed the title ~~DRBG documentation improvements and CTR_DRBG usage simplification~~ DRBG documentation improvements Oct 4, 2019

k-stachowiak previously approved these changes Oct 7, 2019

View reviewed changes

gilles-peskine-arm mentioned this pull request Oct 7, 2019

The selftest program fails with MBEDTLS_CTR_DRBG_USE_128_BIT_KEY #290

Closed

config.pl full: exclude MBEDTLS_CTR_DRBG_USE_128_BIT_KEY

e249c0e

This is a variant toggle, not an extra feature, so it should be tested separately.

gilles-peskine-arm dismissed k-stachowiak’s stale review via e249c0e October 7, 2019 14:00

gilles-peskine-arm requested a review from k-stachowiak October 7, 2019 17:06

gilles-peskine-arm mentioned this pull request Oct 7, 2019

Test MBEDTLS_CTR_DRBG_USE_128_BIT_KEY #291

Merged

k-stachowiak approved these changes Oct 8, 2019

View reviewed changes

AndrzejKurek self-requested a review October 11, 2019 08:06

AndrzejKurek self-assigned this Oct 11, 2019

AndrzejKurek reviewed Oct 11, 2019

View reviewed changes

AndrzejKurek approved these changes Oct 11, 2019

View reviewed changes

gilles-peskine-arm removed the needs: review The pull request is ready for review. This generally means that it has no known issues. label Oct 11, 2019

gilles-peskine-arm merged commit e5e9081 into ARMmbed:development Oct 11, 2019

gilles-peskine-arm mentioned this pull request Oct 29, 2019

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

DRBG documentation improvements #287

DRBG documentation improvements #287

Uh oh!

gilles-peskine-arm commented Oct 3, 2019 •

edited

Loading

Uh oh!

k-stachowiak left a comment

Uh oh!

k-stachowiak Oct 4, 2019

Uh oh!

gilles-peskine-arm commented Oct 7, 2019

Uh oh!

gilles-peskine-arm commented Oct 7, 2019

Uh oh!

AndrzejKurek Oct 11, 2019

Uh oh!

gilles-peskine-arm Oct 11, 2019

Uh oh!

AndrzejKurek Oct 11, 2019

Uh oh!

gilles-peskine-arm Oct 11, 2019

Uh oh!

AndrzejKurek Oct 11, 2019

Uh oh!

gilles-peskine-arm Oct 11, 2019

Uh oh!

gilles-peskine-arm commented Oct 11, 2019

Uh oh!

Uh oh!

DRBG documentation improvements #287

DRBG documentation improvements #287

Uh oh!

Conversation

gilles-peskine-arm commented Oct 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k-stachowiak left a comment

Choose a reason for hiding this comment

Uh oh!

k-stachowiak Oct 4, 2019

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm commented Oct 7, 2019

Uh oh!

gilles-peskine-arm commented Oct 7, 2019

Uh oh!

AndrzejKurek Oct 11, 2019

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Oct 11, 2019

Choose a reason for hiding this comment

Uh oh!

AndrzejKurek Oct 11, 2019

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Oct 11, 2019

Choose a reason for hiding this comment

Uh oh!

AndrzejKurek Oct 11, 2019

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Oct 11, 2019

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm commented Oct 11, 2019

Uh oh!

Uh oh!

gilles-peskine-arm commented Oct 3, 2019 •

edited

Loading