Add more robust key processing: DSA#14992
Conversation
alex
left a comment
alex
left a comment
There was a problem hiding this comment.
All the new vectors need to be documented in test-vectors.rst -- please split those out into their own PR.
For ease of review, I think it'd be best to split the actual changes into one PR per algorithm.
| } | ||
|
|
||
| pub(crate) fn private_key_from_pkey( | ||
| _py: pyo3::Python<'_>, |
There was a problem hiding this comment.
If we don't need this arg, we can just drop it.
| let numbers = key.public_numbers(py)?; | ||
| check_public_key_components(numbers.e.bind(py), numbers.n.bind(py))?; |
There was a problem hiding this comment.
Hmm, ideally we wouldn't have to round trip via the public numbers to create a public key... is there some way to structure this to avoid that?
There was a problem hiding this comment.
@alex thoughts on this alternative? Modifies the existing check to work on bignums as well.
There was a problem hiding this comment.
I'll leave this as the RSA PR btw, and split out the DH and DSA and vectors as requested
sjudson
force-pushed
the
sj/robust-processing
branch
from
85717cf to
2c0c828
Compare
sjudson
force-pushed
the
sj/robust-processing
branch
from
2c0c828 to
f231f16
Compare
sjudson
changed the title
|
If you load a private key (via DER/PEM) with bad public key values this will raise a ValueError when calling |
|
@reaperhulk updated to resolve this, and in a way consistent with your comment on #15015 as well: the validation has been moved down to the binding layer, where it is then applied across the various code paths so that there is a single consistent implementation. However, this has broken one of the certbot-josepy downstream tests, which loads a 512-bit private key (expecting no error at the cryptography layer) and then rejects it. With the stricter validation, that key is now rejected on load. Do you have a sense of how you'd prefer to proceed? Obviously one approach would be to accept being breaking here. I could also modify the validation to accept 512-bit key lengths, despite their insecure/legacy nature. Or even stronger I could drop the bitlength checks from the PEM/DER load paths (but leave them where they already exist) for maximal compatibility. |
3 participants
This PR adds more robust key processing for DSA. In particular:
check_dsa_public_numbers, is added to verify that for DSA the publicyvalue both1 < y < pandy ** q mod p = 1, as keys failing validation may be weak to forgery; and