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

Panic when attempting to extract public key from a secret key with no signed user ID. #259

Open
link2xt opened this issue Dec 8, 2023 · 6 comments

Comments

@link2xt
Copy link
Contributor

link2xt commented Dec 8, 2023

I have exported a private key from deltachat REPL.
Then I have split the key into packets with sq packet dump private-key-default.asc and got these files:

private-key-default.asc-0--SecretKey
private-key-default.asc-1--UserID
private-key-default.asc-2--Signature
private-key-default.asc-3--SecretSubkey
private-key-default.asc-4--Signature

As I tried to reproduce another bug reported on the forum https://support.delta.chat/t/cannot-import-private-key-from-text-file/2846 I removed Signature packets, both 2 (UserID signature) and 4 (key signature) by joining the key back without these packets: sq packet join private-key-default.asc-0--SecretKey private-key-default.asc-1--UserID private-key-default.asc-3--SecretSubkey > key-no-sig.asc.

Here is the resulting key-no-sig.asc file:

-----BEGIN PGP PRIVATE KEY BLOCK-----

xVgEZW54txYJKwYBBAHaRw8BAQdAoIDgbWP13wiTcuiKTRirVG58XW3QN2UMI0Ln
SytfW/AAAP9qJ5LQJ6gopHFi5PEBoBY3Ed0h36gSUoKmitEGHmH1sQ/LzRo8ZW4w
aGljaHIwQGMyLnRlc3RydW4ub3JnPsddBGVueLcSCisGAQQBl1UBBQEBB0DC+eIr
ii3UjrLpYFHQJZ8pBJINbVipYnImn9KmnfJrIgMBCAcAAP908j1t4uqix/JzMTxv
NWV2DIWfmju7JVWqha3/jsYNsBG7
=DrFT
-----END PGP PRIVATE KEY BLOCK-----

Attempting to import it into Delta Chat results in a panic.
Minimal test reproducing the panic in rPGP:

#[test]
fn test_key_no_signature() {
    let p = Path::new("./tests/key-no-sig.asc");
    let mut file = read_file(p.to_path_buf());

    let mut buf = vec![];
    file.read_to_end(&mut buf).unwrap();

    let input = ::std::str::from_utf8(buf.as_slice()).expect("failed to convert to string");
    let (key, _headers) = SignedSecretKey::from_string(input).expect("failed to parse key");
    key.verify().expect("invalid key");
    key.public_key();
}

.public_key() panics:

thread 'test_key_no_signature' panicked at 'missing user ids', src/composed/signed_key/shared.rs:119:35
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
test test_key_no_signature ... FAILED

failures:

failures:
    test_key_no_signature

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 55 filtered out; finished in 0.00s

Panic happens here:

let primary_user = self.users.iter().find(|u| u.is_primary()).map_or_else(
|| self.users.first().expect("missing user ids"),
|user| user,

All unsigned user IDs are removed here:

if user.signatures.is_empty() {
warn!("ignoring unsigned {}", user.id);
false
} else {
true
}
});

@link2xt
Copy link
Contributor Author

link2xt commented Dec 8, 2023

I produced another example with gpg as well:

-----BEGIN PGP PRIVATE KEY BLOCK-----

lFgEZXN7vhYJKwYBBAHaRw8BAQdAfkr84guRcAVCSK3NpS5vlqXBg04+y28mZ6hu
3MxVfhYAAQDDqxDd75+NfHT2XiXzHRh4nityqdxxHU/piWdHNQLOMg90tBFhbGlj
ZUBleGFtcGxlLm9yZ5xdBGVze74SCisGAQQBl1UBBQEBB0BTktPN/mWzt9XjFitw
i0DBOjVBRh6HyVRZDSNUseJqdAMBCAcAAP9ytIdby2BidflOyD1pIm41Rt7CfNr3
onv3LEpJdLk5eBFniHgEGBYIACAWIQT/KJLR2z/w/tKKh//wqCpjjtD2QAUCZXN7
vgIbDAAKCRDwqCpjjtD2QAdrAP9ZDINPMlx1jQaf9W35gRl893wFgN5o9M4MRirw
9+SwVwEAjJ90DJEWMjSWiGZVnKVnEFdVRsodxqt/Xzo92c6cEQ8=
=h/01
-----END PGP PRIVATE KEY BLOCK-----

I created a new key, ran gpg --edit-key, selected UserID with uid 1 command and deleted the signature with delsig command, then saved the key with save command and exported it. Full process is described here:
The process is described in https://support.delta.chat/t/cannot-import-private-key-from-text-file/2846/6?u=link2xt

@dignifiedquire
Copy link
Member

this shouldn’t panic, but is certainly not a valid key

@link2xt
Copy link
Contributor Author

link2xt commented Dec 9, 2023

.verify() actually passes, because it only checks that signatures on UserIDs are correct. If there is no signature or no UserID, it is fine.

@hko-s
Copy link
Contributor

hko-s commented Mar 3, 2024

I think the reason for this problem is that, historically, the metadata for the primary key lives on the binding signature of the "primary User ID". The attempt to extract an unsigned KeyDetails involves attempting to gather information from disparate places in the source SignedKeyDetails.

Concretely: the KeyFlags for the primary key are assumed to be on the primary key binding signature Those KeyFlags are the most blatant thing that's missing in your tests, I think.

This kind of operation is very hairy, because OpenPGP certificates/public keys are complex, and it's not very rigorously defined what the semantics of a given set of key, signature and identity packets are supposed to be.

I'm not sure how to proceed with this problem, and wonder what the practical application is - in what context(s) is SignedKeyDetails::as_unsigned used?

@dignifiedquire
Copy link
Member

we should just fail when reading this key, or return an error, I don’t think this is a format rpgp needs to support, just because the rfc is too vague

@dignifiedquire
Copy link
Member

I got convinced that we will need to add proper support for this…

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

3 participants