Expose the PSA RNG in mbedtls #4110

gilles-peskine-arm · 2021-02-03T19:16:05Z

Expose whatever RNG the PSA subsystem uses to applications using the classic (mbedtls_xxx) API. This is especially useful for applications that use both PSA and TLS and want to leverage the PSA RNG to either save memory or make use of mbedtls_psa_external_get_random.

Resolves #3883.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

With MBEDTLS_USE_PSA_CRYPTO, some of the randomness for the TLS connection is generated inside the PSA crypto subsystem, which has no reproducible mode. Whether there is a nonzero amount of randomness coming from inside the PSA subsystem rather than from the random generator set by mbedtls_ssl_conf_rng() depends on the choice of cipher suite and other connection parameters as well as the level of support for MBEDTLS_USE_PSA_CRYPTO. Rather than give unreliable results, conservatively abort with a clear error message. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Whether MBEDTLS_USE_PSA_CRYPTO is enabled makes a significant difference with respect to how random generators are used (and, for no-HMAC_DRBG, how ECDSA signature is dispatched), so test both with and without it. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Test random generation as a whole. This is different from test_suite_*_drbg and test_suite_entropy, which respectively test PRNG modules and entropy collection. Start with basic tests: good-case tests, and do it twice and compare the results to validate that entropy collection doesn't repeat itself. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

stevew817

Overall, this PR looks good to me. It adds testing coverage to the work from #3895 and defines usage paradigms for the exposed functionality.

Just a couple of stylistic remarks.

stevew817 · 2021-02-10T13:00:54Z

include/mbedtls/psa_util.h

+ *                      `MBEDTLS_ERR_CTR_DRBG_xxx` or
+ *                      `MBEDTLS_ERR_HMAC_DRBG_xxx` on error.
+ */
+int mbedtls_psa_get_random( void *p_rng,


Isn't this a straight-up copy of what's in psa_crypto_random_impl.h? If this is meant to replace that, why isn't the other file being removed?

These are just the declarations that need to be in public headers. psa_crypto_random_impl.h has more declarations and definitions.

There's some duplication between the two headers, but I think it's the minimum necessary, or at least the minimum advisable. A couple of things are duplicated, in a way that a compiler will complain if they diverge, and this way all the implementation stuff is in psa_crypto_random_impl.h where it's easy to see all in one place, but we still get the compiler to complain if the implementation no longer matches the public interface.

Are you convinced by my explanation, or do some things need to be removed from psa_crypto_random_impl.h?

The duplication is fine by me but please add the reasons you mention here to the code documentation. It is not common to duplicate definitions thus better to have the reasons of the duplication.

Once again I'm not sure what change you expect. To me, the comments at the top of the files referring to each other are sufficient. Do you want this to be also mentioned in the documentation of each macro/function/type?

From your comment above the reason of the duplication seems to be:
"A couple of things are duplicated, in a way that a compiler will complain if they diverge, and this way all the implementation stuff is in psa_crypto_random_impl.h where it's easy to see all in one place, but we still get the compiler to complain if the implementation no longer matches the public interface."
I'm just saying what about to put something equivalent as documentation in the header files.

Revisiting this, the compiler doesn't enforce consistency: there's duplication here that's rejected if both headers are included (static variable definition, type definition). So I do need to remove some things from psa_crypto_random_impl.h.

I've removed the redundant definitions and kept redundant declarations where possible. I kept the duplication of the definition of the macro MBEDTLS_PSA_RANDOM_STATE because this is both permitted and checked by the C compiler.

stevew817 · 2021-02-10T13:01:49Z

ChangeLog.d/mbedtls_psa_get_random.txt

+   * Partial implementation of the PSA crypto driver interface: Mbed TLS can
+     now use an external random generator instead of the library's own
+     entropy collection and DRBG code. Enable MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG
+     and See the documentatio of mbedtls_psa_external_get_random() for details.


typos: see and documentation

stevew817 · 2021-02-10T13:02:45Z

ChangeLog.d/mbedtls_psa_get_random.txt

+     entropy collection and DRBG code. Enable MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG
+     and See the documentatio of mbedtls_psa_external_get_random() for details.
+   * Applications using PSA crypto can now use its random generator in the
+     mbedtls_xxx API. See the documentation of mbedtls_psa_get_random() for


Is it even recommended that applications using PSA Crypto continue to call directly into mbedtls_xxx? Having this wording in the public changelog makes it look like it's expected behaviour...

This is the opposite: applications using mbedtls_xxx features (typically X.509/SSL) and calling into PSA. Is the wording here misleading?

I think the wording might be too generic, in that case. To me (though my headspace is very much in crypto land and not in SSL) it reads like an implied approval to mix-and-match PSA and mbedtls_xxx for cryptographic functionality.

I reworded the second changelog entry. Is it clear now?

ronald-cron-arm

This looks almost good to me. I have a few comments though.

ronald-cron-arm · 2021-02-16T09:33:38Z

include/mbedtls/psa_util.h

+ *                      `MBEDTLS_ERR_CTR_DRBG_xxx` or
+ *                      `MBEDTLS_ERR_HMAC_DRBG_xxx` on error.
+ */
+int mbedtls_psa_get_random( void *p_rng,


The duplication is fine by me but please add the reasons you mention here to the code documentation. It is not common to duplicate definitions thus better to have the reasons of the duplication.

tests/suites/test_suite_random.data

programs/ssl/ssl_test_lib.c

programs/ssl/ssl_client2.c

programs/ssl/ssl_test_lib.h

Expose whatever RNG the PSA subsystem uses to applications using the mbedtls_xxx API. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

The SSL test programs can now use mbedtls_psa_get_random() rather than entropy+DRBG as a random generator. This happens if the configuration option MBEDTLS_USE_PSA_CRYPTO is enabled, or if MBEDTLS_TEST_USE_PSA_CRYPTO_RNG is set at build time. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Move the call to destroy the PSK to before freeing the SSL session data and calling rng_free(), which deinitializes the PSA subsystem. This particular ordering was chosen to make the ssl_client2 more similar to ssl_server2. This fixes the client failing on the psa_destroy_key() call in `ssl-opt.sh -f 'opaque psk on client'`. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

The dependency is on MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG plus MBEDTLS_PSA_CRYPTO_C. MBEDTLS_USE_PSA_CRYPTO is irrelevant. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

The point of having an external RNG is that you can disable all built-in RNG functionality: both the entropy part and the DRBG part. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

The SSL test programs can now use the PSA RNG, and the PSA RNG can use an external RNG. The build conditions hadn't been updated and didn't cover the case when MBEDTLS_TEST_USE_PSA_CRYPTO_RNG is enabled but MBEDTLS_USE_PSA_CRYPTO is disabled. Fix this. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

It's no longer restricted to MBEDTLS_USE_PSA_CRYPTO. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Also fix some typos. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm · 2021-02-16T14:49:42Z

I modified the history in the following way (previous history in https://github.com/gilles-peskine-arm/mbedtls/tree/psa-external-random-in-mbedtls-4):

New commit "Add comments to a few #endif closing a long chunk".
Fixed Expose the PSA RNG in mbedtls #4110 (comment) by amending "Fix psa_destroy_key called after mbedtls_psa_crypto_free".
Squashed the copypasta fix for Expose the PSA RNG in mbedtls #4110 (comment) into the commit that introduced it.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

There was some intentional duplication between library/psa_crypto_random_impl.h and include/mbedtls/psa_util.h, with the intent that the compiler would complain if one file was edited in a way that's incompatible with the other file. However, the two files were never included together, and in fact could not be included together because some definitions can't be duplicated (type, static variable). Now library/psa_crypto_random_impl.h includes include/mbedtls/psa_util.h, so the compiler will check what it can. There is less redundancy since it isn't always possible to declare something twice (specifically, types can't be declared). Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

stevew817

Nothing more from my side.

d3zd3z

Looks good to me.

daverodgman

Seems that all of Ronald's comments have been addressed. Merging as Ronald is out this week.

gilles-peskine-arm added 4 commits February 3, 2021 13:54

Fix exit status if the RNG seeding failed

4a23c98

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm added enhancement mbed TLS team component-crypto Crypto primitives and low-level interfaces needs-ci Needs to pass CI tests labels Feb 3, 2021

gilles-peskine-arm force-pushed the psa-external-random-in-mbedtls branch 2 times, most recently from cdf31a3 to 42c27ee Compare February 8, 2021 21:03

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review and removed needs-ci Needs to pass CI tests labels Feb 9, 2021

stevew817 self-requested a review February 9, 2021 12:30

stevew817 suggested changes Feb 10, 2021

View reviewed changes

ronald-cron-arm self-requested a review February 15, 2021 16:27

gilles-peskine-arm requested a review from stevew817 February 15, 2021 17:36

ronald-cron-arm removed the needs-reviewer This PR needs someone to pick it up for review label Feb 16, 2021

ronald-cron-arm requested changes Feb 16, 2021

View reviewed changes

gilles-peskine-arm added 8 commits February 16, 2021 15:39

Expose mbedtls_psa_get_random()

e3ed802

Expose whatever RNG the PSA subsystem uses to applications using the mbedtls_xxx API. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Add comments to a few #endif closing a long chunk

34babfc

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

The PSA external RNG does not require MBEDTLS_USE_PSA_CRYPTO

1dc19ff

The dependency is on MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG plus MBEDTLS_PSA_CRYPTO_C. MBEDTLS_USE_PSA_CRYPTO is irrelevant. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

In external_rng tests, disable the entropy module

38c12fd

The point of having an external RNG is that you can disable all built-in RNG functionality: both the entropy part and the DRBG part. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Fix the use of unitialized memory with MBEDTLS_TEST_NULL_ENTROPY

8e1e46e

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm added 4 commits February 16, 2021 15:46

Exclude random_twice tests with MBEDTLS_TEST_NULL_ENTROPY

ddeb889

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Initialize the test RNG whenever it's present

48113eb

It's no longer restricted to MBEDTLS_USE_PSA_CRYPTO. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Clarify where mbedtls_psa_get_random might be useful

d548d96

Also fix some typos. Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Explain the "external RNG large" test case

cd2d648

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm force-pushed the psa-external-random-in-mbedtls branch from 156809b to cd2d648 Compare February 16, 2021 14:46

gilles-peskine-arm added 2 commits February 16, 2021 16:49

Minor clarification in the public documentation

2cff7e2

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

gilles-peskine-arm requested a review from ronald-cron-arm February 16, 2021 15:55

Work around MSVC bug with duplicate static declarations

277a3a6

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

stevew817 approved these changes Feb 18, 2021

View reviewed changes

d3zd3z approved these changes Feb 19, 2021

View reviewed changes

daverodgman self-requested a review February 22, 2021 12:55

daverodgman approved these changes Feb 22, 2021

View reviewed changes

daverodgman merged commit d6ee36e into Mbed-TLS:development Feb 22, 2021

mpg mentioned this pull request Dec 30, 2022

driver-only ECDSA: fix testing disparities in ecp, random, se_driver_hal #6856

Closed

3 tasks

gilles-peskine-arm mentioned this pull request Feb 14, 2024

Improve support of mbedtls_psa_get_random in client-only builds #8825

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose the PSA RNG in mbedtls #4110

Expose the PSA RNG in mbedtls #4110

gilles-peskine-arm commented Feb 3, 2021 •

edited

Loading

stevew817 left a comment

stevew817 Feb 10, 2021

gilles-peskine-arm Feb 10, 2021

gilles-peskine-arm Feb 15, 2021

ronald-cron-arm Feb 16, 2021

gilles-peskine-arm Feb 16, 2021

ronald-cron-arm Feb 16, 2021

gilles-peskine-arm Feb 16, 2021

gilles-peskine-arm Feb 16, 2021 •

edited

Loading

stevew817 Feb 10, 2021

stevew817 Feb 10, 2021

gilles-peskine-arm Feb 10, 2021

stevew817 Feb 10, 2021

gilles-peskine-arm Feb 15, 2021

ronald-cron-arm left a comment

ronald-cron-arm Feb 16, 2021

gilles-peskine-arm commented Feb 16, 2021

stevew817 left a comment

d3zd3z left a comment

daverodgman left a comment •

edited

Loading

Expose the PSA RNG in mbedtls #4110

Expose the PSA RNG in mbedtls #4110

Conversation

gilles-peskine-arm commented Feb 3, 2021 • edited Loading

stevew817 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilles-peskine-arm Feb 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronald-cron-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Feb 16, 2021

stevew817 left a comment

Choose a reason for hiding this comment

d3zd3z left a comment

Choose a reason for hiding this comment

daverodgman left a comment • edited Loading

Choose a reason for hiding this comment

gilles-peskine-arm commented Feb 3, 2021 •

edited

Loading

gilles-peskine-arm Feb 16, 2021 •

edited

Loading

daverodgman left a comment •

edited

Loading