Add Pk::private_from_rsa_components and Pk::public_from_rsa_components #347
Conversation
mzohreva
force-pushed
the
mz/rsa-key-from-components
branch
from
521bdb6 to
31c795e
Compare
|
Because of default branch name change, the CI is not triggerred now. |
|
Hi @mzohreva , you might need to rebase this PR to trigger the CI . |
mzohreva
force-pushed
the
mz/rsa-key-from-components
branch
from
31c795e to
34f15f6
Compare
| /// Private exponent | ||
| d: &'a Mpi, | ||
| /// Public exponent | ||
| e: &'a Mpi, |
There was a problem hiding this comment.
I understand that these values are used by upstream in rsa_import, but e is redundant here.
EDIT: Not redundant, as λ(n) is not easy to compute.
What is the behavior if we pass inconsistent d, e here (with respect to n)? Perhaps we should add a test, since I don't see an explicit d, n check in rsa_check_context or in rsa_complete which we are rightfully calling on import. It looks like it assumes that d, n are consistent.
There was a problem hiding this comment.
I checked, and rsa_check_context does not guarantee consistency of parameters, and rsa_complete does not check this specifically.
Moreover I think it is an important check, for some implementations use ed = 1 mod λ(n) and others ed = 1 mod φ(n) (see e.g. https://gitlab.com/sequoia-pgp/sequoia/-/issues/792).
There was a problem hiding this comment.
After further research, MbedTLS returns RSA_BAD_INPUT_DATA when trying to deduce_primes, see here.
This check happens a bit too late and in principle inside a loop that aborts early or late depending on the input. Arguably this is upstream but we might want to check for consistency in this struct at least. I am now wondering if this loop be used by an attacker controlling e, n somehow to learn information about d, but this is upstream concern.
There was a problem hiding this comment.
Can we use mbedtls_rsa_check_privkey after rsa_complete to catch any issue with the parameters?
There was a problem hiding this comment.
Yes, that specifically checks for de == 1 mod (p-1) and de == 1 mod (q-1)
| e: &Mpi::new(pk.rsa_public_exponent().unwrap() as _).unwrap(), | ||
| }; | ||
| let mut pk3 = Pk::private_from_rsa_components(components).unwrap(); | ||
| assert_eq!(pk.write_private_der_vec().unwrap(), pk3.write_private_der_vec().unwrap()); |
There was a problem hiding this comment.
This test fails when I don't use the test rng:
use crate::rng::{CtrDrbg, OsEntropy};
let entropy = OsEntropy::new();
let mut rng = CtrDrbg::new(entropy.into(), None).unwrap();
let mut pk = Pk::generate_rsa(&mut rng, 2048, 0x10001).unwrap();
It fails in the last assertion. I think it swaps parameter order on DER serialization?
There was a problem hiding this comment.
Perhaps we should refactor the test RNG so that it randomizes tests, or have another test RNG. (cc. @jethrogb)
No description provided.