reject undersized modulus for dh public keys in spki loader - #15121
Conversation
|
If we're going to do this, the place to do this enforcement shoudl be in cryptography-rust, not on the parser side. |
5d152b1 to
59e7726
Compare
|
makes sense, moved it out of the parser. the check now lives in public_key_from_pkey in cryptography-rust (backend/dh.rs), right after check_dh_parameters, so it sits alongside the other DH key construction rather than in the SPKI loader. reverted the spki.rs changes and added a python regression test that loads a 248-bit DH SPKI and expects a ValueError. |
|
Looks like this test isn't actually hitting the expected code path? |
|
you're right, it wasn't. two things were going on: the der i'd hand-built wasn't a valid group, and on openssl check_dh_parameters (DH_check) already rejects a sub-512 modulus itself with DH_MODULUS_TOO_SMALL, so the ValueError was coming from there rather than the new code. i've switched the test to use the public half of the existing 256-bit dh_key_256.pem vector, which is a real group that clears DH_check, and moved the explicit num_bits check ahead of check_dh_parameters so it's the thing that actually rejects the key. that matches the private-key loader, which does its own num_bits check rather than leaning on openssl's, and keeps the behaviour consistent on backends whose DH_check doesn't enforce the minimum. the load now raises from the new check and the test genuinely hits it. |
| pyo3::exceptions::PyValueError::new_err("Invalid key"), | ||
| )); | ||
| } |
There was a problem hiding this comment.
Is there a reason not to put this check in check_dh_parameters?
There was a problem hiding this comment.
no reason not to, that's tidier. moved the num_bits check into check_dh_parameters and reverted the special-case in public_key_from_pkey, so all the DH construction paths share the same gate now. it still runs ahead of dh.check_key() so it's the thing that rejects the undersized modulus, and the test hits it.
7c1cc3a to
f99accc
Compare
|
rebased on main to clear the conflict too (it was just the changelog). |
|
Thank you @alex for your time. |
2 participants
Loading a public key currently accepts Diffie-Hellman keys whose modulus is well below 512 bits, even though the private-key DER loader and the DHParameterNumbers constructor already reject anything under that minimum, so the public-key path is the one place a badly undersized group slips through. The check belongs in parse_public_key in the SPKI loader, where both DH OIDs (the X9.42 dhpublicnumber form and the PKCS#3 dhKeyAgreement form) get turned into keys. I've added the same num_bits comparison there for both arms so the behaviour lines up with the other two entry points.