Driver hashes rsa v21 #6141

mpg · 2022-07-27T08:17:24Z

Implement #6098

Gatekeeping notes: no ChangeLog entry (part of a series, see #6146), no backport (new feature).

This PR also constitutes an example of how to adapt crypto modules (not governed by MBEDTLS_USE_PSA_CRYPTO) so that they can work in builds with driver-only hashes and no MD, by using PSA as a fall-back when MD is not present, to ensure backwards compatibility.

AndrzejKurek · 2022-07-28T09:14:30Z

include/mbedtls/mbedtls_config.h

@@ -1141,7 +1141,10 @@
 *
 * Enable support for PKCS#1 v2.1 encoding.
 *
- * Requires: MBEDTLS_MD_C, MBEDTLS_RSA_C
+ * Requires: MBEDTLS_RSA_C and (MBEDTLS_MD_C or MBEDTLS_PSA_CRYPTO_C).


Should this be

Suggested change

* Requires: MBEDTLS_RSA_C and (MBEDTLS_MD_C or MBEDTLS_PSA_CRYPTO_C).

* Requires: MBEDTLS_RSA_C and (MBEDTLS_MD_C or (MBEDTLS_PSA_CRYPTO_C and PSA_WANT_ALG_MD5) ).

?

I don't think so, there's nothing specific about MD5 here. However, I agree that we should clarify that if using MD, you also need the MBEDTLS_xxx_C for all the hashes you can to use, and otherwise you also need PSA_WANT_ALG_xxx for those hashes.

AndrzejKurek · 2022-07-28T09:19:06Z

library/rsa.c

+#if defined(MBEDTLS_MD_C)
+#define HASH_MAX_SIZE   MBEDTLS_MD_MAX_SIZE
+#else /* MBEDTLS_MD_C */
+#include "psa/crypto.h"


So we're OK with RSA using PSA even if MBEDTLS_USE_PSA_CRYPTO is not defined?

If MBEDTLS_PKCS1_V21 is defined then one of MBEDTLS_MD_C, MBEDTLS_PSA_CRYPTO_C must be defined. Otherwise we have build error:

https://github.com/Mbed-TLS/mbedtls/pull/6141/files#diff-4fc3811c7c53e1746e1056ac38a6462ad2160077a59898bbd2bc31f5938ac185L165

Yes, these can be defined, but MBEDTLS_USE_PSA_CRYPTO can be undefined to signal that we're not using PSA for (as its doc says) x509 and TLS library, and enable new APIs for using keys handled by PSA Crypto. I'm just wondering if it's readable that PSA can be used for something even though MBEDTLS_USE_PSA_CRYPTO is undefined.

The novelty (use of PSA) is only in builds that where never possible before, so I think it's quite OK from a compatibility perspective.

I don't think we ever meant to promise that nothing would use PSA when MBEDTLS_USE_PSA is not defined, but I agree that this is a reasonable way to interpret the current working, so the new behaviour could be surprising to users. I think we can handle that by adding a warning every time something that previously depended on MD now starts depending on "MD or PSA", similar to this one - so, eventually X.509 and TLS will get such a warning. And probably we should tune the wording on MBEDTLS_USE_PSA as well before the next release to accurately reflect the status as this point - I'll track that as part of #6146.

I just thought: anyway, the only case where PSA is used in RSA is when MD is not defined, and the only way you'll be able to compile TLS or X.509 without MD is by enabling USE_PSA (they will depend on MD_C || USE_PSA). So, X.509 and TLS are indeed only going to use PSA when USE_PSA is enabled.

AndrzejKurek · 2022-07-28T09:20:59Z

library/rsa.c

-        md_info = mbedtls_md_info_from_type( hash_id );
-        if( md_info == NULL )
+        /* Just make sure this hash is supported in this build. */
+        if( mbedtls_hash_info_get_size( hash_id ) == 0 )


Minor:

Suggested change

if( mbedtls_hash_info_get_size( hash_id ) == 0 )

if( mbedtls_hash_info_psa_from_md( hash_id ) == PSA_ALG_NONE )

seems more appropriate

They're equivalent in practice, but I agree checking for NONE looks a bit better.

Fixed as suggested.

AndrzejKurek · 2022-07-28T09:25:49Z

library/rsa.c

@@ -1088,29 +1098,68 @@ int mbedtls_rsa_private( mbedtls_rsa_context *ctx,
 }

 #if defined(MBEDTLS_PKCS1_V21)
+#if !defined(MBEDTLS_MD_C)
+static int ret_from_status( psa_status_t status )


This function has future potential to be refactored to psa_to_mbedtls_error( psa_status_t status, int module ) to have it uniform across modules (shared part & module-specific).

Good point! Could come in handy for #6146 and #6147. What would be a good home for this function? Would library/hash_info.h work? (Since it's specific to MD.)

Btw, it should probably be renamed mbedtls_md_error_from_psa() since it translates to MD error, and for consistency for our naming scheme (the psa_ namespace is not for internal things) and the existing mbedtls_pk_error_from_psa().

I was thinking about using it globally across Mbed TLS, hence the int module argument - this way there would some module-specific errors first (in a switch( module ), followed by a fallback - presumably, set of shared error codes that are the same for every module.

Oh, sorry, I read too fast and missed that part. Yes, why not, it there's some code that can be shared between modules, we could save some code size that way. But I'm slightly worried that the function would become big and hard to read. Also, sometimes even within a module you need variants, see mbedtls_pk_error_from_psa_rsa() which may return either RSA or PK error codes.

I'm inclined to keep this focused on MD as a first step, then look into consolidation with other modules only later, unless you feel strongly otherwise.

Absolutely no strong feelings, it's just a thing that came to my mind, as I can see that we're duplicating similar code in a couple of places.

Agreed. Whoever adopts this PR, I suggest moving this function to library/hash_info.[ch] under the name mbedtls_md_error_from_psa(). That way, #6146, #6147 and #6149 can use it too. It doesn't solve the problem of cross-module near-duplication, but at least it will avoid exact-duplication for all cases where we silently fall back to PSA in the absence of MD.

Regarding the larger picture, @AndrzejKurek can you create an issue about that so we don't forget about it? I guess the "Use PSA: misc. gaps" EPIC would be a good place.

Renamed function ret_from_status->mbedtls_md_error_from_psa and moved to library/hash_info.[ch] as global function.

Issue added here: #6158 with a remark to start by assesing if the potential gain is worth it.

mprse

I did a quick review and looks good at first sight (no critical issues).

But it seems that rebase is now needed.

AndrzejKurek · 2022-07-28T10:41:49Z

tests/scripts/all.sh

@@ -1760,6 +1759,8 @@ component_test_psa_crypto_config_accel_rsa_signature () {

    scripts/config.py -f tests/include/test/drivers/config_test_driver.h set MBEDTLS_SHA1_C
    scripts/config.py -f tests/include/test/drivers/config_test_driver.h set MBEDTLS_SHA512_C
+    # We need to define either MD_C or all of the PSA_WANT_ALG_SHAxxx.
+    scripts/config.py -f tests/include/test/drivers/config_test_driver.h set MBEDTLS_MD_C


Since it's the PSA crypto config with acceleration test - wouldn't we want to go the PSA way to be closer to MD being handled by drivers as well?

From the commit message I think this way was picked as it is simpler at least for now:

Previously MD_C was auto-enabled based on the fact that ALG_RSA_PSS was
requested, but that's no longer the case since the previous commit.
We can fix this in one of two ways: either enable MD_C, or enable all
the PSA_WANT_ALG_SHA_xxx that are needed for test. Go for MD_C because
it's a single line and avoids having to enumerate a list that might grow
in the future.

Maybe we should have psa and no psa versions now?

Left current version.

AndrzejKurek · 2022-07-28T10:47:17Z

library/rsa.c

+#else
+    psa_hash_operation_t op = PSA_HASH_OPERATION_INIT;
+    psa_algorithm_t alg = mbedtls_psa_translate_md( md_alg );
+    psa_status_t status = PSA_SUCCESS;


I'm not sure if it's still canon, but initializing to an error was a good practice before :)

Looking at the code I think that status can be simply initialized to PSA_ERROR_CORRUPTION_DETECTED without any other changes.

As far as I remember from review of my PRs it is acceptable to initialize status to PSA_SUCCESS, but code path must be clear and the comment should be added.

Fixed as suggested + initialized also ret to error value.

AndrzejKurek · 2022-07-28T10:47:38Z

library/rsa.c

+#else
+    psa_hash_operation_t op = PSA_HASH_OPERATION_INIT;
+    psa_algorithm_t alg = mbedtls_psa_translate_md( md_alg );
+    psa_status_t status = PSA_SUCCESS;


Initializing to error remark same as in other places, although here it's a part of handling dlen==0

It seems that here it make sense to leave it as it is now since ret is also initialized to 0 and as you mentioned is is intended to handle dlen==0 case.

Left current version as it make sense in this case.

AndrzejKurek · 2022-07-28T10:53:06Z

I finished the first round of my review and it seems that things look good :)

Previously MD_C was auto-enabled based on the fact that ALG_RSA_PSS was requested, but that's no longer the case since the previous commit. We can fix this in one of two ways: either enable MD_C, or enable all the PSA_WANT_ALG_SHA_xxx that are needed for test. Go for MD_C because it's a single line and avoids having to enumerate a list that might grow in the future. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

sed -i -f md_or_psa_hash.sed \ tests/suites/test_suite_pkcs1_v21.data tests/suites/test_suite_pk.data with md_or_psa_hash.sed containing: s/MBEDTLS_MD5_C/MBEDTLS_HAS_ALG_MD5_VIA_MD_OR_PSA/g s/MBEDTLS_RIPEMD160_C/MBEDTLS_HAS_ALG_RIPEMD160_VIA_MD_OR_PSA/g s/MBEDTLS_SHA1_C/MBEDTLS_HAS_ALG_SHA_1_VIA_MD_OR_PSA/g s/MBEDTLS_SHA224_C/MBEDTLS_HAS_ALG_SHA_224_VIA_MD_OR_PSA/g s/MBEDTLS_SHA256_C/MBEDTLS_HAS_ALG_SHA_256_VIA_MD_OR_PSA/g s/MBEDTLS_SHA384_C/MBEDTLS_HAS_ALG_SHA_384_VIA_MD_OR_PSA/g s/MBEDTLS_SHA512_C/MBEDTLS_HAS_ALG_SHA_512_VIA_MD_OR_PSA/g (The only lines in pk.data that still had old-style dependencies where the ones about PKCS1_V21.) Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Previously the whole .function file had a global dependency on MBEDTLS_SHA1_C. This hasn't been correct for a long time: - on principle, dependency declarations in .function files are for compile-time dependencies; - in practice, a number of test cases do not depend on SHA-1, as they only use SHA-256 or SHA-512 - those cases should not be skipped in builds without SHA-1; - this was "taken advantage of" to skip dependency declarations for test cases that only depended on SHA-1. The previous commit removed the global dependency on SHA1_C; as a result the test cases that actually depend on SHA-1 were not skipped in builds without SHA-1. This commit fixes that by adding dependency declarations where they belong: in the .data file. All cases compute hashes using MD is available, or PSA otherwise; so MD_OR_PSA is appropriate here. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

The code will make the decision based on availability of MD, not of MD+this_hash. The later would only be possible at runtime (the hash isn't known until then, that's the whole point of MD), so we'd need to have both MD-based and PSA-based code paths in a single build, which would have a very negative impact on code size. So, instead, we choose based on the presence of MD, which is know at compile time, so we only have one of the two code paths in each build. Adjust the macros so that they match the logic of the code using them. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Signed-off-by: Przemek Stekiel <przemyslaw.stekiel@mobica.com>

mprse · 2022-08-11T10:59:05Z

I rebased the PR on top of development as we normally do. PR was already reviewed and accepted by both reviewers, so this should not be a problem.
Previously I wasn't able to do a force push probably because of network connection problem.

EDIT:
Resolved conflicts:
In file tests/suites/test_suite_pk.data removed radix test arguments as they were removed form pk_rsa_verify_ext_test_vec test function.

AndrzejKurek

LGTM

Since Mbed-TLS#6141 it can "fall back" to PSA when MD is not available (but will use MD if available, to preserve backwards compatibility). Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg self-assigned this Jul 27, 2022

mpg added DO-NOT-MERGE needs-ci Needs to pass CI tests labels Jul 27, 2022

mpg force-pushed the driver-hashes-rsa-v21 branch from 2a79d06 to e0aec86 Compare July 27, 2022 22:01

mpg mentioned this pull request Jul 28, 2022

Driver-only hashes: PKCS5 #6145

Closed

8 tasks

mpg force-pushed the driver-hashes-rsa-v21 branch from e0aec86 to c95eee6 Compare July 28, 2022 08:47

mpg 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 priority-high High priority - will be reviewed soon and removed DO-NOT-MERGE needs-ci Needs to pass CI tests labels Jul 28, 2022

mpg marked this pull request as ready for review July 28, 2022 08:50

mpg added this to To Do in Roadmap Board for Mbed TLS via automation Jul 28, 2022

AndrzejKurek reviewed Jul 28, 2022

View reviewed changes

mpg mentioned this pull request Jul 28, 2022

Driver-only hashes: PKCS12 #6147

Closed

9 tasks

AndrzejKurek reviewed Jul 28, 2022

View reviewed changes

mprse reviewed Jul 28, 2022

View reviewed changes

This was referenced Jul 28, 2022

Create ChangeLog entry for "driver-only" work so far #6146

Closed

Driver-based builds: RSA 2.1 (OAEP, PSS) without software hashes mpg/mbedtls#4

Closed

AndrzejKurek reviewed Jul 28, 2022

View reviewed changes

mpg force-pushed the driver-hashes-rsa-v21 branch from c95eee6 to c5e9dfe Compare July 28, 2022 10:51

This was referenced Jul 28, 2022

Driver-only hashes: EC J-PAKE #6149

Closed

Driver-only hashes: X.509 #6127

Closed

Driver-only hashes: TLS 1.2: test_suite_ssl #6128

Closed

mpg removed their assignment Jul 28, 2022

mpg and others added 8 commits August 11, 2022 12:47

Use more suitable function for checking if hash is supported

712bb9c

Signed-off-by: Przemek Stekiel <przemyslaw.stekiel@mobica.com>

make ret_from_status() global function and move it to has_info.[ch]

2aae040

Signed-off-by: Przemek Stekiel <przemyslaw.stekiel@mobica.com>

Initialize status/ret to error value

f98b57f

Signed-off-by: Przemek Stekiel <przemyslaw.stekiel@mobica.com>

Fix include file path

71bf28b

Signed-off-by: Przemek Stekiel <przemyslaw.stekiel@mobica.com>

mprse dismissed wernerlewis’s stale review via 71bf28b August 11, 2022 10:55

mprse force-pushed the driver-hashes-rsa-v21 branch from a81f23a to 71bf28b Compare August 11, 2022 10:55

Roadmap Board for Mbed TLS automation moved this from Has Approval to In Development Aug 11, 2022

mprse requested review from wernerlewis and AndrzejKurek and removed request for AndrzejKurek August 11, 2022 10:59

mprse added the needs-review Every commit must be reviewed by at least two team members, label Aug 11, 2022

wernerlewis approved these changes Aug 11, 2022

View reviewed changes

bensze01 moved this from In Development to Has Approval in Roadmap Board for Mbed TLS Aug 11, 2022

mprse removed the needs-ci Needs to pass CI tests label Aug 11, 2022

superna9999 mentioned this pull request Aug 11, 2022

Driver-only hashes: EC J-PAKE #6195

Merged

4 tasks

AndrzejKurek approved these changes Aug 12, 2022

View reviewed changes

mprse 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 Aug 12, 2022

daverodgman merged commit a7448bf into Mbed-TLS:development Aug 16, 2022

Roadmap Board for Mbed TLS automation moved this from Has Approval to Done Aug 16, 2022

mprse mentioned this pull request Aug 18, 2022

Driver-only hashes: X.509 #6171

Merged

mpg mentioned this pull request Aug 30, 2022

Driver-only hashes: RSA 2.1 #6098

Closed

mpg mentioned this pull request Oct 27, 2022

Implement md dispatch through PSA (3.2 edition) #6474

Closed

daverodgman removed this from Done in Roadmap Board for Mbed TLS Apr 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Driver hashes rsa v21 #6141

Driver hashes rsa v21 #6141

mpg commented Jul 27, 2022 •

edited

Loading

AndrzejKurek Jul 28, 2022 •

edited

Loading

mpg Jul 28, 2022 •

edited

Loading

AndrzejKurek Jul 28, 2022

mprse Jul 28, 2022

AndrzejKurek Jul 28, 2022

mpg Jul 28, 2022

mpg Jul 28, 2022

AndrzejKurek Jul 28, 2022

mpg Jul 28, 2022

mprse Jul 29, 2022

AndrzejKurek Jul 28, 2022 •

edited

Loading

mpg Jul 28, 2022

AndrzejKurek Jul 28, 2022

mpg Jul 28, 2022

AndrzejKurek Jul 28, 2022

mpg Jul 28, 2022

mprse Jul 29, 2022

AndrzejKurek Jul 29, 2022

mprse left a comment

AndrzejKurek Jul 28, 2022

mprse Jul 29, 2022

mprse Jul 29, 2022

AndrzejKurek Jul 28, 2022

mprse Jul 29, 2022

mprse Jul 29, 2022

AndrzejKurek Jul 28, 2022 •

edited

Loading

mprse Jul 29, 2022

mprse Jul 29, 2022

AndrzejKurek commented Jul 28, 2022

mprse commented Aug 11, 2022 •

edited

Loading

AndrzejKurek left a comment

	* Requires: MBEDTLS_RSA_C and (MBEDTLS_MD_C or MBEDTLS_PSA_CRYPTO_C).
	* Requires: MBEDTLS_RSA_C and (MBEDTLS_MD_C or (MBEDTLS_PSA_CRYPTO_C and PSA_WANT_ALG_MD5) ).

	if( mbedtls_hash_info_get_size( hash_id ) == 0 )
	if( mbedtls_hash_info_psa_from_md( hash_id ) == PSA_ALG_NONE )

Driver hashes rsa v21 #6141

Driver hashes rsa v21 #6141

Conversation

mpg commented Jul 27, 2022 • edited Loading

AndrzejKurek Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

mpg Jul 28, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrzejKurek Jul 28, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mprse 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

AndrzejKurek Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrzejKurek commented Jul 28, 2022

mprse commented Aug 11, 2022 • edited Loading

AndrzejKurek left a comment

Choose a reason for hiding this comment

mpg commented Jul 27, 2022 •

edited

Loading

AndrzejKurek Jul 28, 2022 •

edited

Loading

mpg Jul 28, 2022 •

edited

Loading

AndrzejKurek Jul 28, 2022 •

edited

Loading

AndrzejKurek Jul 28, 2022 •

edited

Loading

mprse commented Aug 11, 2022 •

edited

Loading