Move RSA basic key parsing/writing to rsa.c #8740

valeriosetti · 2024-01-23T17:14:31Z

Description

This PR moves RSA private/public parsing functions from PK module to the RSA one. In this way the PK dependency of PSA crypto is removed even when building with RSA enabled.

Resolves #8647 (primary goal).
Resolves #5145 (in passing).

PR checklist

changelog provided
backport not required
tests provided

Signed-off-by: Valerio Setti <[email protected]>

…paque RSA key Signed-off-by: Valerio Setti <[email protected]>

These functions are meant to be used internally, so their prototype declaration is kept into rsa_internal.h. Signed-off-by: Valerio Setti <[email protected]>

Signed-off-by: Valerio Setti <[email protected]>

…ormat This feature was an unofficial extension which was never documented. Now that we are removing the PK dependency in order to use only functions from RSA module, PEM support is unavailable. Therefore we explicitly remove it. Signed-off-by: Valerio Setti <[email protected]>

Use new functions from the RSA module to parse and write private and public keys in PKCS#1 format. Signed-off-by: Valerio Setti <[email protected]>

mpg

Looking pretty good to me. My only points are about documentation and testing.

library/rsa_internal.h

tests/suites/test_suite_rsa.function

tests/suites/test_suite_rsa.data

tests/suites/test_suite_x509parse.data

The goal is to remove usage of PK return values in order to completely eliminate that dependency. This commit also updates pkparse and test_suite_x509parse to align with this change in return values. Signed-off-by: Valerio Setti <[email protected]>

Signed-off-by: Valerio Setti <[email protected]>

Do not disable RSA_C and related modules because now it does not automatically re-enable PK module. Signed-off-by: Valerio Setti <[email protected]>

Signed-off-by: Valerio Setti <[email protected]>

…pub keys Signed-off-by: Valerio Setti <[email protected]>

Signed-off-by: Valerio Setti <[email protected]>

…cies() Since we officially disabled support for importing of PEM formatted keys into PSA we removed dedicated tests from test_suite_psa_crypto. As a consequence MBEDTLS_PEM_PARSE_C is no more an exception for component_check_test_dependencies(). Signed-off-by: Valerio Setti <[email protected]>

gilles-peskine-arm · 2024-02-06T17:27:13Z

Er, right, fe329ce did pass the CI. Sorry for the confusion. I don't know what happened here: maybe I posted on the wrong tab (I was looking at several PR on my review queue)?

gilles-peskine-arm

LGTM apart from a check that isn't doing what it's supposed to do.

gilles-peskine-arm · 2024-02-06T18:16:28Z

tests/suites/test_suite_rsa.data

@@ -725,7 +725,7 @@ RSA parse public key - correct values, extra integer inside the SEQUENCE
 rsa_parse_pkcs1_key:1:"30818c028181009f091e6968b474f76f0e9c237c1d895996ae704b4f6d706acec8d2daac6209bf524aa3f658d0283adba1077f6cbe92e425dcde52290b239cade91be86c88425434986806e85734e159768f3dfea932baaa9409d25bace8ee9dce0cdde0903207299de575ae60feccf0daf82334ab83638539b0da74072f253acea8afc8e66bb70203010001020100":MBEDTLS_ERR_ASN1_LENGTH_MISMATCH

 RSA parse public key - correct values, extra integer outside the SEQUENCE
-rsa_parse_pkcs1_key:1:"308189028181009f091e6968b474f76f0e9c237c1d895996ae704b4f6d706acec8d2daac6209bf524aa3f658d0283adba1077f6cbe92e425dcde52290b239cade91be86c88425434986806e85734e159768f3dfea932baaa9409d25bace8ee9dce0cdde0903207299de575ae60feccf0daf82334ab83638539b0da74072f253acea8afc8e66bb70203010001":0
+rsa_parse_pkcs1_key:1:"308189028181009f091e6968b474f76f0e9c237c1d895996ae704b4f6d706acec8d2daac6209bf524aa3f658d0283adba1077f6cbe92e425dcde52290b239cade91be86c88425434986806e85734e159768f3dfea932baaa9409d25bace8ee9dce0cdde0903207299de575ae60feccf0daf82334ab83638539b0da74072f253acea8afc8e66bb70203010001020100":0


Hold on: since there's extra data, the function should return an error.

And it looks like we're missing a similar test case for key pairs.

And it looks like we're missing a similar test case for key pairs.

It's on line 664:
RSA parse private key - correct values, extra integer outside the SEQUENCE

Hold on: since there's extra data, the function should return an error.

This is basically the feedback I was waiting for after my last comment:

It makes more sense to me to do it this way because the user can provide a larger buffer which contains a valid key (priv or pub, it does not matter) and we are able to parse only the relevant data from there.

In my opinion, since the extra data is after the sequence it could make sense to have a success here because we're not corrupting the main SEQUENCE (it's just some extra data after it). However if you think that this must still be an error I will update the code accordingly ;)

I can't think of a direct security risk in accepting a key followed by arbitrary garbage, but I'm always uneasy accepting invalid inputs: it can often be a step in an attack in some weird circumstances that are hard to anticipate. So I would prefer to reject all invalid data unless there's a compelling reason.

gilles-peskine-arm · 2024-02-06T18:23:23Z

library/rsa.c

@@ -109,6 +109,10 @@ int mbedtls_rsa_parse_key(mbedtls_rsa_context *rsa, const unsigned char *key, si

    end = p + len;

+    if (end > (key + keylen)) {


This check never fires: at this point, the comparison is between the end of the SEQUENCE content and the end of the overall buffer, and a success from mbedtls_asn1_get_tag guarantees that they're in the desired order. I think the fix is to put this exact check just above end = p + len.

Same with parse_pubkey.

success from mbedtls_asn1_get_tag guarantees that they're in the desired order

Ops, that's a detail that I missed before. Thanks for the hint!

I think the fix is to put this exact check just above end = p + len.

Mmmm, but (if I'm not wrong) doing so would result in a never-firing-check again. end is set few lines above as end = p + keylen; (where p = (unsigned char *) key; at that point) and then mbedtls_asn1_get_tag() does not update it, so checking if (end > (key + keylen)) never fires. Am I wrong?

On the other hand if I replace if (end > (key + keylen)) with if (end != (key + keylen)) there are other test suites that start failing. For example test_suite_pkparse fails at:

Parse RSA Key #2 (Correct password) ............................... FAILED Parse RSA Key #4 (DES Encrypted) .................................. FAILED Parse RSA Key #5 (3DES Encrypted) ................................. FAILED Parse RSA Key #6 (AES-128 Encrypted) .............................. FAILED Parse RSA Key #7 (AES-192 Encrypted) .............................. FAILED Parse RSA Key #8 (AES-256 Encrypted) .............................. FAILED Parse RSA Key #9 (2048-bit, DES Encrypted) ........................ FAILED Parse RSA Key #10 (2048-bit, 3DES Encrypted) ...................... FAILED Parse RSA Key #11 (2048-bit, AES-128 Encrypted) ................... FAILED Parse RSA Key #12 (2048-bit, AES-192 Encrypted) ................... FAILED Parse RSA Key #13 (2048-bit, AES-256 Encrypted) ................... FAILED Parse RSA Key #14 (4096-bit, DES Encrypted) ....................... FAILED Parse RSA Key #15 (4096-bit, 3DES Encrypted) ...................... FAILED Parse RSA Key #16 (4096-bit, AES-128 Encrypted) ................... FAILED Parse RSA Key #17 (4096-bit, AES-192 Encrypted) ................... FAILED Parse RSA Key #18 (4096-bit, AES-256 Encrypted) ................... FAILED

I will have a look at this of course, but I'm not sure that having the extra data is a real bad thing (see my previous comment). Wdyt?

Edit: I just checked the first failure above (Parse RSA Key #2 (Correct password)) and the problem in this case is that the key is encrypted (AES-128-CBC) so PEM allocates a buffer whose length is the one of the encrypted key content, not the decrypted one. As a consequence the following call to mbedtls_rsa_parse_key fails.

There might be some history of sloppiness in pk parsing of encrypted keys, because the decryption code (mbedtls_pkcs5_pbes2 and mbedtls_pkcs12_pbe) used to not report the length of the plaintext. But nowadays pkparse calls mbedtls_pkcs5_pbes2_ext and mbedtls_pkcs12_pbe_ext, which do report the length of the plaintext. So I'd expect this to be easy to fix.

The failing cases I reported above are not related to pkcs5/12. For example in
Parse RSA Key #2 (Correct password)
the flow is:
mbedtls_pk_parse_key -> mbedtls_pem_read_buffer -> pem_aes_decrypt

Here len is not updated at the end of mbedtls_pem_read_buffer compared to what it was allocated from the first call to mbedtls_base64_decode() on the encrypted buffer. As a consequence the following call to mbedtls_rsa_parse_key fails.
The only way to prevent this would be to analyze the decrypted buffer to determine how long it is based on the SEQUENCE data, but that's part of the RSA parser in my opinion. Wdyt?

Oh, I'd forgotten we also had a decryption method in pem.c. It's a similar construction as PKCS5 and PKCS12: CBC where the old decryption code doesn't strip away the padding. For PKCS5 and PKCS12, we added _ext functions that do strip the padding. So I guess we should do the same thing in pem.c: remove PKCS7 padding after CBC decryption.

Since pem.c can be built without MBEDTLS_CIPHER_C, we can't use the cipher.c code (get_pkcs_padding) to strip the padding. On the other hand, since we're parsing the content, we don't have to worry about a padding oracle: there's a decryption oracle via the content anyway. So the padding check doesn't need to be constant-time.

To move on with this pull request which is already pretty long, I propose to merge it as is, and fix this in a follow-up.

Signed-off-by: Valerio Setti <[email protected]>

gilles-peskine-arm

Approved on the condition that we fix #8799 before the next release.

mpg

LGTM. Thanks Gilles for splitting #8799 out of this and to Valerio for identifying that the problem was with PEM decryption.

This is possible because after Mbed-TLS#8740 RSA_C no longer depends on PK to parse and write private/public keys. This commit also solves related issues that arose after this change in "pk.c" and "test_suite_pk". In particular now we can use rsa's module functions for parsing and writing keys without need to rely on pk_parse and pk_write functions. Signed-off-by: Valerio Setti <[email protected]>

valeriosetti added 6 commits January 23, 2024 08:35

pkparse: rename RSA key and pubkey parsing functions

fd49a46

Signed-off-by: Valerio Setti <[email protected]>

pkwrite: split pk_write_rsa_der() with a dedicated function for non-o…

429cd50

…paque RSA key Signed-off-by: Valerio Setti <[email protected]>

pk/rsa: move RSA parse/write private/public key functions to rsa module

b328c44

These functions are meant to be used internally, so their prototype declaration is kept into rsa_internal.h. Signed-off-by: Valerio Setti <[email protected]>

test_suite_rsa: add some basic testing of new parse/write priv/pub keys

8e6093d

Signed-off-by: Valerio Setti <[email protected]>

psa_crypt_rsa: remove dependency from the PK module

7b7ffd3

Use new functions from the RSA module to parse and write private and public keys in PKCS#1 format. Signed-off-by: Valerio Setti <[email protected]>

valeriosetti added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review size-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels Jan 23, 2024

valeriosetti requested a review from mpg January 23, 2024 17:14

valeriosetti self-assigned this Jan 23, 2024

valeriosetti force-pushed the issue8647 branch from 1c49e30 to 9895b38 Compare January 24, 2024 08:47

mpg requested changes Jan 24, 2024

View reviewed changes

valeriosetti added 5 commits January 24, 2024 12:36

config_adjust_legacy: do not auto-enable PK when RSA is enabled in PSA

2ddabb3

Signed-off-by: Valerio Setti <[email protected]>

all.sh: update common_test_psa_crypto_config_accel_ecc_some_curves()

c6d7f53

Do not disable RSA_C and related modules because now it does not automatically re-enable PK module. Signed-off-by: Valerio Setti <[email protected]>

pk_wrap: use RSA module functions to write priv/pub key in RSA wrappers

18dd000

Signed-off-by: Valerio Setti <[email protected]>

rsa: write documentation of new functions for parse/writing RSA priv/…

a5f36fc

…pub keys Signed-off-by: Valerio Setti <[email protected]>

valeriosetti force-pushed the issue8647 branch from 9895b38 to 1dc1501 Compare January 24, 2024 12:56

valeriosetti requested a review from mpg January 24, 2024 12:56

valeriosetti added 4 commits January 24, 2024 16:42

test_suite_rsa: improve rsa_parse_write_pkcs1_key() adding more checks

1533c3f

Signed-off-by: Valerio Setti <[email protected]>

test_suite_[pkparse/rsa]: move RSA private key parsing tests

6def24c

Signed-off-by: Valerio Setti <[email protected]>

test_suite_rsa: extend rsa_parse_pkcs1_key adding tests for public key

6d597f1

Signed-off-by: Valerio Setti <[email protected]>

changelog: document RSA parse/write improvements

ea98647

Signed-off-by: Valerio Setti <[email protected]>

valeriosetti force-pushed the issue8647 branch from 1dc1501 to ea98647 Compare January 24, 2024 15:42

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Feb 6, 2024

gilles-peskine-arm requested changes Feb 6, 2024

View reviewed changes

gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Feb 6, 2024

rsa: remove unnecessary check in priv/pub key parsing

447bbce

Signed-off-by: Valerio Setti <[email protected]>

valeriosetti requested a review from gilles-peskine-arm February 7, 2024 07:02

gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Feb 7, 2024

gilles-peskine-arm mentioned this pull request Feb 7, 2024

mbedtls_rsa_parse_key and mbedtls_rsa_parse_pubkey accept trailing garbage #8799

Closed

gilles-peskine-arm approved these changes Feb 7, 2024

View reviewed changes

mpg approved these changes Feb 8, 2024

View reviewed changes

mpg 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 Feb 8, 2024

mpg added this pull request to the merge queue Feb 8, 2024

Merged via the queue into Mbed-TLS:development with commit 7bf1e98 Feb 8, 2024
6 checks passed

valeriosetti mentioned this pull request Feb 12, 2024

mbedtls_rsa_parse_key and mbedtls_rsa_parse_pubkey accept trailing garbage #8804

Merged

3 tasks

mpg mentioned this pull request Feb 14, 2024

Wrong dependency on PK_WRITE for RSA_C when PSA_CRYPTO_C is enabled #7126

Closed

valeriosetti deleted the issue8647 branch December 6, 2024 06:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move RSA basic key parsing/writing to rsa.c #8740

Move RSA basic key parsing/writing to rsa.c #8740

valeriosetti commented Jan 23, 2024 •

edited by mpg

Loading

mpg left a comment

gilles-peskine-arm commented Feb 6, 2024

gilles-peskine-arm left a comment

gilles-peskine-arm Feb 6, 2024

valeriosetti Feb 7, 2024

gilles-peskine-arm Feb 7, 2024

gilles-peskine-arm Feb 6, 2024

valeriosetti Feb 7, 2024

valeriosetti Feb 7, 2024 •

edited

Loading

gilles-peskine-arm Feb 7, 2024

valeriosetti Feb 7, 2024

gilles-peskine-arm Feb 7, 2024

gilles-peskine-arm Feb 7, 2024

gilles-peskine-arm left a comment

mpg left a comment

		@@ -109,6 +109,10 @@ int mbedtls_rsa_parse_key(mbedtls_rsa_context rsa, const unsigned char key, si

		end = p + len;

		if (end > (key + keylen)) {

Move RSA basic key parsing/writing to rsa.c #8740

Move RSA basic key parsing/writing to rsa.c #8740

Conversation

valeriosetti commented Jan 23, 2024 • edited by mpg Loading

Description

PR checklist

mpg left a comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Feb 6, 2024

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

valeriosetti Feb 7, 2024 • 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

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

mpg left a comment

Choose a reason for hiding this comment

valeriosetti commented Jan 23, 2024 •

edited by mpg

Loading

valeriosetti Feb 7, 2024 •

edited

Loading