rsa_get_bitlen does not return correct number of bits

[jethrogb]

mbedtls_pk_get_bitlen rounds up the size of RSA keys to the nearest byte.

The reason is that rsa_get_bitlen returns 8× the number of bytes required to store the modulus. This could be up to 7 bits higher than the actual number of bits.

[ciarmcom]

ARM Internal Ref: IOTSSL-1672

[tom-daubney-arm]

Whilst this is true the function in question is not part of the public API and is only used internally. This is not something we are looking to change in the near future and so will close this issue.

[jethrogb]

What do you mean this isn't part of the public API? What other function is there to get key metadata?

[tom-cosgrove-arm]

The public API is broadly what is in include/mbedtls, and documented, notably excluding structure fields marked MBEDTLS_PRIVATE.

rsa_get_bitlen() is a static function in library/pk_wrap.c, that can be accessed via mbedtls_rsa_info, which is an instance of struct mbedtls_pk_info_t, which is forward-declared in include/mbedtls/pk.h, but only fully declared in library/pk_wrap.h, which is not part of the public API.

[jethrogb]

rsa_get_bitlen is the value of the get_bitlen field of struct mbedtls_pk_info_t for RSA. This is called by mbedtls_pk_get_bitlen which I'm pretty sure is public API.

[gilles-peskine-arm]

@tom-daubney-arm As @jethrogb notes, this does have an impact on the public API. This limitation of RSA (no function to get the correct bit length) also restricts the PSA code: it refuses to create RSA keys whose bit-length is not a multiple of 8.

I don't know how to interpret the fact that we've never lifted this limitation: either it shows that it's still relevant (since it applies to PSA and not just to classic APIs), or it shows that it's not important (since we've never bothered to fix it).

One thing that made me not push to fix it is that we have a history of bugs around non-byte-aligned keys, with incorrect calculations of padding sizes which led at least once to a buffer overflow.

On the other hand, functionally, it's a very easy fix. We'd just need to add mbedtls_rsa_get_bitlen to match the pk function, and it's trivial to implement. Ideally we should have more testing of non-byte-aligned key sizes, but that's true whether the bit-length is correctly reported or not — if we want to remove the risk of bugs with non-byte-aligned key sizes, we need to prevent their creation, not report them incorrectly.

[gilles-peskine-arm]

So I'm reopening this issue, because it's a genuine bug. Maybe we don't intend to fix it (though it's not clear to me at this point whether anyone has made this decision with an understanding of the problem), but until we retire the corresponding API (which could mean until RSA is sufficiently restricted that you can't create non-byte-aligned keys anymore), it's a genuine, known bug so it should be documented by an open issue.

[jethrogb]

which could mean until RSA is sufficiently restricted that you can't create non-byte-aligned keys anymore

This would break interoperability so I don't think that's a good direction for solutions.

[gilles-peskine-arm]

@jethrogb Our PSA interface blocks non-byte-aligned RSA modulus sizes and we haven't gotten any complaints so far. Have you encountered such keys in practice? In the HSM/SE world, a lot of implementations have even more stringent restrictions, like having to be of the form 2^n or 3*2^n.

[jethrogb]

Generating 2046-bit keys and deserializing 2047-bit keys from PEM/DER both work fine in 2.28. Our HSM supports this. If 3.x no longer supports this we'll need to look at another crypto library as this would break backcompat.

[gilles-peskine-arm]

@jethrogb Thanks for the information! So now we have one confirmed user for which this is important. If you have time, feel free to rejuvenate 1d26709 and expand the testing.

[tom-daubney-arm]

I will take this on and provide a fix

[jwinzig-at-hilscher]

@tom-daubney-arm Will this bug be fixed in the near future?

[daverodgman]

@minosgalanakis does this fall into the MBEDTLS_PRIVATE work?

[gilles-peskine-arm]

@minosgalanakis I'm adding this to the MBEDTLS_PRIVATE epic. This is debatable: it was already a bug before private fields, but the fact that mbedtls_pk_rsa(pk)->N is now a private field makes it more annoying to work around it.