Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enforce a sane key size when generating an RSA key #7556

Closed
gilles-peskine-arm opened this issue May 5, 2023 · 3 comments · Fixed by #7864
Closed

Enforce a sane key size when generating an RSA key #7556

gilles-peskine-arm opened this issue May 5, 2023 · 3 comments · Fixed by #7864
Assignees
Labels
component-crypto Crypto primitives and low-level interfaces enhancement size-xs Estimated task size: extra small (a few hours at most)

Comments

@gilles-peskine-arm
Copy link
Contributor

gilles-peskine-arm commented May 5, 2023

When generating an RSA key with mbedtls_rsa_gen_key, the minimum key size is 128 bits. This is clearly not enforced for security, just to avoid edge cases with small numbers during the generation process.

The PSA API does not enforce a minimum. There's no pk API for key generation.

This creates a risk of misuse: if you accidentally specify a key size in bytes (because that's the intended size of the signature or ciphertext) rather than bits, then a reasonable wish for a 2048-bit = 256-byte key becomes a request for an insanely small 256-bit key — but still large enough for a PKCS#1v1.5 encryption of a 16-byte payload, so you might not notice in functional testing.

We should enforce a minimum key size in mbedtls_rsa_gen_key that's not outright insecure. What should the threshold be?

  • 1024 bits is what I'd consider the absolute minimum below which we don't need to provide functionality. It's not publicly broken, but close. 1024 bytes is 8192 bits, and if you want an RSA key that large, you're on the cautious side so unlikely to confuse bits and bytes, and even if you do you get something that's not outright broken.
  • 2048 bits is the minimum that I'd accept in a security design review. But maybe we still need to support legacy systems that use smaller keys?

Should we make any change to 2.28 LTS? It's a security improvement, but not a critical one (since the library itself isn't broken: this is only about a risk of accidental misuse).

Note that I do not propose to introduce any minimum when using a key. You will still be able to verify or even re-create RSA-321 signatures (but not generate new keys).


Task description

  • Enforce the chosen minimum size in mbedtls_rsa_gen_key and psa_generate_key (before driver dispatch).
  • Change current test cases that generate a smaller RSA key (I know we have a couple that generate 512-bit keys) to use the new minimum size.
  • Add test-cases to check that keys smaller than the new minimum are rejected, both via mbedtls_rsa_gen_key and via psa_generate_key. Test at least min - 2 (min - 1 would not be conclusive because we require the size to be a multiple of 2) and min - 128 (in case the code changes to enforce more divisibility for the size).
  • Write a changelog entry.
@gilles-peskine-arm gilles-peskine-arm added enhancement component-crypto Crypto primitives and low-level interfaces size-xs Estimated task size: extra small (a few hours at most) labels May 5, 2023
@gilles-peskine-arm
Copy link
Contributor Author

There's one other key types which could be vulnerable to a bit/bytes confusion: classic finite-field Diffie-Hellman. But there the risk is lower because you have to generate or select parameters, so it's not just about the key size. mbedtls_dhm_make_public allows specifying a smaller-than-nominal private key size, but its argument is expressed in bytes, not bits, so the risk of mistake there is in the other direction.

@VitSolcikNXP
Copy link

I assume there will be some macro controlling this size? Then we perhaps may set it to 2048 by default in config as it's the minimum I would choose as well. And legacy users/maintenance can change that. This prevent both cases, using small key size by mistake in code and/or in design.

@gilles-peskine-arm
Copy link
Contributor Author

A configuration option means more work for testing, so we weren't planning to make it an option. But we can do that if different users have conflicting requirements.

By the way, currently, RSA key generation doesn't have its own option for a maximum size. But (except via PSA drivers) it's subject to the generic MBEDTLS_MPI_MAX_SIZE which is 8192 bits by default. PSA has its own upper limit (also applying to drivers) which is set to 4096 bits, currently not configurable. If that's a concern, please file a separate issue on GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-crypto Crypto primitives and low-level interfaces enhancement size-xs Estimated task size: extra small (a few hours at most)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants