Fix pk_parse_key()'s use of rsa_complete() #367

mpg · 2020-02-14T10:41:30Z

This PR fixes bugs and inconsistencies in rsa_complete() and pk_parse_key():

pk_parse_key() used to accept keys that were later rejected by rsa_export(), as found by oss-fuzz
it also used to silently fix the values of some keys rather than report the key as incorrect
it also failed to enforce the same size minimum as pk_parse_pubkey() which was inconsistent.

The PR also enhances testing of pk_parse_key_pkcs1_der() by exercising all of its possible failure modes (new and old).

This PR is meant to replace #363 by using a simpler and more consistent fix, and avoiding API extensions in order to ease backporting to the LTS branches.

Fixes #189
Closes #363

Backports

Changelog

   * Fix a bug in mbedtls_pk_parse_key() that would cause it to accept some
     RSA keys that would later be rejected by functions expecting private
     keys. Found by Catena cyber using oss-fuzz (issue 20467).
   * Fix a bug in mbedtls_pk_parse_key() that would cause it to accept some
     RSA keys with invalid values by silently fixing those values.

mpg · 2020-02-17T08:15:20Z

The CI only failed the tests that rely on Mbed OS, which are currently expected to fail until Mbed OS catches up with the latest changes in the PSA Crypto API.

yanesca

LGTM.

gilles-peskine-arm

I disagree with adding more cases where we depend on whether the data pointer in a bignum is null. In the long term, I want to be able to change the bignum representation to a more opaque one which may not have any equivalent to “null”. I'm ok with the approach taken in #363 (but haven't reviewed it fully).

gilles-peskine-arm · 2020-02-17T10:15:57Z

tests/suites/test_suite_pkparse.data

+depends_on:MBEDTLS_RSA_C
+pk_parse_key:"3063020100021100cc8ab070369ede72920e5a51523c8571020301000102110000000000000000000000000000000000020900000000000000000002090000000000000000000209009471f14c26428401020813425f060c4b72210208052b93d01747a87c":MBEDTLS_ERR_PK_KEY_INVALID_FORMAT
+
+Key ASN1 (RSAPrivateKey, correct values, length mismatch)


Suggested change

Key ASN1 (RSAPrivateKey, correct values, length mismatch)

Key ASN1 (RSAPrivateKey, correct values, trailing garbage)

gilles-peskine-arm · 2020-02-17T10:37:17Z

library/rsa.c

-        mbedtls_mpi_cmp_int( &ctx->Q, 0 ) != 0 &&
-        mbedtls_mpi_cmp_int( &ctx->D, 0 ) != 0 &&
-        mbedtls_mpi_cmp_int( &ctx->E, 0 ) != 0;
+        ctx->N.p != NULL &&


I disagree with exposing this implementation detail of mbedtls_mpi. If N->n == 0 then it shouldn't make a difference whether N->p is null. For example, I recently changed mbedtls_mpi_copy not to make a difference, partly because it misled Coverity into thinking that more null checks are necessary and partly to make the code more robust against possible changes in the details of the behavior of the bignum module.

mbedtls_rsa_complete has treated zero (which is never a valid value for any of the fields) as meaning “to be completed” ever since it was introduced. Please keep it that way.

If N->n == 0, then mbedtls_mpi_cmp_int() still finds it equal to 0. The only reliable way to differentiate 0 from unset is checking N->p at the moment.

I agree that this is not ideal and that we should not rely on implementation details, but I don't think that this PR is the best place to fix this.

My disagreement is about treating any bignum value as “unset”.

IMO that behaviour rsa_complete() was a design mistake, which had unintended consequences, and since it was never documented I don't think we should feel forced to keep it.

But I take note of your opposition and will look for options we can both agree on.

gilles-peskine-arm · 2020-02-17T10:38:23Z

library/rsa.c

-        mbedtls_mpi_cmp_int( &ctx->E, 0 ) != 0;
+        ctx->N.p != NULL &&
+        ctx->P.p != NULL &&
+        ctx->Q.p != NULL &&


This doesn't fix the bug of treating a key with e.g. d ≠ 0 but p=q=0 as public.

Because there is no such bug. There are only two legal ways of importing a key:

by using pkparse

by using a sequence of calls to rsa_import() followed by a call to rsa_complete().
Once you imported a key in one of these ways, the condition you mention can never happen.

s/bug/lack of robustness/ then

I'd still consider that quite orthogonal to the primary issue the PR is addressing, and I'd rather address it separately.

I'm fine with leaving it for later. But I found it surprising that you wouldn't fix this since you were changing this code anyway.

When parsing a PKCS#1 RSAPrivateKey structure, all parameters are always present. After importing them, we need to call rsa_complete() for the sake of alternative implementations. That function interprets zero as a signal for "this parameter was not provided". As that's never the case, we mustn't pass any zero value to that function, so we need to explicitly check for it.

- remove incorrect compile-time dependency (the individual cases already have correct run-time dependency information) - remove unused argument - remove unused stack buffer - remove useless code block

(Only the top-level ones, ie, for each call to eg asn1_get_mpi(), ensure there's at least one test case that makes this call fail in one way, but don't test the various ways to make asn1_get_mpi fail - that should be covered elsewhere.) - the new checks added by the previous commits needed exercising - existing tests sometimes had wrong descriptions or where passing for the wrong reason (eg with the "length mismatch" test, the function actually failed before reaching the length check) - while at it, add tests for the rest as well The valid minimal-size key was generated with: openssl genrsa 128 2>/dev/null | openssl rsa -outform der 2>/dev/null | xxd -p

Some code paths want to access members of the mbedtls_rsa_context structure. We can only do that when using our own implementation, as otherwise we don't know anything about that structure.

mpg · 2020-02-18T09:40:43Z

@gilles-peskine-arm @yanesca I just force-pushed a new version using the strategy we discussed yesterday.

While at it, I noticed a bug in a previous fix (reading extra CRT values) and even though it's not directly related to the core goal of this PR, I fixed it as well since it's a one-line fix.

I'll start working on backports as soon as this has been reviewed and approved by at least one of you.

gilles-peskine-arm · 2020-02-18T09:59:55Z

library/pkparse.c

-    if( ( ret = mbedtls_asn1_get_mpi( &p, end, &T ) ) != 0 ||
-        ( ret = mbedtls_asn1_get_mpi( &p, end, &T ) ) != 0 ||
-        ( ret = mbedtls_asn1_get_mpi( &p, end, &T ) ) != 0 )
+    if( ( ret = asn1_get_nonzero_mpi( &p, end, &T ) ) != 0 ||


Since we're only verifying “existance” (sic) here, why test for zero?

For consistency with the CRT case. I'd like to avoid changing the set of keys accepted by this function too much depending on whether MBEDTLS_RSA_NO_CRT is enabled or not, at least when we can conveniently avoid differences.

yanesca

LGTM

yanesca · 2020-02-18T13:16:22Z

The CI still fails due to a known issue that is unrelated to the changes in this PR.

* ARMmbed#365 Change PSA compatibility API to inline functions * ARMmbed#367 Fix pk_parse_key()'s use of rsa_complete() * ARMmbed#370 Bump version to Mbed TLS 2.21.0

mpg added bug Something isn't working needs: review The pull request is ready for review. This generally means that it has no known issues. needs: ci Needs a passing full CI run needs: backports Needs backports to Mbed TLS branches labels Feb 14, 2020

mpg mentioned this pull request Feb 14, 2020

Fix pk_parse_key() validation of RSA private keys #363

Closed

6 tasks

mpg removed the needs: ci Needs a passing full CI run label Feb 17, 2020

yanesca previously approved these changes Feb 17, 2020

View reviewed changes

gilles-peskine-arm requested changes Feb 17, 2020

View reviewed changes

mpg added 3 commits February 18, 2020 10:18

Check public part when parsing private RSA key

c422679

Clean up test function pk_parse_key

b65370f

- remove incorrect compile-time dependency (the individual cases already have correct run-time dependency information) - remove unused argument - remove unused stack buffer - remove useless code block

mpg dismissed yanesca’s stale review via 5343949 February 18, 2020 09:25

mpg added 2 commits February 18, 2020 10:31

Fix pkparse bug wrt MBEDTLS_RSA_ALT

bbb5a0a

Some code paths want to access members of the mbedtls_rsa_context structure. We can only do that when using our own implementation, as otherwise we don't know anything about that structure.

gilles-peskine-arm approved these changes Feb 18, 2020

View reviewed changes

mpg changed the title ~~Fix rsa_complete() and pk_parse_key()~~ Fix pk_parse_key()'s use of rsa_complete() Feb 18, 2020

This was referenced Feb 18, 2020

[backport 2.16] Fix pk_parse_key()'s use of rsa_complete() Mbed-TLS/mbedtls#3048

Merged

[backport 2.7] Fix pk_parse_key()'s use of rsa_complete() Mbed-TLS/mbedtls#3049

Merged

yanesca approved these changes Feb 18, 2020

View reviewed changes

yanesca removed the needs: review The pull request is ready for review. This generally means that it has no known issues. label Feb 18, 2020

gilles-peskine-arm added ready for merge Design and code approved, CI passed, and likewise for backports. Label added by gatekeepers only. and removed needs: backports Needs backports to Mbed TLS branches labels Feb 19, 2020

gilles-peskine-arm merged commit f841eab into ARMmbed:development Feb 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix pk_parse_key()'s use of rsa_complete() #367

Fix pk_parse_key()'s use of rsa_complete() #367

mpg commented Feb 14, 2020 •

edited

Loading

mpg commented Feb 17, 2020

yanesca left a comment

gilles-peskine-arm left a comment

gilles-peskine-arm Feb 17, 2020

gilles-peskine-arm Feb 17, 2020

yanesca Feb 17, 2020

gilles-peskine-arm Feb 17, 2020

mpg Feb 17, 2020

gilles-peskine-arm Feb 17, 2020

mpg Feb 17, 2020

gilles-peskine-arm Feb 17, 2020

mpg Feb 17, 2020

gilles-peskine-arm Feb 17, 2020

mpg commented Feb 18, 2020

gilles-peskine-arm Feb 18, 2020

mpg Feb 18, 2020

yanesca left a comment

yanesca commented Feb 18, 2020

	Key ASN1 (RSAPrivateKey, correct values, length mismatch)
	Key ASN1 (RSAPrivateKey, correct values, trailing garbage)

Fix pk_parse_key()'s use of rsa_complete() #367

Fix pk_parse_key()'s use of rsa_complete() #367

Conversation

mpg commented Feb 14, 2020 • edited Loading

Backports

Changelog

mpg commented Feb 17, 2020

yanesca left a comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpg commented Feb 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanesca left a comment

Choose a reason for hiding this comment

yanesca commented Feb 18, 2020

mpg commented Feb 14, 2020 •

edited

Loading