psa: Support RSA signature without MBEDTLS_GENPRIME #4513

Patater · 2021-05-14T09:34:54Z

Description

On space-constrained platforms, it is a useful configuration to be able
to import/export and perform RSA key pair operations, but to exclude RSA
key generation, potentially saving flash space. It is not possible to
express this with the PSA_WANT_ configuration system at the present
time. However, in previous versions of Mbed TLS (v2.24.0 and earlier) it
was possible to configure a software PSA implementation which was
capable of making RSA signatures but not capable of generating RSA keys.
To do this, one unset MBEDTLS_GENPRIME.

Since the addition of MBEDTLS_PSA_BUILTIN_KEY_TYPE_RSA_KEY_PAIR, this
expressivity was lost. Expressing that you wanted to work with RSA key
pairs forced you to include the ability to generate key pairs as well.

Change psa_crypto_rsa.c to only call mbedtls_rsa_gen_key() if
MBEDTLS_GENPRIME is also set. This restores the configuration behavior
present in Mbed TLS v2.24.0 and earlier versions.

It left as a future exercise to add the ability to PSA to be able to
express a desire for a software or accelerator configuration that
includes RSA key pair operations, like signature, but excludes key pair
generation.

Without this change, linker errors will occur when attempts to call,
which doesn't exist when MBEDTLS_GENPRIME is unset.
psa_crypto_rsa.c.obj: in function rsa_generate_key': psa_crypto_rsa.c:320: undefined reference to mbedtls_rsa_gen_key'

Fixes #4512

Signed-off-by: Jaeden Amero [email protected]

Status

READY

Requires Backporting

Yes

development_2.x ([Backport 2.x] psa: Support RSA signature without MBEDTLS_GENPRIME #4546)

Migrations

If there is any API change, what's the incentive and logic for it.
NO

Additional comments

None

Todos

Tests
~~[ ] Documentation~~
Changelog updated
Backported ([Backport 2.x] psa: Support RSA signature without MBEDTLS_GENPRIME #4546)

Steps to test or reproduce

Build with MBEDTLS_GENPRIME unset.

Until we have a fix for Mbed-TLS/mbedtls#4512, we need to patch the fix during import time. Otherwise, we run into linker errors when PSA attempts to use RSA key generation, which we've excluded. This patch is extracted from Mbed-TLS/mbedtls#4513

Patater · 2021-05-18T10:40:20Z

@paul-elliott-arm Could you verify that the needs: work and needs: ci labels are correct still? It appears that CI was flaky before (experimental TLS 1.3 test). Re-running CI since then, we now have a pass.

gilles-peskine-arm

Unfortunately the simple solution is not acceptable since it breaks other use cases.

gilles-peskine-arm · 2021-05-18T21:43:30Z

include/mbedtls/config_psa.h

@@ -593,8 +593,10 @@ extern "C" {
 #define MBEDTLS_PSA_BUILTIN_ALG_RSA_PSS 1
 #define PSA_WANT_ALG_RSA_PSS 1
 #endif /* MBEDTLS_PKCS1_V21 */
+#if defined(MBEDTLS_GENPRIME)


This also disables signature and decryption.

Unfortunately, there is no way to express “private-key operations but not key generation” in the current PSA crypto configuration system. This is a known limitation. Resolving this limitation will require more design work.

Maybe this is a bug, but I am able to perform a signature using an imported RSA key pair even when MBEDTLS_GENPRIME is not set (without this fix in Mbed TLS v2.24.0, and with this fix in v2.25.0). I can provide a sample application if that's helpful.

What's the intention behind both MBEDTLS_PSA_BUILTIN_KEY_TYPE_RSA_KEY_PAIR and MBEDTLS_PSA_BUILTIN_KEY_TYPE_RSA_PUBLIC_KEY ? Why differentiate?

The fix here provides symmetry in what MBEDTLS_PSA_BUILTIN_KEY_TYPE_RSA_KEY_PAIR sets, and what TLS options enable it.

There was (in Mbed TLS v2.24.0) a way to express “private-key operations but not key generation” in the PSA crypto configuration system. Was it intentional to remove this in v2.25.0 and up?

The intention between differentiating have-key-pair and have-public-key is to allow builds that have public-key operations only (typically: I want to verify some signatures, but I'm not going to sign anything), which can save significant code size if the implementation is structured for it: you don't need any blinding code, CRT parameter handling, etc. Mbed TLS is not yet structured for it, but we'll work on that in the coming months.

Private key operations may have worked accidentally even when only PSA_WANT_KEY_TYPE_RSA_PUBLIC_KEY was enabled and not PSA_WANT_KEY_TYPE_RSA_KEY_PAIR, but we're progressively tightening down on the configuration, and also ramping up testing (#4444 will add systematic negative testing).

There was (in Mbed TLS v2.24.0) a way to express “private-key operations but not key generation” in the PSA crypto configuration system.

In the classic configuration system, yes. In the PSA configuration system, no. We didn't intend to remove anything, but we're aware that the new system doesn't exactly match the granularity of the old one.

I should also highlight that I'm not using MBEDTLS_PSA_CRYPTO_CONFIG or PSA_WANT_*. Indeed, with PSA_WANT_* there is no way to express "private-key operations but not key generation".

I am using the "old-style configuration mechanism", which it seems there is some intention to provide backwards compatibility with, hence my raising of this PR to help with that effort. If this aspect of PSA configuration isn't one you want to support because PSA_WANT_* can't express it yet, that's not great for my use case, but I can understand.

Another question would be if the configuration settings I've added to all.sh are considered invalid somehow. Is building with the default config minus MBEDTLS_GENPRIME (and no other changes) no longer supported?

Breaking configurations without MBEDTLS_GENPRIME was an unintended regression. I fully agree that we should fix it. But I don't want to fix it by breaking something else. I think it should be possible to fix it by adding (back?) a few #ifdef MBDETLS_GENPRIME in the right places.

With MBEDTLS_PSA_CRYPTO_CONFIG, MBEDTLS_GENPRIME is a mandatory consequence of PSA_WANT_KEY_TYPE_RSA_KEY_PAIR for the time being, that's a limitation to fix later.

As a quick fix, I think the best solution would be to add #if defined(MBEDTLS_GENPRIME) back into psa_crypto_*.c. That's not the long-term solution, because it only matters for the software implementation, it doesn't allow e.g. an accelerator to declare that it can accelerate RSA signatures but not RSA key generation. But it's a good short-term solution because it fixes the problem you care about without breaking (or worsening) things that currently work (-ish), and it doesn't require any new design that we're not sure is the right design.

gilles-peskine-arm · 2021-05-19T14:25:11Z

tests/scripts/all.sh

+component_build_psa_crypto_rsa_no_genprime() {
+    msg "build: default config minus MBEDTLS_GENPRIME"
+    scripts/config.py unset MBEDTLS_GENPRIME
+    make


Please run make test as well

Patater · 2021-05-19T15:05:02Z

Modified the RSA software implementation to use the MBEDTLS_GENPRIME define to gate calling of mbedtls_rsa_gen_key().

Patater · 2021-05-19T15:10:08Z

Also call make test, even though not necessary to run into the discovered linker error, it is helpful to see if the configuration without MBEDTLS_GENPRIME is broken in some other fashion.

Patater · 2021-05-19T15:15:19Z

One outstanding issue is if the accelerator configuration PSA_CRYPTO_DRIVER_TEST && MBEDTLS_PSA_ACCEL_KEY_TYPE_RSA_KEY_PAIR needs to work without MBEDTLS_GENPRIME being set.

#if ( defined(MBEDTLS_PSA_BUILTIN_KEY_TYPE_RSA_KEY_PAIR) ||  \
      ( defined(PSA_CRYPTO_DRIVER_TEST) &&                   \
        defined(MBEDTLS_PSA_ACCEL_KEY_TYPE_RSA_KEY_PAIR) ) )
#define BUILTIN_KEY_TYPE_RSA_KEY_PAIR    1
#endif

gilles-peskine-arm · 2021-05-19T15:35:41Z

library/psa_crypto_rsa.c

@@ -274,7 +274,10 @@ static psa_status_t rsa_export_public_key(
 #endif /* defined(BUILTIN_KEY_TYPE_RSA_KEY_PAIR) ||
        * defined(BUILTIN_KEY_TYPE_RSA_PUBLIC_KEY) */

-#if defined(BUILTIN_KEY_TYPE_RSA_KEY_PAIR)
+/* XXX Does MBEDTLS_PSA_ACCEL_KEY_TYPE_RSA_KEY_PAIR use these functions? Both
+ * of them or just rsa_generate_key? */


Currently only rsa_generate_key.

Does it use this implementation of rsa_generate_key() or is another rsa_generate_key() substituted in at link time?

We don't do link-time substitution.

library/psa_crypto_rsa.c

Patater · 2021-05-20T12:29:33Z

Updated with removal of XXX review comment question and addition of MBEDTLS_GENPRIME ifdef in psa_generate_key_internal() to gate calling mbedtls_psa_rsa_generate_key().

The test "PSA generate key: RSA, 1024 bits, good, encrypt (OAEP SHA-256)" had a dependency on MBEDTLS_GENPRIME, but this was not listed in the dependencies. Add MBEDTLS_GENPRIME to the test's dependencies to ensure it has what it needs to run. Signed-off-by: Jaeden Amero <[email protected]>

On space-constrained platforms, it is a useful configuration to be able to import/export and perform RSA key pair operations, but to exclude RSA key generation, potentially saving flash space. It is not possible to express this with the PSA_WANT_ configuration system at the present time. However, in previous versions of Mbed TLS (v2.24.0 and earlier) it was possible to configure a software PSA implementation which was capable of making RSA signatures but not capable of generating RSA keys. To do this, one unset MBEDTLS_GENPRIME. Since the addition of MBEDTLS_PSA_BUILTIN_KEY_TYPE_RSA_KEY_PAIR, this expressivity was lost. Expressing that you wanted to work with RSA key pairs forced you to include the ability to generate key pairs as well. Change psa_crypto_rsa.c to only call mbedtls_rsa_gen_key() if MBEDTLS_GENPRIME is also set. This restores the configuration behavior present in Mbed TLS v2.24.0 and earlier versions. It left as a future exercise to add the ability to PSA to be able to express a desire for a software or accelerator configuration that includes RSA key pair operations, like signature, but excludes key pair generation. Without this change, linker errors will occur when attempts to call, which doesn't exist when MBEDTLS_GENPRIME is unset. psa_crypto_rsa.c.obj: in function `rsa_generate_key': psa_crypto_rsa.c:320: undefined reference to `mbedtls_rsa_gen_key' Fixes Mbed-TLS#4512 Signed-off-by: Jaeden Amero <[email protected]>

Patater · 2021-05-20T16:09:47Z

Updated to add missing dependency to "PSA generate key: RSA, 1024 bits, good, encrypt (OAEP SHA-256)". It wasn't depending on MBEDTLS_GENPRIME, but should have been.

gilles-peskine-arm

Looks good to me. Please make a backport to development_2.x.

gilles-peskine-arm · 2021-05-20T20:38:25Z

ChangeLog.d/psa-without-genprime-fix.txt

+   * Restore the ability to configure PSA via Mbed TLS options to support RSA
+     key pair operations but exclude RSA key generation. When MBEDTLS_GENPRIME
+     is not defined PSA will no longer attempt to use mbedtls_rsa_gen_key().


I think this could just have said “Fix the build when MBEDTLS_GENPRIME is disabled.” But ok.

Patater · 2021-05-21T09:26:17Z

Raised backport to development_2.x #4546

Until we have a fix for Mbed-TLS/mbedtls#4512, we need to patch the fix during import time. Otherwise, we run into linker errors when PSA attempts to use RSA key generation, which we've excluded. This patch is extracted from Mbed-TLS/mbedtls#4513

Patater mentioned this pull request May 14, 2021

tls: Upgrade to Mbed TLS v2.25.0 ARMmbed/mbed-os#14652

Merged

Patater force-pushed the psa-without-genprime-fix branch from 5d6c048 to 0b6d6ae Compare May 14, 2021 10:06

paul-elliott-arm added Arm Contribution bug needs-ci Needs to pass CI tests 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 needs-work size-s Estimated task size: small (~2d) labels May 14, 2021

gilles-peskine-arm requested changes May 18, 2021

View reviewed changes

gilles-peskine-arm removed needs-ci Needs to pass CI tests 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 fix available labels May 18, 2021

gilles-peskine-arm reviewed May 19, 2021

View reviewed changes

Patater force-pushed the psa-without-genprime-fix branch from 0b6d6ae to 2d89d08 Compare May 19, 2021 15:03

Patater force-pushed the psa-without-genprime-fix branch from 2d89d08 to 63bf66b Compare May 19, 2021 15:09

gilles-peskine-arm reviewed May 19, 2021

View reviewed changes

library/psa_crypto_rsa.c Show resolved Hide resolved

ronald-cron-arm self-requested a review May 19, 2021 16:13

Patater changed the title ~~config: Fix PSA configuration backwards compat~~ psa: Support RSA signature without MBEDTLS_GENPRIME May 20, 2021

Patater force-pushed the psa-without-genprime-fix branch from 63bf66b to ada2a54 Compare May 20, 2021 12:28

Patater added 2 commits May 20, 2021 17:08

Patater force-pushed the psa-without-genprime-fix branch from ada2a54 to 424fa93 Compare May 20, 2021 16:09

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels May 20, 2021

gilles-peskine-arm approved these changes May 20, 2021

View reviewed changes

gilles-peskine-arm added the needs-backports Backports are missing or are pending review and approval. label May 20, 2021

gilles-peskine-arm mentioned this pull request May 25, 2021

[Backport 2.x] psa: Support RSA signature without MBEDTLS_GENPRIME #4546

Merged

3 tasks

ronald-cron-arm approved these changes May 27, 2021

View reviewed changes

ronald-cron-arm added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels May 27, 2021

ronald-cron-arm merged commit 142c205 into Mbed-TLS:development May 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

psa: Support RSA signature without MBEDTLS_GENPRIME #4513

psa: Support RSA signature without MBEDTLS_GENPRIME #4513

Patater commented May 14, 2021 •

edited

Loading

Patater commented May 18, 2021 •

edited

Loading

gilles-peskine-arm left a comment

gilles-peskine-arm May 18, 2021

Patater May 19, 2021

gilles-peskine-arm May 19, 2021

Patater May 19, 2021

gilles-peskine-arm May 19, 2021

gilles-peskine-arm May 19, 2021

gilles-peskine-arm May 19, 2021

Patater commented May 19, 2021

Patater commented May 19, 2021 •

edited

Loading

Patater commented May 19, 2021

gilles-peskine-arm May 19, 2021

Patater May 19, 2021

gilles-peskine-arm May 19, 2021

Patater commented May 20, 2021 •

edited

Loading

Patater commented May 20, 2021 •

edited

Loading

gilles-peskine-arm left a comment

gilles-peskine-arm May 20, 2021

Patater commented May 21, 2021

psa: Support RSA signature without MBEDTLS_GENPRIME #4513

psa: Support RSA signature without MBEDTLS_GENPRIME #4513

Conversation

Patater commented May 14, 2021 • edited Loading

Description

Status

Requires Backporting

Migrations

Additional comments

Todos

Steps to test or reproduce

Patater commented May 18, 2021 • edited Loading

gilles-peskine-arm 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

Patater commented May 19, 2021

Patater commented May 19, 2021 • edited Loading

Patater commented May 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Patater commented May 20, 2021 • edited Loading

Patater commented May 20, 2021 • edited Loading

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Patater commented May 21, 2021

Patater commented May 14, 2021 •

edited

Loading

Patater commented May 18, 2021 •

edited

Loading

Patater commented May 19, 2021 •

edited

Loading

Patater commented May 20, 2021 •

edited

Loading

Patater commented May 20, 2021 •

edited

Loading