From 3596bb0cf11e91e08596aff516435f17e2cfe2b9 Mon Sep 17 00:00:00 2001 From: Kunal Mehta Date: Thu, 12 Oct 2023 16:20:27 -0400 Subject: [PATCH] redwood: Relax requirements for decrypting messages Even if a source key is no longer valid per policy, we still want them to be able to decrypt a previously valid message for them. We can also drop the revocation/expiry filters, which were mostly theoretical in the SecureDrop context anyways. We first try validating the key with the StandardPolicy before trying again with a NullPolicy to avoid downgrade attacks. Since we now have another type of policy being used, the constant has been renamed to STANDARD_POLICY to be clearer what kind it is. Fixes #6991. --- redwood/src/decryption.rs | 4 +-- redwood/src/keys.rs | 62 +++++++++++++++++++++++---------------- redwood/src/lib.rs | 11 ++++--- 3 files changed, 42 insertions(+), 35 deletions(-) diff --git a/redwood/src/decryption.rs b/redwood/src/decryption.rs index 089b1fdc38..8175063e30 100644 --- a/redwood/src/decryption.rs +++ b/redwood/src/decryption.rs @@ -5,11 +5,9 @@ use crate::keys::secret_key_from_cert; use anyhow::anyhow; use sequoia_openpgp::crypto::{Password, SessionKey}; use sequoia_openpgp::parse::stream::*; -use sequoia_openpgp::policy::Policy; use sequoia_openpgp::types::SymmetricAlgorithm; pub(crate) struct Helper<'a> { - pub(crate) policy: &'a dyn Policy, pub(crate) secret: &'a sequoia_openpgp::Cert, pub(crate) passphrase: &'a Password, } @@ -45,7 +43,7 @@ impl<'a> DecryptionHelper for Helper<'a> { where D: FnMut(SymmetricAlgorithm, &SessionKey) -> bool, { - let key = secret_key_from_cert(self.policy, self.secret)?; + let key = secret_key_from_cert(self.secret)?; for pkesk in pkesks { // Note: this check won't work for messages encrypted with --throw-keyids, diff --git a/redwood/src/keys.rs b/redwood/src/keys.rs index 09075b69a7..2e0bc8c6bc 100644 --- a/redwood/src/keys.rs +++ b/redwood/src/keys.rs @@ -2,23 +2,9 @@ use crate::{Error, Result}; use sequoia_openpgp::cert::prelude::ValidErasedKeyAmalgamation; use sequoia_openpgp::packet::key::{PublicParts, SecretParts, UnspecifiedRole}; use sequoia_openpgp::packet::Key; -use sequoia_openpgp::policy::Policy; +use sequoia_openpgp::policy::{NullPolicy, Policy}; use sequoia_openpgp::Cert; -/// We want to use the same iterators on public and secret keys but it's not -/// really possible to do it in a function because of type differences so we -/// use a macro instead. -macro_rules! filter_keys { - ( $keys:expr, $policy: ident ) => { - $keys - .with_policy($policy, None) - .supported() - .alive() - .revoked(false) - .for_storage_encryption() - }; -} - /// Get public encryption keys from the specified cert, returning an error if /// no valid keys are found. pub(crate) fn keys_from_cert<'a>( @@ -27,7 +13,14 @@ pub(crate) fn keys_from_cert<'a>( ) -> Result>> { // Pull the encryption keys that are compatible with by the standard policy // (e.g. not SHA-1) supported by Sequoia, and not revoked. - let keys: Vec<_> = filter_keys!(cert.keys(), policy).collect(); + let keys: Vec<_> = cert + .keys() + .with_policy(policy, None) + .supported() + .alive() + .revoked(false) + .for_storage_encryption() + .collect(); // Each certificate must have at least one supported encryption key if keys.is_empty() { @@ -37,18 +30,35 @@ pub(crate) fn keys_from_cert<'a>( } } -/// Get the secret encryption key, which is the first and only subkey, from the cert. -pub(crate) fn secret_key_from_cert<'a>( - policy: &'a dyn Policy, - cert: &'a Cert, +/// Get the secret encryption key from the cert. +pub(crate) fn secret_key_from_cert( + cert: &Cert, ) -> Result> { - // Pull the encryption keys that are compatible with by the standard policy - // (e.g. not SHA-1) supported by Sequoia, and not revoked. - // These filter options should be kept in sync with `Helper::decrypt()`. - let keys: Vec<_> = filter_keys!(cert.keys().secret(), policy).collect(); + // Get the secret encryption key for decryption. We don't care about + // whether it is revoked or expired, or even if its not a weak key + // since we're just decrypting. + // We first try to find the key using the StandardPolicy and then + // fall back to a NullPolicy to prevent downgrade attacks. + let key = cert + .keys() + .secret() + .with_policy(crate::STANDARD_POLICY, None) + .for_storage_encryption() + .next(); + if let Some(key) = key { + return Ok(key.key().clone()); + } + + // No key found, try again with a null policy + let null = NullPolicy::new(); + let key = cert + .keys() + .secret() + .with_policy(&null, None) + .for_storage_encryption() + .next(); - // Just return the first encryption key - match keys.get(0) { + match key { Some(key) => Ok(key.key().clone()), None => Err(Error::NoSupportedKeys(cert.fingerprint().to_string())), } diff --git a/redwood/src/lib.rs b/redwood/src/lib.rs index 3565752475..54e1187156 100644 --- a/redwood/src/lib.rs +++ b/redwood/src/lib.rs @@ -24,7 +24,7 @@ mod decryption; mod keys; mod stream; -const POLICY: &StandardPolicy = &StandardPolicy::new(); +const STANDARD_POLICY: &StandardPolicy = &StandardPolicy::new(); #[derive(thiserror::Error, Debug)] pub enum Error { @@ -104,7 +104,7 @@ pub fn generate_source_key_pair( pub fn is_valid_public_key(input: &str) -> Result { let cert = Cert::from_str(input)?; // We don't need the keys, just need to check there's at least one and no error - keys::keys_from_cert(POLICY, &cert)?; + keys::keys_from_cert(STANDARD_POLICY, &cert)?; // And there is no secret key material if cert.is_tsk() { return Err(Error::HasSecretKeyMaterial); @@ -116,7 +116,7 @@ pub fn is_valid_public_key(input: &str) -> Result { pub fn is_valid_secret_key(input: &str, passphrase: String) -> Result { let passphrase: Password = passphrase.into(); let cert = Cert::from_str(input)?; - let secret_key = keys::secret_key_from_cert(POLICY, &cert)?; + let secret_key = keys::secret_key_from_cert(&cert)?; secret_key.decrypt_secret(&passphrase)?; Ok(cert.fingerprint().to_string()) } @@ -164,7 +164,7 @@ fn encrypt( } for cert in certs.iter() { - recipient_keys.extend(keys::keys_from_cert(POLICY, cert)?); + recipient_keys.extend(keys::keys_from_cert(STANDARD_POLICY, cert)?); } // In reverse order, we set up a writer that will write an encrypted and @@ -207,14 +207,13 @@ pub fn decrypt( let recipient = Cert::from_str(&secret_key)?; let passphrase: Password = passphrase.into(); let helper = decryption::Helper { - policy: POLICY, secret: &recipient, passphrase: &passphrase, }; // Now, create a decryptor with a helper using the given Certs. let mut decryptor = DecryptorBuilder::from_bytes(&ciphertext)? - .with_policy(POLICY, None, helper)?; + .with_policy(STANDARD_POLICY, None, helper)?; // Decrypt the data. let mut buffer: Vec = vec![];