mbedtls icon indicating copy to clipboard operation
mbedtls copied to clipboard

Improve support of `mbedtls_psa_get_random` in client-only builds

Open mpg opened this issue 1 year ago • 2 comments

Context: PSA has as standard RNG API that is accessible from everywhere as soon as psa_crypto_init() has been called. Legacy crypto APIs tend to accept f_rng, p_rng arguments because the legacy crypto API doesn't inlucde a globally-accessible RNG. The function mbedtls_psa_get_random() and companion macro MBEDTLS_PSA_RANDOM_STATE exist as a bridge between the two worlds, in that they allow legacy crypto APIs to use the PSA RNG.

However, as they are currently implemented, unless MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG is defined, they assume the RNG is built-in. This does work nicely with client-only (CRYPTO_CLIENT && !CRYPTO_C) builds. (And possibly neither with driver-only builds if the DRBG were accelerated.)

I suggest we:

  • Change the implementation to be always the one currently use with MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG . That it, only use the standard PSA APIs and quite making assumption about how it's implemented internally.
  • Move the implementation from psa_crypto.c where (1) it doesn't belong (it's not a PSA API) and (2) it's guarded by PSA_CRYPTO_C as the rest of the file is, to psa_util.c where (1) it belongs (it's a PSA<->legacy bridge, declared in psa_util.h) and (2) it would be guarded only by PSA_CRYPTO_CLIENT.

mpg avatar Feb 14 '24 09:02 mpg

@gilles-peskine-arm Can you sanity-check that this makes sense and I didn't miss anything obvious?

Also, should this go in the "legacy<->PSA bridge" EPIC, as it's about improving an existing bridge, or in the "3.6 misc." EPIC (as a nice-to-have) as it's about client-only builds?

mpg avatar Feb 14 '24 09:02 mpg

Yes, that makes sense. I can't remember why I implemented it this way and I can't find any clue in the history of https://github.com/Mbed-TLS/mbedtls/pull/4110. I guess the issue was framed as exposing the internal PSA RNG abstraction to users. And maybe I wanted to minimize the overhead, but an extra function call is negligible compared to the crypto that has to take place.

I don't know which epic, it's not a release blocker anyway: I'd be ok with changing this in a patch update.

gilles-peskine-arm avatar Feb 14 '24 12:02 gilles-peskine-arm