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

2020-02-14T04:41:30-06:00

mpg commented

2020-02-14 04:41:30 -06:00

(Migrated from github.com)

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

mbedtls-2.16 https://github.com/ARMmbed/mbedtls/pull/3048
mbedtls-2.7 https://github.com/ARMmbed/mbedtls/pull/3049

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.

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 - [x] mbedtls-2.16 https://github.com/ARMmbed/mbedtls/pull/3048 - [x] mbedtls-2.7 https://github.com/ARMmbed/mbedtls/pull/3049 ## 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 commented

2020-02-17 02:15:20 -06:00

(Migrated from github.com)

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 (Migrated from github.com) reviewed

2020-02-17 04:12:59 -06:00

yanesca (Migrated from github.com) left a comment

No description provided.

LGTM.

gilles-peskine-arm (Migrated from github.com) requested changes

2020-02-17 04:39:57 -06:00

gilles-peskine-arm (Migrated from github.com) left a comment

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).

library/rsa.c

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.

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`](https://github.com/ARMmbed/mbed-crypto/pull/346) 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.

library/rsa.c

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

tests/suites/test_suite_pkparse.data

					
				@ -1072,33 +1072,84 @@ Parse EC Key #15 (SEC1 DER, secp256k1, SpecifiedECDomain)

				depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256K1_ENABLED:MBEDTLS_PK_PARSE_EC_EXTENDED

Key ASN1 (RSAPrivateKey, correct values, trailing garbage)

```suggestion Key ASN1 (RSAPrivateKey, correct values, trailing garbage) ```

yanesca (Migrated from github.com) reviewed

2020-02-17 04:55:46 -06:00

library/rsa.c

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.

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.

mpg (Migrated from github.com) reviewed

2020-02-17 04:56:21 -06:00

library/rsa.c

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.

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.

gilles-peskine-arm (Migrated from github.com) reviewed

2020-02-17 04:58:20 -06:00

library/rsa.c

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

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

mpg (Migrated from github.com) reviewed

2020-02-17 04:58:55 -06:00

library/rsa.c

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.

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 (Migrated from github.com) reviewed

2020-02-17 04:59:25 -06:00

library/rsa.c

s/bug/lack of robustness/ then

mpg (Migrated from github.com) reviewed

2020-02-17 05:07:49 -06:00

library/rsa.c

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

gilles-peskine-arm (Migrated from github.com) reviewed

2020-02-17 05:09:25 -06:00

library/rsa.c

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.

mpg commented

2020-02-18 03:40:43 -06:00

(Migrated from github.com)

@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 @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 (Migrated from github.com) approved these changes

2020-02-18 04:01:44 -06:00

library/pkparse.c

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

mpg (Migrated from github.com) reviewed

2020-02-18 04:10:47 -06:00

library/pkparse.c

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.

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 (Migrated from github.com) approved these changes

2020-02-18 07:14:07 -06:00

yanesca (Migrated from github.com) left a comment

LGTM

yanesca commented

2020-02-18 07:16:22 -06:00

(Migrated from github.com)

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

Sign in to join this conversation.

No reviewers

gilles-peskine-arm

yanesca

No labels

HEADER FORMATTING ISSUE

help wanted

Information

invalid

Jira status: AWAITING REVIEW

Jira status: CLOSED

Jira status: DONE

Jira status: IN PROGRESS

Jira status: IN REVIEW

Jira status: OPEN

Jira status: REOPENED

Jira status: RESOLVED

open for community contribution

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gh-ARMmbed/mbed-crypto!367

				`@ -1072,33 +1072,84 @@ Parse EC Key #15 (SEC1 DER, secp256k1, SpecifiedECDomain)`
				`depends_on:MBEDTLS_PEM_PARSE_C:MBEDTLS_ECP_C:MBEDTLS_ECP_DP_SECP256K1_ENABLED:MBEDTLS_PK_PARSE_EC_EXTENDED`

Rows
Columns