From 8c50ba96a5434aeefbf44e0d034072dfc6669521 Mon Sep 17 00:00:00 2001 From: Justus Winter Date: Mon, 14 Dec 2020 16:37:33 +0100 Subject: openpgp: Change general purpose keys to have a signing subkey. - Certificates with a primary key that is not signing capable, and a subkey that is, are strictly more secure than ones that combine signing and certification capabilities in the primary key. - If the owner of a certificate with a signing-capable primary key can be tricked into creating a binary signature over carefully chosen attacker-controlled data, this signature can be repurposed to bind arbitrary attacker-controlled components to the certificate using a chosen-prefix collision attack on the hash function (see e.g. "SHA-1 is a Shambles" for a similar attack). - Having a separate signing-subkey mitigates the attack, because signatures by the signing subkey cannot bind components to the certificate. --- openpgp/src/cert.rs | 14 ++++++++++++-- openpgp/src/cert/amalgamation/key.rs | 3 ++- openpgp/src/cert/amalgamation/key/iter.rs | 4 ++-- openpgp/src/cert/builder.rs | 18 ++++++++++++------ 4 files changed, 28 insertions(+), 11 deletions(-) diff --git a/openpgp/src/cert.rs b/openpgp/src/cert.rs index fc548cd2..648be37a 100644 --- a/openpgp/src/cert.rs +++ b/openpgp/src/cert.rs @@ -4034,6 +4034,8 @@ mod test { + 1 // binding signature + 1 // subkey + 1 // binding signature + + 1 // subkey + + 1 // binding signature ); let cert = check_set_validity_period(p, cert); assert_eq!(cert.clone().into_packet_pile().children().count(), @@ -4045,6 +4047,8 @@ mod test { + 2 // two new binding signatures + 1 // subkey + 1 // binding signature + + 1 // subkey + + 1 // binding signature ); } @@ -4075,6 +4079,8 @@ mod test { + 1 // binding signature + 1 // subkey + 1 // binding signature + + 1 // subkey + + 1 // binding signature ); let cert = check_set_validity_period(p, cert); assert_eq!(cert.clone().into_packet_pile().children().count(), @@ -4089,6 +4095,8 @@ mod test { + 2 // two new binding signatures + 1 // subkey + 1 // binding signature + + 1 // subkey + + 1 // binding signature ); assert_eq!(&primary_uid, cert.with_policy(p, None)?.primary_userid()?.userid()); Ok(()) @@ -5674,11 +5682,12 @@ Pu1xwz57O4zo1VYf6TqHJzVC3OMvMUM2hhdecMUe5x6GorNaj6g= let (cert, _) = CertBuilder::general_purpose( None, Some("alice@example.org")).generate().unwrap(); assert_eq!(cert.userids().count(), 1); - assert_eq!(cert.subkeys().count(), 1); + assert_eq!(cert.subkeys().count(), 2); assert_eq!(cert.unknowns().count(), 0); assert_eq!(cert.bad_signatures().count(), 0); assert_eq!(cert.userids().nth(0).unwrap().self_signatures().len(), 1); assert_eq!(cert.subkeys().nth(0).unwrap().self_signatures().len(), 1); + assert_eq!(cert.subkeys().nth(1).unwrap().self_signatures().len(), 1); // Create a variant of cert where the signatures have // additional information in the unhashed area. @@ -5698,11 +5707,12 @@ Pu1xwz57O4zo1VYf6TqHJzVC3OMvMUM2hhdecMUe5x6GorNaj6g= let cert_b = Cert::from_packets(packets.into_iter())?; let cert = cert.merge_public_and_secret(cert_b)?; assert_eq!(cert.userids().count(), 1); - assert_eq!(cert.subkeys().count(), 1); + assert_eq!(cert.subkeys().count(), 2); assert_eq!(cert.unknowns().count(), 0); assert_eq!(cert.bad_signatures().count(), 0); assert_eq!(cert.userids().nth(0).unwrap().self_signatures().len(), 1); assert_eq!(cert.subkeys().nth(0).unwrap().self_signatures().len(), 1); + assert_eq!(cert.subkeys().nth(1).unwrap().self_signatures().len(), 1); Ok(()) } diff --git a/openpgp/src/cert/amalgamation/key.rs b/openpgp/src/cert/amalgamation/key.rs index f8fbaa01..5962ea4b 100644 --- a/openpgp/src/cert/amalgamation/key.rs +++ b/openpgp/src/cert/amalgamation/key.rs @@ -154,7 +154,8 @@ //! # CertBuilder::general_purpose(None, Some("alice@example.org")) //! # .generate()?; //! # let timestamp = None; -//! # let issuer = cert.fingerprint(); +//! # let issuer = cert.with_policy(p, None)?.keys() +//! # .for_signing().nth(0).unwrap().fingerprint(); //! # let mut i = 0; //! let cert = cert.with_policy(p, timestamp)?; //! if let RevocationStatus::Revoked(_) = cert.revocation_status() { diff --git a/openpgp/src/cert/amalgamation/key/iter.rs b/openpgp/src/cert/amalgamation/key/iter.rs index 0601c2f0..53a72f79 100644 --- a/openpgp/src/cert/amalgamation/key/iter.rs +++ b/openpgp/src/cert/amalgamation/key/iter.rs @@ -434,7 +434,7 @@ impl<'a, P, R> KeyAmalgamationIter<'a, P, R> /// // Use it. /// # i += 1; /// } - /// # assert_eq!(i, 2); + /// # assert_eq!(i, 3); /// # Ok(()) } /// ``` pub fn supported(mut self) -> Self { @@ -1553,7 +1553,7 @@ impl<'a, P, R> ValidKeyAmalgamationIter<'a, P, R> /// // Use it. /// # i += 1; /// } - /// # assert_eq!(i, 2); + /// # assert_eq!(i, 3); /// # Ok(()) } /// ``` pub fn supported(mut self) -> Self { diff --git a/openpgp/src/cert/builder.rs b/openpgp/src/cert/builder.rs index 15d0a350..627bd4f0 100644 --- a/openpgp/src/cert/builder.rs +++ b/openpgp/src/cert/builder.rs @@ -233,9 +233,10 @@ impl CertBuilder<'_> { /// Generates a general-purpose certificate. /// /// The returned builder is set to generate a certificate with a - /// certification- and signature-capable primary key, and an - /// encryption-capable subkey. The subkey is marked as being - /// appropriate for both data in transit and data at rest. + /// certification-capable primary key, a signing-capable subkye, + /// and an encryption-capable subkey. The encryption subkey is + /// marked as being appropriate for both data in transit and data + /// at rest. /// /// # Examples /// @@ -248,7 +249,7 @@ impl CertBuilder<'_> { /// CertBuilder::general_purpose(None, /// Some("Alice Lovelace ")) /// .generate()?; - /// # assert_eq!(cert.keys().count(), 2); + /// # assert_eq!(cert.keys().count(), 3); /// # assert_eq!(cert.userids().count(), 1); /// # Ok(()) /// # } @@ -262,12 +263,17 @@ impl CertBuilder<'_> { ciphersuite: ciphersuite.into().unwrap_or(Default::default()), primary: KeyBlueprint { flags: KeyFlags::empty() - .set_certification() - .set_signing(), + .set_certification(), validity: Some(time::Duration::new(3 * 52 * 7 * 24 * 60 * 60, 0)), ciphersuite: None, }, subkeys: vec![ + KeyBlueprint { + flags: KeyFlags::empty() + .set_signing(), + validity: None, + ciphersuite: None, + }, KeyBlueprint { flags: KeyFlags::empty() .set_transport_encryption() -- cgit v1.2.3