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

0.12.3 from_private_key_der() accepts incorrect EcdsaSigningAlgorithm for PKCS8 DER #704

Closed
cpu opened this issue Feb 20, 2025 · 3 comments

Comments

@cpu
Copy link
Contributor

cpu commented Feb 20, 2025

Problem:

Hi there 👋

In 0.12.3 it looks like EcdsaPrivateKey::from_private_key_der() changed such that it no longer enforces that the alg: &'static EcdsaSigningAlgorithm matches the parsed key. I think this might have been a side-effect of #686 (and perhaps the EVP_PKEY refactoring?) where it looks like the new LcPtr::<EVP_PKEY>::parse_rfc5208_private_key() codepath doesn't take the alg into account, but does pass it forward in the returned Self.

You can reproduce this issue with the following Rust-script:

#!/usr/bin/env -S cargo -Z script
---cargo
[package]
edition = "2021"
[dependencies]
# Change to 1.12.2 to fix the bug, or 1.12.3 to reproduce.
aws-lc-rs = "=1.12.3"
rustls-pki-types = "1"
---

use aws_lc_rs::signature;
use rustls_pki_types::pem::PemObject;
use rustls_pki_types::PrivatePkcs8KeyDer;

fn main() {
    // P-384 test key
    let pk_pem = r#"-----BEGIN PRIVATE KEY-----
MIG2AgEAMBAGByqGSM49AgEGBSuBBAAiBIGeMIGbAgEBBDCox+o8d2IzZRUaW91Q
+5XhSTvppqz3IE6zp+t+eV7cjN+03FpjYdzI5MUoYMDvuw2hZANiAASpYDU237gY
F2L24KJSs/NlEHyXs6tKebsin6uVklyDu3WB7aS9NfKatnNF4Dm4l8fxtXU0bDMk
TJewtdXtUp5YK9kffYrWgDuhjq4X2SiUmOdYdDKzleh2ebpLokzCSxk=
-----END PRIVATE KEY-----"#;
    let pk_der = PrivatePkcs8KeyDer::from_pem_slice(pk_pem.as_ref()).unwrap();

    // P-256 ECDSA signature algorithm (not P-384!) provided to from_private_key()
    let res = signature::EcdsaKeyPair::from_private_key_der(&signature::ECDSA_P256_SHA256_ASN1_SIGNING, pk_der.secret_pkcs8_der());
    dbg!(&res);
    // Expected: KeyRejected("WrongAlgorithm") Error
    res.unwrap_err();
}

With =0.12.2 the program exits cleanly and displays the expected error:

[/home/daniel/.cargo/target/a7/a57459a46dc135/repro.rs:28:5] &res = Err(
    KeyRejected(
        "WrongAlgorithm",
    ),
)

With =0.12.3 we panic on an unwrap of Ok():

[/home/daniel/.cargo/target/a7/a57459a46dc135/repro.rs:28:5] &res = Ok(
    EcdsaKeyPair { public_key: EcdsaPublicKey("04a9603536dfb8181762f6e0a252b3f365107c97b3ab4a79bb229fab95925c83bb7581eda4bd35f29ab67345e039b897c7f1b575346c33244c97b0b5d5ed529e582bd91f7d8ad6803ba18eae17d9289498e7587432b395e87679ba4ba24cc24b19") },
)

thread 'main' panicked at /home/daniel/.cargo/target/a7/a57459a46dc135/repro.rs:30:9:
called `Result::unwrap_err()` on an `Ok` value: EcdsaKeyPair { public_key: EcdsaPublicKey("04a9603536dfb8181762f6e0a252b3f365107c97b3ab4a79bb229fab95925c83bb7581eda4bd35f29ab67345e039b897c7f1b575346c33244c97b0b5d5ed529e582bd91f7d8ad6803ba18eae17d9289498e7587432b395e87679ba4ba24cc24b19") }

Solution:

This was reported to us downstream in rustls/rcgen#317 because rcgen has some (cumbersome) logic that calls from_private_key_der() for a series of EcdsaSigningAlgorithm choices, expecting that an error will be produced for incorrect guesses.

If possible it would be nice to have the validation of algorithm ID restored :-)

@justsmth
Copy link
Contributor

justsmth commented Feb 20, 2025

Thanks for the notification! I went ahead and yanked the release.

I've found where the bug is. I'll work on getting it fixed and tested. Hopefully, have another patch out soon.

@justsmth
Copy link
Contributor

Hello! We just released aws-lc-rs v1.12.4, which has a fix for this. This issue will now be closed.

Please let us know if you have any other problems related to our library. Thanks again for letting us now about this! 😊

@cpu
Copy link
Contributor Author

cpu commented Feb 21, 2025

Thanks for the super quick fix. Much appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants