slipenkomk/sequoia-sq
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Change sq key delete to refuse to work with weakly bound subkeys.

Browse Source 
- `sq key delete` deletes all secret key material associated with a
    certificate.  Of course, we don't want to delete secret key
    material that we are not confident belongs to the certificate.

  - Imagine Alice creates a new certificate.  Mallory see this, and
    anticipates that she is going to delete the old certificate.  He
    attaches her new encryption-capable subkey to the old certificate
    using some weak cryptography, publishes it, and then Alice gets
    the update to her old certificate via parcimonie.  When she
    deletes the secret key material associated with the old
    certificate, she would also delete her new secret key material.
    Ouch!  Admittedly, this attack is a bit contrived.

  - Alternatively, we could skip subkeys whose bindings rely on
    weak cryptography.  This behavior would probably surprise most
    users.  It could have serious consequences as well, since the
    user thought they deleted the secret key material, but didn't.

  - Instead, we are conservative: if a subkey's binding signature
    relies on weak cryptography AND we have secret key material for
    it, we abort, and suggest using `sq key subkey delete` instead.

  - See #375 and #457.
This commit is contained in:
Neal H. Walfield 2024-11-21 14:02:06 +01:00
parent df23d2bb25
commit faa350b694
No known key found for this signature in database
GPG Key ID: 6863C9AD5B4D22D3
9 changed files with 280 additions and 51 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

80
src/commands/key/delete.rs
View File

@ -4,11 +4,15 @@ use anyhow::Context;
use sequoia_openpgp as openpgp;
use openpgp::Cert;
use openpgp::cert::amalgamation::ValidateAmalgamation;
use openpgp::cert::amalgamation::key::PrimaryKey;
use crate::Result;
use crate::Sq;
use crate::cli;
use crate::common::NULL_POLICY;
use crate::common::key::delete;
use crate::common::key::get_keys;
pub fn dispatch(sq: Sq, command: cli::key::delete::Command)
-> Result<()>
@ -16,15 +20,85 @@ pub fn dispatch(sq: Sq, command: cli::key::delete::Command)
let (cert, cert_source)
= sq.resolve_cert(&command.cert, sequoia_wot::FULLY_TRUSTED)?;
let vc = Cert::with_policy(&cert, sq.policy, sq.time)
// Fail if the certificate is not valid under the current policy.
Cert::with_policy(&cert, sq.policy, sq.time)
.with_context(|| {
sq.hint(format_args!(
"The certificate {} is not valid under the \
current policy. You can still delete individual \
keys using `sq key subkey delete`.",
cert.fingerprint()));
format!("The certificate {} is not valid under the \
current policy.",
cert.fingerprint())
})?;
let kas = vc.keys().collect::<Vec<_>>();
// We want to delete all secret key material associated with the
// certificate, but we don't want to delete secret key material
// that we are not confident belongs to the certificate.
//
// Imagine Alice creates a new certificate. Mallory see this, and
// anticipates that she is going to delete the old certificate.
// He attaches her new encryption-capable subkey to the old
// certificate using some weak cryptography, publishes it, and
// then Alice gets the update to her old certificate via
// parcimonie. When she deletes the secret key material
// associated with the old certificate, she would also delete her
// new secret key material. Ouch! Admittedly, this attack is a
// bit contrived.
//
// Alternatively, we could skip subkeys whose bindings rely on
// weak cryptography. This behavior would probably surprise most
// users. It could have serious consequences as well, since the
// user thought they deleted the secret key material, but didn't.
//
// Instead, we are conservative: if a subkey's binding signature
// relies on weak cryptography AND we have secret key material for
// it, we abort, and suggest using `sq key subkey delete` instead.
delete::delete(sq, &cert, cert_source, &kas,
// Get all keys valid under the NULL policy.
let nc = Cert::with_policy(&cert, NULL_POLICY, sq.time)
.with_context(|| {
format!("The certificate {} is not valid under the \
null policy.",
cert.fingerprint())
})?;
let kas = nc.keys().collect::<Vec<_>>();
let kas = kas.iter().collect::<Vec<_>>();
let to_delete = get_keys(&sq, &cert_source, &kas)?;
// Go through the keys with secret key material, and make sure
// their binding is valid under the current policy.
let mut bad = Vec::new();
for (ka, _remote) in to_delete.iter() {
if ka.primary() {
// We check that the primary key is valid above.
continue;
}
if let Err(err) = ka.component_amalgamation().clone()
.with_policy(sq.policy, sq.time)
{
bad.push((ka.fingerprint(), err));
}
}
if ! bad.is_empty() {
wprintln!("Some keys are not valid according \
to the current policy:");
for (fpr, err) in bad.into_iter() {
wprintln!(" - {}: {}",
fpr,
crate::one_line_error_chain(err));
}
wprintln!("Cowardly refusing to delete all of the secret key \
material to avoid accidentally losing data. Use \
`sq key subkey delete` to delete the keys individually.");
return Err(anyhow::anyhow!(
"The authenticity of some subkeys is uncertain."));
}
delete::delete(sq, &cert, cert_source, to_delete,
command.output, false)
}

6
src/commands/key/password.rs
View File

@ -8,6 +8,7 @@ use openpgp::Cert;
use crate::Result;
use crate::Sq;
use crate::cli;
use crate::common::key::get_keys;
use crate::common::key::password;
pub fn dispatch(sq: Sq, command: cli::key::password::Command)
@ -24,8 +25,11 @@ pub fn dispatch(sq: Sq, command: cli::key::password::Command)
})?;
let kas = vc.keys().collect::<Vec<_>>();
let kas = kas.iter().collect::<Vec<_>>();
password::password(sq, &cert, cert_source, &kas,
let to_change = get_keys(&sq, &cert_source, &kas)?;
password::password(sq, &cert, cert_source, to_change,
command.clear_password,
command.new_password_file.as_deref(),
command.output, false)

6
src/commands/key/subkey/delete.rs
View File

@ -9,6 +9,7 @@ use crate::Result;
use crate::Sq;
use crate::common::key::delete;
use crate::common::NULL_POLICY;
use crate::common::key::get_keys;
pub fn dispatch(sq: Sq, command: crate::cli::key::subkey::delete::Command)
-> Result<()>
@ -26,7 +27,10 @@ pub fn dispatch(sq: Sq, command: crate::cli::key::subkey::delete::Command)
})?;
let kas = sq.resolve_keys(&vc, &cert_source, &command.keys, true)?;
let kas = kas.iter().collect::<Vec<_>>();
delete::delete(sq, &cert, cert_source, &kas,
let to_delete = get_keys(&sq, &cert_source, &kas)?;
delete::delete(sq, &cert, cert_source, to_delete,
command.output, false)
}

7
src/commands/key/subkey/password.rs
View File

@ -7,8 +7,9 @@ use openpgp::Cert;
use crate::Result;
use crate::Sq;
use crate::common::key::password;
use crate::common::NULL_POLICY;
use crate::common::key::get_keys;
use crate::common::key::password;
pub fn dispatch(sq: Sq, command: crate::cli::key::subkey::password::Command)
-> Result<()>
@ -26,9 +27,11 @@ pub fn dispatch(sq: Sq, command: crate::cli::key::subkey::password::Command)
})?;
let kas = sq.resolve_keys(&vc, &cert_source, &command.keys, true)?;
let kas = kas.iter().collect::<Vec<_>>();
let to_change = get_keys(&sq, &cert_source, &kas)?;
password(sq, &cert, cert_source, &kas,
password(sq, &cert, cert_source, to_change,
command.clear_password, command.new_password_file.as_deref(),
command.output, false)
}

21
src/common/key.rs
View File

@ -1,10 +1,7 @@
use sequoia_openpgp as openpgp;
use openpgp::Result;
use openpgp::cert::amalgamation::key::PrimaryKey;
use openpgp::cert::amalgamation::key::ValidErasedKeyAmalgamation;
use openpgp::packet::Key;
use openpgp::packet::key::KeyParts;
use openpgp::packet::key;
use sequoia_keystore as keystore;
@ -36,14 +33,14 @@ pub use password::password;
pub fn get_keys<'a, P>(
sq: &Sq,
cert_source: &FileStdinOrKeyHandle,
kas: &[ValidErasedKeyAmalgamation<'a, P>])
-> Result<Vec<(Key<key::PublicParts, key::UnspecifiedRole>,
bool,
kas: &[&'a ValidErasedKeyAmalgamation<'a, P>])
-> Result<Vec<(&'a ValidErasedKeyAmalgamation<'a, P>,
Option<Vec<keystore::Key>>)>>
where
P: 'a + KeyParts,
{
let mut list: Vec<(Key<_, _>, bool, Option<_>)> = Vec::new();
let mut list: Vec<(&ValidErasedKeyAmalgamation<'a, P>, Option<_>)>
= Vec::new();
let mut no_secret_key_material_count = 0;
@ -59,10 +56,7 @@ where
continue;
}
list.push((
ka.key().clone().parts_into_public(),
ka.primary(),
None));
list.push((ka, None));
}
}
FileStdinOrKeyHandle::KeyHandle(ref _kh) => {
@ -80,10 +74,7 @@ where
continue;
}
list.push((
ka.key().clone().parts_into_public(),
ka.primary(),
Some(remote_keys)));
list.push((ka, Some(remote_keys)));
}
}
}

21
src/common/key/delete.rs
View File

@ -5,38 +5,40 @@ use sequoia_openpgp as openpgp;
use openpgp::Cert;
use openpgp::Packet;
use openpgp::Result;
use openpgp::cert::amalgamation::key::PrimaryKey;
use openpgp::cert::amalgamation::key::ValidErasedKeyAmalgamation;
use openpgp::packet::key::KeyParts;
use openpgp::serialize::Serialize;
use sequoia_keystore as keystore;
use crate::Sq;
use crate::cli::types::FileOrStdout;
use crate::cli::types::FileStdinOrKeyHandle;
use super::get_keys;
pub fn delete<'a, P>(
sq: Sq,
cert: &Cert,
cert_source: FileStdinOrKeyHandle,
kas: &[ValidErasedKeyAmalgamation<'a, P>],
to_delete: Vec<(
&'a ValidErasedKeyAmalgamation<'a, P>,
Option<Vec<keystore::Key>>
)>,
output: Option<FileOrStdout>,
binary: bool)
-> Result<()>
where
P: 'a + KeyParts,
{
let to_delete = get_keys(&sq, &cert_source, kas)?;
let ks = matches!(cert_source, FileStdinOrKeyHandle::KeyHandle(_));
if ks {
// Delete the secret key material from the key store.
for (key, _primary, remote_keys) in to_delete.into_iter() {
for (ka, remote_keys) in to_delete.into_iter() {
let remote_keys = remote_keys.expect("have remote keys");
assert!(! remote_keys.is_empty());
for mut kh in remote_keys.into_iter() {
kh.delete_secret_key_material().with_context(|| {
format!("Deleting {}", key.fingerprint())
format!("Deleting {}", ka.fingerprint())
})?;
}
}
@ -44,9 +46,10 @@ where
// Strip the secret key material from the certificate.
let mut stripped: Vec<Packet> = Vec::new();
for (key, primary, _) in to_delete.into_iter() {
for (ka, _) in to_delete.into_iter() {
let key = ka.key().clone().parts_into_secret().expect("have a secret");
let pk = key.take_secret().0;
if primary {
if ka.primary() {
stripped.push(
Packet::PublicKey(pk.role_into_primary()));
} else {

23
src/common/key/password.rs
View File

@ -6,6 +6,7 @@ use sequoia_openpgp as openpgp;
use openpgp::Cert;
use openpgp::Packet;
use openpgp::Result;
use openpgp::cert::amalgamation::key::PrimaryKey;
use openpgp::cert::amalgamation::key::ValidErasedKeyAmalgamation;
use openpgp::crypto::Password;
use openpgp::packet::key::KeyParts;
@ -24,7 +25,10 @@ pub fn password<'a, P>(
sq: Sq,
cert: &Cert,
cert_source: FileStdinOrKeyHandle,
kas: &[ValidErasedKeyAmalgamation<'a, P>],
to_change: Vec<(
&'a ValidErasedKeyAmalgamation<'a, P>,
Option<Vec<keystore::Key>>
)>,
clear_password: bool,
new_password_file: Option<&Path>,
output: Option<FileOrStdout>,
@ -50,14 +54,13 @@ where
Ok(new_password_.clone().unwrap())
};
let mut list = super::get_keys(&sq, &cert_source, kas)?;
let uid = sq.best_userid(&cert, true);
let ks = matches!(cert_source, FileStdinOrKeyHandle::KeyHandle(_));
if ks {
// Change the password of the secret key material on the key
// store.
for (key, _primary, remote_keys) in list.into_iter() {
for (key, remote_keys) in to_change.into_iter() {
let remote_keys = remote_keys.expect("have remote keys");
assert!(! remote_keys.is_empty());
for mut remote_key in remote_keys.into_iter() {
@ -118,18 +121,20 @@ where
}
} else {
// First, decrypt all secrets.
for (key, _primary, _remote_keys) in list.iter_mut() {
*key = sq.decrypt_key(
let to_change = to_change.into_iter().map(|(ka, remote)| {
let key = ka.key().clone().parts_into_secret()?;
let key = sq.decrypt_key(
Some(&cert),
key.clone().parts_into_secret()?,
key,
true, // May prompt.
false, // Don't allow skipping.
)?.parts_into_public();
}
Ok((key, ka.primary(), remote))
}).collect::<Result<Vec<_>>>()?;
let mut packets: Vec<Packet> = Vec::new();
if let Some(new) = get_new_password()? {
for (key, primary, _remote_keys) in list.into_iter() {
for (key, primary, _remote_keys) in to_change.into_iter() {
let key = key.parts_into_secret()
.expect("have secret key amterial")
.encrypt_secret(&new)?
@ -144,7 +149,7 @@ where
}
}
} else {
for (key, primary, _remote_keys) in list.into_iter() {
for (key, primary, _remote_keys) in to_change.into_iter() {
let key = key.parts_into_public();
if primary {
packets.push(

36
tests/integration/common.rs
View File

@ -802,10 +802,10 @@ impl Sq {
}
/// Delete the specified key.
pub fn key_delete<'a, H, Q>(&self,
cert_handle: H,
output_file: Q)
-> Cert
pub fn try_key_delete<'a, H, Q>(&self,
cert_handle: H,
output_file: Q)
-> Result<Cert>
where H: Into<FileOrKeyHandle>,
Q: Into<Option<&'a Path>>,
{
@ -818,20 +818,34 @@ impl Sq {
match &cert_handle {
FileOrKeyHandle::FileOrStdin(path) => {
cmd.arg("--cert-file").arg(path);
assert!(output_file.is_some());
if let Some(output_file) = output_file {
cmd.arg("--output").arg(output_file);
} else {
cmd.arg("--output").arg("-");
}
}
FileOrKeyHandle::KeyHandle((_kh, s)) => {
cmd.arg("--cert").arg(&s);
if let Some(output_file) = output_file {
cmd.arg("--output").arg(output_file);
}
}
};
if let Some(output_file) = output_file {
cmd.arg("--output").arg(output_file);
}
let output = self.run(cmd, Some(true));
let output = self.run(cmd, None);
self.handle_cert_output(output, cert_handle, output_file, None)
.expect("can parse certificate")
}
/// Delete the specified key.
pub fn key_delete<'a, H, Q>(&self,
cert_handle: H,
output_file: Q)
-> Cert
where H: Into<FileOrKeyHandle>,
Q: Into<Option<&'a Path>>,
{
self.try_key_delete(cert_handle, output_file)
.expect("success")
}
/// Run `sq key revoked` and return the revocation certificate.

131
tests/integration/sq_key_delete.rs
View File

@ -1,7 +1,15 @@
use std::collections::HashSet;
use sequoia_openpgp as openpgp;
use openpgp::Cert;
use openpgp::Fingerprint;
use openpgp::Result;
use openpgp::cert::amalgamation::ValidAmalgamation;
use openpgp::parse::Parse;
use openpgp::types::RevocationStatus;
use super::common::Sq;
use super::common::STANDARD_POLICY;
#[test]
fn sq_key_delete() -> Result<()> {
@ -32,3 +40,126 @@ fn sq_key_delete() -> Result<()> {
Ok(())
}
#[test]
fn unbound_subkey() {
// Make sure we don't delete secret key material if there is an
// unbound subkey.
let sq = Sq::new();
let cert_path = sq.test_data()
.join("keys")
.join("unbound-subkey.pgp");
let cert = Cert::from_file(&cert_path).expect("can read");
let vc = cert.with_policy(STANDARD_POLICY, sq.now())
.expect("valid cert");
// One subkey should be considered invalid.
let bound: HashSet<Fingerprint>
= HashSet::from_iter(vc.keys().map(|ka| ka.fingerprint()));
let all: HashSet<Fingerprint>
= HashSet::from_iter(cert.keys().map(|ka| ka.fingerprint()));
assert!(bound.len() < all.len());
let result = sq.key_delete(&cert_path, None);
for ka in result.keys() {
if bound.contains(&ka.fingerprint()) {
assert!(! ka.has_secret());
} else {
assert!(ka.has_secret());
}
}
}
#[test]
fn soft_revoked_subkey() {
// Make sure we can delete a soft revoked subkey.
let sq = Sq::new();
let cert_path = sq.test_data()
.join("keys")
.join("soft-revoked-subkey.pgp");
let cert = Cert::from_file(&cert_path).expect("can read");
let vc = cert.with_policy(STANDARD_POLICY, sq.now())
.expect("valid cert");
// Make sure the revoked key is there and is really revoked.
let mut revoked = None;
for k in vc.keys().subkeys() {
if let RevocationStatus::Revoked(_) = k.revocation_status() {
assert!(revoked.is_none(),
"Only expected a single revoked subkey");
revoked = Some(k.key_handle());
}
}
if revoked.is_none() {
panic!("Expected a revoked subkey, but didn't fine one");
}
let updated = sq.key_delete(cert_path, None);
assert!(! updated.is_tsk());
}
#[test]
fn hard_revoked_subkey() {
// Make sure we can delete a hard revoked subkey.
let sq = Sq::new();
let cert_path = sq.test_data()
.join("keys")
.join("hard-revoked-subkey.pgp");
let cert = Cert::from_file(&cert_path).expect("can read");
let vc = cert.with_policy(STANDARD_POLICY, sq.now())
.expect("valid cert");
// Make sure the revoked key is there and is really revoked.
let mut revoked = None;
for k in vc.keys().subkeys() {
if let RevocationStatus::Revoked(_) = k.revocation_status() {
assert!(revoked.is_none(),
"Only expected a single revoked subkey");
revoked = Some(k.key_handle());
}
}
if revoked.is_none() {
panic!("Expected a revoked subkey, but didn't fine one");
}
let updated = sq.key_delete(cert_path, None);
assert!(! updated.is_tsk());
}
#[test]
fn sha1_subkey() {
// Make sure we can't delete secret key material if there is a
// subkey that is bound using SHA-1.
let sq = Sq::new();
let cert_path = sq.test_data()
.join("keys")
.join("sha1-subkey-priv.pgp");
let cert = Cert::from_file(&cert_path).expect("can read");
let vc = cert.with_policy(STANDARD_POLICY, sq.now())
.expect("valid cert");
// Make sure the subkey key is there and really uses SHA-1.
let valid_subkeys: Vec<_> = vc.keys().subkeys()
.map(|ka| ka.fingerprint())
.collect();
let all_subkeys: Vec<_> = cert.keys().subkeys()
.map(|ka| ka.fingerprint())
.collect();
assert_eq!(valid_subkeys.len(), 0);
assert_eq!(all_subkeys.len(), 1);
assert!(sq.try_key_delete(cert_path, None).is_err());
}