Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PSA: Specification for random generation and entropy drivers #3882

Merged

Conversation

gilles-peskine-arm
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm commented Nov 13, 2020

Create an interface for PSA random generation and entropy drivers.

@gilles-peskine-arm gilles-peskine-arm added needs-design-approval 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 labels Nov 13, 2020
@gilles-peskine-arm gilles-peskine-arm self-assigned this Nov 13, 2020

* The random output must be of cryptographic quality, with a uniform distribution. Therefore, if the random generator includes an entropy source, this entropy source must be fed through a CSPRNG (cryptographically secure pseudo-random number generator).
* Random generation is expected to be fast. (If a device can provide entropy but is slow at generating random data, declare it as an [entropy driver](#entropy-collection-entry-point) instead.)
* The random generator must be able to incorporate entropy provided by an outside source.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What of the cases where a hardware RNG doesn't accept external input?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, that driver would only be suitable as one entropy source. It's a matter of security design: if there are multiple entropy sources, the RNG should remain strong even if all but one sources are compromised.

I do think there is a fairly common design space where there is a single peripheral that includes an entropy source and a DRBG, and the integrator does not wish to save entropy to storage. In this case, add_entropy is not needed.

Looking at what I've written, I think that add_entropy can be made optional, but a driver without an add_entropy entry point will be less portable: it can only be integrated on platforms without other entropy sources and without saved entropy.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify that in the spec, then? It currently doesn't cater to those devices that have a single entropy source and CSPRNG prepackaged in a hardware peripheral, with no way of accessing these separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

* `"init_random"` (optional): if this function is present, [the core calls it once](#random-generator-initialization) after allocating a `"random_context_t"` object.
* `"add_entropy"` (entry point): the core calls this function to [inject entropy](#entropy-injection).
* `"get_random"` (entry point): the core calls this function whenever it needs to [obtain random data](#the-get_random-entry-point).
* `"initial_entropy_size"` (integer): the minimum number of bytes of entropy that the core must supply before the driver can output random data.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this allowed to be 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I'll clarify.

* `"add_entropy"` (entry point): the core calls this function to [inject entropy](#entropy-injection).
* `"get_random"` (entry point): the core calls this function whenever it needs to [obtain random data](#the-get_random-entry-point).
* `"initial_entropy_size"` (integer): the minimum number of bytes of entropy that the core must supply before the driver can output random data.
* `"reseed_entropy_size"` (integer): the minimum number of bytes of entropy that the core must supply when the driver runs out of entropy.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this allowed to be 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it makes sense for this to be 0. Either the driver never runs out of entropy and this value is not meaningful (although it can be a hint if the core wants to enforce prediction resistance), or the driver can run out of entropy (because its security policy treats entropy as consumable, which is mandated by some security standards) and then 0 is not valid.

@@ -191,6 +191,8 @@ The signature of a driver entry point generally looks like the signature of the

Some entry points are grouped in families that must be implemented as a whole. If a driver supports an entry point family, it must provide all the entry points in the family.

Drivers can also have entry points related to random generation. For transparent drivers, these are [random generation entry points](#random-generation-entry-points). For opaque drivers, these are [entropy collection entry points](#entropy-collection-entry-point).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the split? Neither of these have anything to do with key material, so shouldn't both be in the realm of transparent drivers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. It's closer to opaque drivers because you can have multiple entropy sources, whereas there is a single random generator. But it's only a very minor complication to allow entropy sources in transparent drivers.

@gilles-peskine-arm gilles-peskine-arm added component-crypto Crypto primitives and low-level interfaces enhancement HwDrivers labels Nov 13, 2020
@gilles-peskine-arm gilles-peskine-arm force-pushed the psa-random-driver-spec branch 2 times, most recently from 219944d to 8a54d03 Compare November 14, 2020 11:37
@@ -80,4 +80,10 @@ static inline int mbedtls_key_owner_id_equal( mbedtls_key_owner_id_t id1,

#endif /* MBEDTLS_PSA_CRYPTO_KEY_ID_ENCODES_OWNER */

#if defined(MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG)
typedef struct {
uint8_t opaque[32];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this very inflexible for something that is dependant on the underlying driver?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. This is a temporary place until we add support crypto_drivers.h. Once we finish implementing driver support, mbedtls_psa_external_random_context_t and mbedtls_psa_external_get_random will be generated from the driver description (although we may keep the ability to define the function manually until Mbed TLS 3.0 for backward compatibility). In the meantime, I don't know what to put here as a sample type. To keep the API simple, Mbed TLS allocates the context, so it needs to know the size.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not ideal to have to define this type, but I don't see a better solution that can be done quickly.

@@ -604,6 +605,11 @@
#error "MBEDTLS_PSA_INJECT_ENTROPY is not compatible with actual entropy sources"
#endif

#if defined(MBEDTLS_PSA_INJECT_ENTROPY) && \
!defined(MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you have inverted the logic here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in #3895.

* of random data and set \c *output_length to \p output_size.
*
* Requires: MBEDTLS_PSA_CRYPTO_C
*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be good to add a warning saying that this needs to be a cryptographic-quality RNG, since it will be used for generating keys?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. Addressed in #3895.

@stevew817
Copy link
Contributor

Maybe an additional changelog entry for MBEDTLS_PSA_CRYPTO_EXTERNAL_RNG?

@gilles-peskine-arm
Copy link
Contributor Author

To facilitate reviewing, I've split this pull request into specification and implementation. The previous version e618877 is at https://github.com/gilles-peskine-arm/mbedtls/tree/psa-random-driver-spec-3.

@daverodgman daverodgman requested a review from mpg November 19, 2020 10:19
@daverodgman daverodgman removed the needs-reviewer This PR needs someone to pick it up for review label Nov 19, 2020
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking pretty good! Other than two language issues, my only real concern is about initial_entropy_size defaulting to 0.

docs/proposed/psa-driver-interface.md Outdated Show resolved Hide resolved
docs/proposed/psa-driver-interface.md Show resolved Hide resolved
docs/proposed/psa-driver-interface.md Outdated Show resolved Hide resolved
@gilles-peskine-arm
Copy link
Contributor Author

As in #3878 (comment), there was a merge conflict due to the time stamp from a parallel edit, so I've moved the base to current development, removed the time stamp (using the same commit as on #3878) and rebased all the existing commits on top of the new base. The previous version is at https://github.com/gilles-peskine-arm/mbedtls/tree/psa-random-driver-spec-4. I'll address the remaining review comments in subsequent commits.

@gilles-peskine-arm
Copy link
Contributor Author

@ronald-cron-arm Do the remaining threads (#3882 (comment), #3882 (comment), #3882 (comment)) require action from me? I read them as questions and I don't think they justify changing the document, but as I understand it you aren't happy with the current state of the document, so can you please clarify what kind of changes you want in each case?

@ronald-cron-arm
Copy link
Contributor

I've proposed a change for one of the three comments that were still not resolved and resolved the two other ones.

@gilles-peskine-arm
Copy link
Contributor Author

Given that Manuel and Ronald are at most requesting minor changes, and there is no opposing voice, I consider this draft approved for design. I will circulate it more widely, especially among TRNG makers, before considering the design to be fully stable.

Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the changes since my previous approval and found nothing wrong.

Copy link
Contributor

@ronald-cron-arm ronald-cron-arm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as well.

@ronald-cron-arm ronald-cron-arm removed the needs-review Every commit must be reviewed by at least two team members, label Dec 11, 2020
@ronald-cron-arm ronald-cron-arm merged commit 8f05aeb into Mbed-TLS:development Dec 11, 2020
@bensze01 bensze01 added this to the Driver Interface: Removing unused code milestone Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-crypto Crypto primitives and low-level interfaces enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Break the dependency of psa_crypto on software AES
6 participants