Fix pk_parse_key() validation of RSA private keys #363

2020-02-07T08:46:29-06:00

mpg commented

2020-02-07 08:46:29 -06:00

(Migrated from github.com)

We recently added a check of the return value of mbedtls_rsa_export after mbedtls_pk_parse_key in fuzz_privkey (https://github.com/ARMmbed/mbedtls/pull/2999) and oss-fuzz found an issue (restricted link: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=20467).

This PR fixes the issue by making sure that pk_parse_key() rejects keys that are obviously not valid private keys, as its documentation promises (and as rsa_export() does).

Status: WIP

Todo

Add a non-regression test for pk_parse_key()
We accept keys with d == 0 and silently fix the value to d = p * q, I don't think we should
Fix issue with error code wrapping
Write a ChangeLog entry here.

Backports

mbedtls-2.16
mbedtls-2.7

We recently added a check of the return value of `mbedtls_rsa_export` after `mbedtls_pk_parse_key` in `fuzz_privkey` (https://github.com/ARMmbed/mbedtls/pull/2999) and oss-fuzz found an issue (restricted link: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=20467). This PR fixes the issue by making sure that `pk_parse_key()` rejects keys that are obviously not valid private keys, as its documentation promises (and as `rsa_export()` does). Status: WIP ## Todo - [x] Add a non-regression test for `pk_parse_key()` - [ ] We accept keys with `d == 0` and silently fix the value to `d = p * q`, I don't think we should - [ ] Fix issue with error code wrapping - [ ] Write a ChangeLog entry here. ### Backports - [ ] mbedtls-2.16 - [ ] mbedtls-2.7

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

2020-02-10 14:10:57 -06:00

library/rsa.c

					
				@ -401,0 +401,4 @@

				int mbedtls_rsa_is_private( const mbedtls_rsa_context *ctx )

				{

				    return(

				        mbedtls_mpi_cmp_int( &ctx->N, 0 ) != 0 &&

This function returns 0 on an incomplete key. I find this behavior surprising. I'd prefer mbedtls_rsa_is_private(rsa) to return the same thing before and after mbedtls_rsa_complete(rsa). If it doesn't, this should be documented clearly.

This function returns 0 on an incomplete key. I find this behavior surprising. I'd prefer `mbedtls_rsa_is_private(rsa)` to return the same thing before and after `mbedtls_rsa_complete(rsa)`. If it doesn't, this should be documented clearly.

jack-fortanix (Migrated from github.com) reviewed

2020-02-11 17:02:44 -06:00

library/rsa.c

					
				@ -401,0 +405,4 @@

				        mbedtls_mpi_cmp_int( &ctx->P, 0 ) != 0 &&

				        mbedtls_mpi_cmp_int( &ctx->Q, 0 ) != 0 &&

				        mbedtls_mpi_cmp_int( &ctx->D, 0 ) != 0 &&

				        mbedtls_mpi_cmp_int( &ctx->E, 0 ) != 0 );

Two comments - first this seems to accept values which are negative, values which could be deserialized from the DER structure. Also it seems to ignore the CRT params, which should probably also be checked, if CRT is enabled at least.

mpg (Migrated from github.com) reviewed

2020-02-12 02:25:57 -06:00

library/rsa.c

					
				@ -401,0 +405,4 @@

				        mbedtls_mpi_cmp_int( &ctx->P, 0 ) != 0 &&

				        mbedtls_mpi_cmp_int( &ctx->Q, 0 ) != 0 &&

				        mbedtls_mpi_cmp_int( &ctx->D, 0 ) != 0 &&

				        mbedtls_mpi_cmp_int( &ctx->E, 0 ) != 0 );

Thanks for your feedback! Actually I think the checks in rsa_check_context() (called from mbedtls_rsa_complete() called from mbedtls_pk_parse_key()) already prevent that, but I agree that it should be more obvious. I was intending to rework the PR anyway, as I'm no longer convinced mbedtls_rsa_is_private() is the right function here, so I'll keep that in mind when reworking.

Thanks for your feedback! Actually I think the checks in `rsa_check_context()` (called from `mbedtls_rsa_complete()` called from `mbedtls_pk_parse_key()`) already prevent that, but I agree that it should be more obvious. I was intending to rework the PR anyway, as I'm no longer convinced `mbedtls_rsa_is_private()` is the right function here, so I'll keep that in mind when reworking.

mpg (Migrated from github.com) reviewed

2020-02-12 02:27:20 -06:00

library/rsa.c

					
				@ -401,0 +401,4 @@

				int mbedtls_rsa_is_private( const mbedtls_rsa_context *ctx )

				{

				    return(

				        mbedtls_mpi_cmp_int( &ctx->N, 0 ) != 0 &&

Good point, I agree this should be documented. I'm likely to rework the PR in a way that no longer relies on this function, but I'll make sure to properly document that aspect of the new function.

mpg commented

2020-02-14 04:45:23 -06:00

(Migrated from github.com)

@gilles-peskine-arm @jack-fortanix I decided to switch to a new approach and I though it would be cleaner to open a new PR for that, so this PR is now superseded by: https://github.com/ARMmbed/mbed-crypto/pull/367