Mbed-TLS / mbedtls

An open source, portable, easy to use, readable and flexible TLS library, and reference implementation of the PSA Cryptography API. Releases are on a varying cadence, typically around 3 - 6 months between releases.
https://www.trustedfirmware.org/projects/mbed-tls/
Other
5.56k stars 2.61k forks source link

PSA Interruptible operations need guarding with PSA_WANT defines #7029

Open paul-elliott-arm opened 1 year ago

paul-elliott-arm commented 1 year ago

From Gilles:

These functions should only be defined if some interruptible functions are supported. Most applications don't need interruptibility and don't want to pay for the code size.

In the application interface, we need to define PSA_WANT_xxx constants conveying which combinations of algorithms and key types have a restartable interface. For example, Mbed TLS only supports it for ECC with Weierstrass curves, not for Montgomery curves or RSA.

From the driver interface (if we even attempt to implement it right now), we need to deduce corresponding MBEDTLS_PSA_ACCEL_xxx and MBEDTLS_PSA_BUILTIN_xxx symbols.

For internal use, we'll need to derive symbols like PSA_WANT_SOME_INTERRUPTIBLE, PSA_WANT_SOME_INTERRUPTIBLE_SIGN, etc. to gate the relevant functions.

From further discussion with Gilles, although he is happy for this to go in a seperate PR, there are some discussions yet to be had about the granularity of PSA_WANT defines required. I need to check with him if we can just go with blanket cover for now, and add granularity later.

Edit (from @valeriosetti): while at this, please fix also https://github.com/Mbed-TLS/mbedtls/pull/9651#discussion_r1858239501

mpg commented 1 year ago

there are some discussions yet to be had

For me, this means this task is unlikely to be an S, so I'm taking the liberty of re-labeling to M. Please let me know if you disagree!

gilles-peskine-arm commented 3 months ago

In the application interface, we need to define PSA_WANT_xxx constants conveying which combinations of algorithms and key types have a restartable interface.

Actually, I doubt that we need this level of granularity. I propose to make it simpler: a single MBEDTLS_PSA_INTERRUPTIBLE_OPERATIONS that enables all interruptible APIs. It'll be effectively a new name for MBEDTLS_ECP_RESTARTABLE: define one from the other (the direction depends on MBEDTLS_PSA_CRYPTO_CONFIG, eventually we'll get rid of MBEDTLS_ECP_RESTARTABLE internally but that's a task for later that can wait until after 4.0).

gilles-peskine-arm commented 3 months ago

a single MBEDTLS_PSA_INTERRUPTIBLE_OPERATIONS that enables all interruptible APIs. It'll be effectively a new name for MBEDTLS_ECP_RESTARTABLE

No, this isn't right. We do want to allow builds where the API functions exist but the implementation doesn't do interruptibility, for applications that are using the interruptible functions for portability and that are compiled for a platform that doesn't do interruptibility for code size or attack surface.

paul-elliott-arm commented 3 months ago

It would certainly be useful to scope out the interruptible PSA_WANT defines here - the uncertainty of what is required is part of the reason this has not been done.

a single MBEDTLS_PSA_INTERRUPTIBLE_OPERATIONS that enables all interruptible APIs. It'll be effectively a new name for MBEDTLS_ECP_RESTARTABLE

No, this isn't right. We do want to allow builds where the API functions exist but the implementation doesn't do interruptibility, for applications that are using the interruptible functions for portability and that are compiled for a platform that doesn't do interruptibility for code size or attack surface.

Other than setting the number of ops per operation to bethe max value, we don't really have this functionality, though. Is it worth complicating this for something we don't have yet? As you say, a single define would be simple, a raft of defines somewhat more difficult, especially if we have to agree on what these defines are to start with. Could we start out with what you proposed, and go from there later?

gilles-peskine-arm commented 3 months ago

Is it worth complicating this for something we don't have yet?

The configuration interface is a user-facing interface, so we need to decide before 4.0.

As you say, a single define would be simple, a raft of defines somewhat more difficult, especially if we have to agree on what these defines are to start with. Could we start out with what you proposed, and go from there later?

I'm leaning towards not having a raft of defines. I'd initially thought we would need to have separate defines per mechanism, but on second thoughts, I think we don't need that in the API (PSA_WANT_xxx), only for drivers (MBEDTLS_PSA_ACCEL_xxx), and perhaps not even there. I think we need to allow three kinds of builds:

So the code structure would be something like this:

#if IOP_ENABLED
psa_iop_foo_start(op, args) {
    *op.args = *args;
#if MBEDTLS_ECP_RESTARTABLE
    if (is_ecp(mechanism)) return mbedtls_psa_ecp_iop_foo(op);
#endif
    return PSA_SUCCESS;
}

psa_iop_foo_complete(op, out) {
#if MBEDTLS_ECP_RESTARTABLE
    if (is_ecp(op.mechanism)) return mbedtls_psa_ecp_iop_foo(op, out);
#endif
    status = psa_foo(op.args, out);
    op.args = 0;
    return status;
}
#endif // IOP_ENABLED
gilles-peskine-arm commented 3 months ago

Design decision: we do want a general IOP_ENABLED option (name TBD). But we're not going to work on it right now, we'll add it later. Note that it's not just about the new functions, it's also about the existing signature functions.

For the time being, we continue having the iop functions always built, like the other API functions. Applications that don't use the functions should do link-time dead code elimination.

valeriosetti commented 1 day ago

I took the liberty to slightly modify the title because guards are now required also for interruptible key generation, not only signing.