Fix pk_parse_key() validation of RSA private keys #363
Conversation
This will be used in pkparse to make sure that when parsing a private key, what we're getting will actually looks like a private key to the rest of the code. Currently pk_parse_key() accepts keys that rsa_export() does not recognize as private.
mpg
added
bug
- 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 commit 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
| int mbedtls_rsa_is_private( const mbedtls_rsa_context *ctx ) | ||
| { | ||
| return( | ||
| mbedtls_mpi_cmp_int( &ctx->N, 0 ) != 0 && |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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 ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@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: #367 |
mpg
added
the
DO NOT MERGE
We recently added a check of the return value of
mbedtls_rsa_exportaftermbedtls_pk_parse_keyinfuzz_privkey(Mbed-TLS/mbedtls#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 asrsa_export()does).Status: WIP
Todo
pk_parse_key()d == 0and silently fix the value tod = p * q, I don't think we shouldBackports