mbedtls_rsa_parse_key and mbedtls_rsa_parse_pubkey accept trailing garbage

[valeriosetti]

Description

Modify rsa.c private/public key parsing in order to reject SEQUENCEs with extra data at the end of the sequence (a.k.a. trailing garbage).

Resolves #8799

PR checklist

• [ ] changelog Since I'm changing the behavior of a public function I suspect it's required, isn't it?
• [ ] backport not required
• [ ] tests provided

[gilles-peskine-arm]

Note that the CI is complaining about a couple of minor things.

[mpg]

This is a bug fix (according to the ChangeLog entry), so it should be backported unless there's a reason not to (and I'm not seeing any).

To be more precise, I think there are two parts to this PR: (1) PEM is now checking padding and (2) RSA key parsing functions now reject trailing garbage, and I'm only talking about backporting (1) which is an actual bug fix.

OTOH, (2) is more like a hardening, and arguably changing the unwritten contract of existing functions, so I wouldn't really blame people for relying on the "accept trailing garbage" behaviour (aka "I don't need to pass the exact length of the key, only an upper bound, and the function will figure out the actual length for me").

Wdyt?

[gilles-peskine-arm]

To be more precise, I think there are two parts to this PR: (1) PEM is now checking padding and (2) RSA key parsing functions now reject trailing garbage, and I'm only talking about backporting (1) which is an actual bug fix.

Both bug fixes are validating data more precisely, where the previous behavior was sloppy but in a low-risk way. So there's some risk of breaking code that relied on the previous sloppy behavior, and not much gain in fixing the bug. What made us tighten the behavior in the development branch was primarily maintainability: having functions with a clear semantics, which we could test without carving weird exceptions. To me this adds up to fixing the bug in development, but leaving LTS branches alone.

When we fixed the similar issue with PKCS5 and PKCS12 encrypted keys, we did not backport that part (“mbedtls_pk_parse_key() now rejects trailing garbage in encrypted keys.” released in 3.5.0). (Discussion: https://github.com/Mbed-TLS/mbedtls-restricted/issues/1027 — private link, because this was tied to bug fixes with a potential security implications.)

So I don't think we should backport anything here.

[mpg]

So I don't think we should backport anything here.

Fine with me, I agree with your points. (And thanks for the reference to the similar PKCS5/12 issues, I had been thinking about that too but then forgot about it.)