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

Add Pk::private_from_rsa_components and Pk::public_from_rsa_components #347

Merged
merged 1 commit into from
Feb 9, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 96 additions & 2 deletions mbedtls/src/pk/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,34 @@ const CUSTOM_PK_INFO: pk_info_t = {
}
};

/// RSA components for constructing a private key.
pub enum RsaPrivateComponents<'a> {
WithPrimes {
/// Private 1st prime
p: &'a Mpi,
/// Private 2nd prime
q: &'a Mpi,
/// Public exponent
e: &'a Mpi,
},
WithPrivateExponent {
/// Public modulus
n: &'a Mpi,
/// Private exponent
d: &'a Mpi,
/// Public exponent
e: &'a Mpi,
Copy link
Contributor

@zugzwang zugzwang Feb 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use mbedtls_rsa_check_privkey after rsa_complete to catch any issue with the parameters?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that specifically checks for de == 1 mod (p-1) and de == 1 mod (q-1)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed in #352

},
}

/// RSA components for constructing a public key.
pub struct RsaPublicComponents<'a> {
/// Public modulus
pub n: &'a Mpi,
/// Public exponent
pub e: &'a Mpi,
}

// If this changes then certificate.rs unsafe code in public_key needs to also
// change.
define!(
Expand Down Expand Up @@ -200,7 +228,7 @@ define!(
// - Only const access to context: eckey_check_pair, eckey_get_bitlen,
// eckey_can_do, eckey_check_pair
//
// - Const acccess / copies context to a stack based variable eckey_verify_wrap,
// - Const access / copies context to a stack based variable eckey_verify_wrap,
// eckey_sign_wrap: ../../../mbedtls-sys/vendor/crypto/library/pk_wrap.c:251
// creates a stack ecdsa variable and uses ctx to initialize it. ctx is passed
// as 'key', a const pointer to mbedtls_ecdsa_from_keypair( &ecdsa, ctx )
Expand Down Expand Up @@ -348,7 +376,7 @@ Please use `private_from_ec_components_with_rng` instead."
///
/// This function will return an error if:
///
/// * Fails to genearte `EcPoint` from given EcGroup in `curve`.
/// * Fails to generate `EcPoint` from given EcGroup in `curve`.
/// * The underlying C `mbedtls_pk_setup` function fails to set up the `Pk` context.
/// * The `EcPoint::mul` function fails to generate the public key point.
pub fn private_from_ec_components_with_rng<F: Random>(mut curve: EcGroup, private_key: Mpi, rng: &mut F) -> Result<Pk> {
Expand Down Expand Up @@ -376,6 +404,39 @@ Please use `private_from_ec_components_with_rng` instead."
Ok(ret)
}

/// Construct a private key from RSA components.
pub fn private_from_rsa_components(components: RsaPrivateComponents<'_>) -> Result<Pk> {
mzohreva marked this conversation as resolved.
Show resolved Hide resolved
let mut ret = Self::init();
let (n, p, q, d, e) = match components {
RsaPrivateComponents::WithPrimes { p, q, e } => (None, Some(p), Some(q), None, Some(e)),
RsaPrivateComponents::WithPrivateExponent { n, d, e } => (Some(n), None, None, Some(d), Some(e)),
};
let to_ptr = |mpi: Option<&Mpi>| match mpi {
None => ptr::null(),
Some(mpi) => mpi.handle(),
};
unsafe {
pk_setup(&mut ret.inner, pk_info_from_type(Type::Rsa.into())).into_result()?;
let ctx = ret.inner.pk_ctx as *mut rsa_context;
rsa_import(ctx, to_ptr(n), to_ptr(p), to_ptr(q), to_ptr(d), to_ptr(e)).into_result()?;
rsa_complete(ctx).into_result()?;
}
Ok(ret)
}

/// Construct a public key from RSA components.
pub fn public_from_rsa_components(components: RsaPublicComponents<'_>) -> Result<Pk> {
let mut ret = Self::init();
let RsaPublicComponents { n, e } = components;
unsafe {
pk_setup(&mut ret.inner, pk_info_from_type(Type::Rsa.into())).into_result()?;
let ctx = ret.inner.pk_ctx as *mut rsa_context;
rsa_import(ctx, n.handle(), ptr::null(), ptr::null(), ptr::null(), e.handle()).into_result()?;
rsa_complete(ctx).into_result()?;
}
Ok(ret)
}

pub fn public_custom_algo(algo_id: &[u64], pk: &[u8]) -> Result<Pk> {
let mut ret = Self::init();
unsafe {
Expand Down Expand Up @@ -1564,4 +1625,37 @@ iy6KC991zzvaWY/Ys+q/84Afqa+0qJKQnPuy/7F5GkVdQA/lfbhi
assert_eq!(l.unwrap(), LEN);
}
}

#[test]
fn private_from_rsa_components_sanity() {
let mut pk = Pk::generate_rsa(&mut crate::test_support::rand::test_rng(), 2048, 0x10001).unwrap();
let components = RsaPrivateComponents::WithPrimes {
p: &pk.rsa_private_prime1().unwrap(),
q: &pk.rsa_private_prime2().unwrap(),
e: &Mpi::new(pk.rsa_public_exponent().unwrap() as _).unwrap(),
};
let mut pk2 = Pk::private_from_rsa_components(components).unwrap();
assert_eq!(pk.write_private_der_vec().unwrap(), pk2.write_private_der_vec().unwrap());

let components = RsaPrivateComponents::WithPrivateExponent {
n: &pk.rsa_public_modulus().unwrap(),
d: &pk.rsa_private_exponent().unwrap(),
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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should refactor the test RNG so that it randomizes tests, or have another test RNG. (cc. @jethrogb)

}

#[test]
fn public_from_rsa_components_sanity() {
let mut pk = Pk::generate_rsa(&mut crate::test_support::rand::test_rng(), 2048, 0x10001).unwrap();
let mut pk = Pk::from_public_key(&pk.write_public_der_vec().unwrap()).unwrap();

let components = RsaPublicComponents {
n: &pk.rsa_public_modulus().unwrap(),
e: &Mpi::new(pk.rsa_public_exponent().unwrap() as _).unwrap(),
};
let mut pk2 = Pk::public_from_rsa_components(components).unwrap();
assert_eq!(pk.write_public_der_vec().unwrap(), pk2.write_public_der_vec().unwrap());
}
}
Loading