From 43933db69c5fd0b447504cbe1d6c64d409e927c2 Mon Sep 17 00:00:00 2001 From: Wiktor Kwapisiewicz Date: Tue, 20 Sep 2022 10:50:13 +0200 Subject: openpgp: Introduce Aead::decrypt_verify. - Adds new one-operation decryption and verification. Some backends, like OpenSSL, do not support AEAD decryption without verification. - Adjust other backends to manually do first decryption and then verification, similarily to how the old code behaved. - Add test that exercises SKESK5 code touching AEAD. - Remove `decrypt` as it is no longer needed. - Blocks !1361. - See #931. --- openpgp/src/crypto/aead.rs | 94 ++++++++++++------------------- openpgp/src/crypto/backend/cng/aead.rs | 46 +++++++++++---- openpgp/src/crypto/backend/nettle/aead.rs | 37 +++++++++--- openpgp/src/crypto/backend/rust/aead.rs | 47 ++++++++++++---- openpgp/src/packet/skesk.rs | 18 ++---- openpgp/src/serialize/stream.rs | 60 ++++++++++++++++++++ 6 files changed, 201 insertions(+), 101 deletions(-) diff --git a/openpgp/src/crypto/aead.rs b/openpgp/src/crypto/aead.rs index fc147b41..fb3b8176 100644 --- a/openpgp/src/crypto/aead.rs +++ b/openpgp/src/crypto/aead.rs @@ -15,7 +15,6 @@ use crate::utils::{ use crate::Error; use crate::Result; use crate::crypto::SessionKey; -use crate::crypto::mem::secure_cmp; use crate::seal; use crate::parse::Cookie; @@ -32,12 +31,6 @@ const MAX_CHUNK_SIZE: usize = 1 << 22; // 4MiB /// Maximum size of any Nonce used by an AEAD mode. pub const MAX_NONCE_LEN: usize = 16; -/// Disables authentication checks. -/// -/// This is DANGEROUS, and is only useful for debugging problems with -/// malformed AEAD-encrypted messages. -const DANGER_DISABLE_AUTHENTICATION: bool = false; - /// Converts a chunk size to a usize. pub(crate) fn chunk_size_usize(chunk_size: u64) -> Result { chunk_size.try_into() @@ -58,18 +51,20 @@ pub(crate) fn chunk_size_usize(chunk_size: u64) -> Result { /// [sealed]: https://rust-lang.github.io/api-guidelines/future-proofing.html#sealed-traits-protect-against-downstream-implementations-c-sealed pub trait Aead : seal::Sealed { /// Adds associated data `ad`. - fn update(&mut self, ad: &[u8]); + fn update(&mut self, ad: &[u8]) -> Result<()>; /// Encrypts one block `src` to `dst`. - fn encrypt(&mut self, dst: &mut [u8], src: &[u8]); - /// Decrypts one block `src` to `dst`. - fn decrypt(&mut self, dst: &mut [u8], src: &[u8]); + fn encrypt(&mut self, dst: &mut [u8], src: &[u8]) -> Result<()>; /// Produce the digest. - fn digest(&mut self, digest: &mut [u8]); + fn digest(&mut self, digest: &mut [u8]) -> Result<()>; /// Length of the digest in bytes. fn digest_size(&self) -> usize; + + /// Decrypt one block `src` to `dst` and verify if the digest + /// matches `valid_digest`. + fn decrypt_verify(&mut self, dst: &mut [u8], src: &[u8], valid_digest: &[u8]) -> Result<()>; } /// Whether AEAD cipher is used for data encryption or decryption. @@ -299,13 +294,8 @@ impl<'a, S: Schedule> Decryptor<'a, S> { // is less than `plaintext.len()`, then it is either because we // reached the end of the input or an error occurred. fn read_helper(&mut self, plaintext: &mut [u8]) -> Result { - use std::cmp::Ordering; - let mut pos = 0; - // Buffer to hold a digest. - let mut digest = vec![0u8; self.digest_size]; - // 1. Copy any buffered data. if !self.buffer.is_empty() { let to_copy = cmp::min(self.buffer.len(), plaintext.len()); @@ -400,10 +390,10 @@ impl<'a, S: Schedule> Decryptor<'a, S> { self.aead.context(self.sym_algo, &self.key, iv, CipherOp::Decrypt) .map(|mut aead| { - aead.update(ad); - aead + aead.update(ad)?; + Ok::, anyhow::Error>(aead) }) - })?; + })??; // Decrypt the chunk and check the tag. let to_decrypt = chunk.len() - self.digest_size; @@ -418,15 +408,7 @@ impl<'a, S: Schedule> Decryptor<'a, S> { &mut plaintext[pos..pos + to_decrypt] }; - aead.decrypt(buffer, &chunk[..to_decrypt]); - - // Check digest. - aead.digest(&mut digest); - if secure_cmp(&digest[..], &chunk[to_decrypt..]) - != Ordering::Equal && ! DANGER_DISABLE_AUTHENTICATION - { - return Err(Error::ManipulatedMessage.into()); - } + aead.decrypt_verify(buffer, &chunk[..to_decrypt], &chunk[to_decrypt..])?; if double_buffer { let to_copy = plaintext.len() - pos; @@ -459,20 +441,14 @@ impl<'a, S: Schedule> Decryptor<'a, S> { self.aead.context(self.sym_algo, &self.key, iv, CipherOp::Decrypt) .map(|mut aead| { - aead.update(ad); - aead + aead.update(ad)?; + Ok::, anyhow::Error>(aead) }) - })?; - - aead.digest(&mut digest); + })??; let final_digest = self.source.data(final_digest_size)?; - if final_digest.len() != final_digest_size - || secure_cmp(&digest[..], final_digest) != Ordering::Equal - && ! DANGER_DISABLE_AUTHENTICATION - { - return Err(Error::ManipulatedMessage.into()); - } + + aead.decrypt_verify(&mut [], &[], final_digest)?; // Consume the data only on success so that we keep // returning the error. @@ -683,22 +659,22 @@ impl Encryptor { self.aead.context(self.sym_algo, &self.key, iv, CipherOp::Encrypt) .map(|mut aead| { - aead.update(ad); - aead + aead.update(ad)?; + Ok::, anyhow::Error>(aead) }) - })?; + })??; let inner = self.inner.as_mut().unwrap(); // Encrypt the chunk. - aead.encrypt(&mut self.scratch, &self.buffer); + aead.encrypt(&mut self.scratch, &self.buffer)?; self.bytes_encrypted += self.scratch.len() as u64; self.chunk_index += 1; crate::vec_truncate(&mut self.buffer, 0); inner.write_all(&self.scratch)?; // Write digest. - aead.digest(&mut self.scratch[..self.digest_size]); + aead.digest(&mut self.scratch[..self.digest_size])?; inner.write_all(&self.scratch[..self.digest_size])?; } } @@ -712,21 +688,21 @@ impl Encryptor { self.aead.context(self.sym_algo, &self.key, iv, CipherOp::Encrypt) .map(|mut aead| { - aead.update(ad); - aead + aead.update(ad)?; + Ok::, anyhow::Error>(aead) }) - })?; + })??; let inner = self.inner.as_mut().unwrap(); // Encrypt the chunk. - aead.encrypt(&mut self.scratch, chunk); + aead.encrypt(&mut self.scratch, chunk)?; self.bytes_encrypted += self.scratch.len() as u64; self.chunk_index += 1; inner.write_all(&self.scratch)?; // Write digest. - aead.digest(&mut self.scratch[..self.digest_size]); + aead.digest(&mut self.scratch[..self.digest_size])?; inner.write_all(&self.scratch[..self.digest_size])?; } else { // Stash for later. @@ -747,14 +723,14 @@ impl Encryptor { self.aead.context(self.sym_algo, &self.key, iv, CipherOp::Encrypt) .map(|mut aead| { - aead.update(ad); - aead + aead.update(ad)?; + Ok::, anyhow::Error>(aead) }) - })?; + })??; // Encrypt the chunk. unsafe { self.scratch.set_len(self.buffer.len()) } - aead.encrypt(&mut self.scratch, &self.buffer); + aead.encrypt(&mut self.scratch, &self.buffer)?; self.bytes_encrypted += self.scratch.len() as u64; self.chunk_index += 1; crate::vec_truncate(&mut self.buffer, 0); @@ -762,7 +738,7 @@ impl Encryptor { // Write digest. unsafe { self.scratch.set_len(self.digest_size) } - aead.digest(&mut self.scratch[..self.digest_size]); + aead.digest(&mut self.scratch[..self.digest_size])?; inner.write_all(&self.scratch[..self.digest_size])?; } @@ -773,11 +749,11 @@ impl Encryptor { self.aead.context(self.sym_algo, &self.key, iv, CipherOp::Encrypt) .map(|mut aead| { - aead.update(ad); - aead + aead.update(ad)?; + Ok::, anyhow::Error>(aead) }) - })?; - aead.digest(&mut self.scratch[..self.digest_size]); + })??; + aead.digest(&mut self.scratch[..self.digest_size])?; inner.write_all(&self.scratch[..self.digest_size])?; Ok(inner) diff --git a/openpgp/src/crypto/backend/cng/aead.rs b/openpgp/src/crypto/backend/cng/aead.rs index a7b87613..fd4c298f 100644 --- a/openpgp/src/crypto/backend/cng/aead.rs +++ b/openpgp/src/crypto/backend/cng/aead.rs @@ -1,7 +1,9 @@ //! Implementation of AEAD using Windows CNG API. +use std::cmp::Ordering; use crate::{Error, Result}; use crate::crypto::aead::{Aead, CipherOp}; +use crate::crypto::mem::secure_cmp; use crate::seal; use crate::types::{AEADAlgorithm, SymmetricAlgorithm}; @@ -10,6 +12,12 @@ use win_crypto_ng::symmetric::{BlockCipherKey, Aes}; use win_crypto_ng::symmetric::block_cipher::generic_array::{GenericArray, ArrayLength}; use win_crypto_ng::symmetric::block_cipher::generic_array::typenum::{U128, U192, U256}; +/// Disables authentication checks. +/// +/// This is DANGEROUS, and is only useful for debugging problems with +/// malformed AEAD-encrypted messages. +const DANGER_DISABLE_AUTHENTICATION: bool = false; + trait GenericArrayExt> { const LEN: usize; @@ -81,20 +89,25 @@ macro_rules! impl_aead { ($($type: ty),*) => { $( impl Aead for EaxOnline<$type, Encrypt> { - fn update(&mut self, ad: &[u8]) { self.update_assoc(ad) } + fn update(&mut self, ad: &[u8]) -> Result<()> { + self.update_assoc(ad); + Ok(()) + } fn digest_size(&self) -> usize { >::LEN } - fn digest(&mut self, digest: &mut [u8]) { + fn digest(&mut self, digest: &mut [u8]) -> Result<()> { let tag = self.tag_clone(); digest[..tag.len()].copy_from_slice(&tag[..]); + Ok(()) } - fn encrypt(&mut self, dst: &mut [u8], src: &[u8]) { + fn encrypt(&mut self, dst: &mut [u8], src: &[u8]) -> Result<()> { let len = core::cmp::min(dst.len(), src.len()); dst[..len].copy_from_slice(&src[..len]); - EaxOnline::<$type, Encrypt>::encrypt(self, &mut dst[..len]) + EaxOnline::<$type, Encrypt>::encrypt(self, &mut dst[..len]); + Ok(()) } - fn decrypt(&mut self, _dst: &mut [u8], _src: &[u8]) { + fn decrypt_verify(&mut self, _dst: &mut [u8], _src: &[u8], _valid_digest: &[u8]) -> Result<()> { panic!("AEAD decryption called in the encryption context") } } @@ -102,21 +115,34 @@ macro_rules! impl_aead { )* $( impl Aead for EaxOnline<$type, Decrypt> { - fn update(&mut self, ad: &[u8]) { self.update_assoc(ad) } + fn update(&mut self, ad: &[u8]) -> Result<()> { + self.update_assoc(ad); + Ok(()) + } fn digest_size(&self) -> usize { >::LEN } - fn digest(&mut self, digest: &mut [u8]) { + fn digest(&mut self, digest: &mut [u8]) -> Result<()> { let tag = self.tag_clone(); digest[..tag.len()].copy_from_slice(&tag[..]); + Ok(()) } - fn encrypt(&mut self, _dst: &mut [u8], _src: &[u8]) { + fn encrypt(&mut self, _dst: &mut [u8], _src: &[u8]) -> Result<()> { panic!("AEAD encryption called in the decryption context") } - fn decrypt(&mut self, dst: &mut [u8], src: &[u8]) { + fn decrypt_verify(&mut self, dst: &mut [u8], src: &[u8], valid_digest: &[u8]) -> Result<()> { let len = core::cmp::min(dst.len(), src.len()); dst[..len].copy_from_slice(&src[..len]); - self.decrypt_unauthenticated_hazmat(&mut dst[..len]) + self.decrypt_unauthenticated_hazmat(&mut dst[..len]); + let mut digest = vec![0u8; self.digest_size()]; + + self.digest(&mut digest)?; + if secure_cmp(&digest[..], valid_digest) + != Ordering::Equal && ! DANGER_DISABLE_AUTHENTICATION + { + return Err(Error::ManipulatedMessage.into()); + } + Ok(()) } } impl seal::Sealed for EaxOnline<$type, Decrypt> {} diff --git a/openpgp/src/crypto/backend/nettle/aead.rs b/openpgp/src/crypto/backend/nettle/aead.rs index e7ae9d85..bef98831 100644 --- a/openpgp/src/crypto/backend/nettle/aead.rs +++ b/openpgp/src/crypto/backend/nettle/aead.rs @@ -1,25 +1,46 @@ //! Implementation of AEAD using Nettle cryptographic library. +use std::cmp::Ordering; + use nettle::{aead, cipher}; use crate::{Error, Result}; use crate::crypto::aead::{Aead, CipherOp}; +use crate::crypto::mem::secure_cmp; use crate::seal; use crate::types::{AEADAlgorithm, SymmetricAlgorithm}; +/// Disables authentication checks. +/// +/// This is DANGEROUS, and is only useful for debugging problems with +/// malformed AEAD-encrypted messages. +const DANGER_DISABLE_AUTHENTICATION: bool = false; + impl seal::Sealed for T {} impl Aead for T { - fn update(&mut self, ad: &[u8]) { - self.update(ad) + fn update(&mut self, ad: &[u8]) -> Result<()> { + self.update(ad); + Ok(()) } - fn encrypt(&mut self, dst: &mut [u8], src: &[u8]) { - self.encrypt(dst, src) + fn encrypt(&mut self, dst: &mut [u8], src: &[u8]) -> Result<()> { + self.encrypt(dst, src); + Ok(()) } - fn decrypt(&mut self, dst: &mut [u8], src: &[u8]) { - self.decrypt(dst, src) + fn decrypt_verify(&mut self, dst: &mut [u8], src: &[u8], valid_digest: &[u8]) -> Result<()> { + self.decrypt(dst, src); + let mut digest = vec![0u8; self.digest_size()]; + + self.digest(&mut digest); + if secure_cmp(&digest[..], valid_digest) + != Ordering::Equal && ! DANGER_DISABLE_AUTHENTICATION + { + return Err(Error::ManipulatedMessage.into()); + } + Ok(()) } - fn digest(&mut self, digest: &mut [u8]) { - self.digest(digest) + fn digest(&mut self, digest: &mut [u8]) -> Result<()> { + self.digest(digest); + Ok(()) } fn digest_size(&self) -> usize { self.digest_size() diff --git a/openpgp/src/crypto/backend/rust/aead.rs b/openpgp/src/crypto/backend/rust/aead.rs index e5fcdb0b..30bbf546 100644 --- a/openpgp/src/crypto/backend/rust/aead.rs +++ b/openpgp/src/crypto/backend/rust/aead.rs @@ -1,6 +1,7 @@ //! Implementation of AEAD using pure Rust cryptographic libraries. use std::cmp; +use std::cmp::Ordering; use cipher::{BlockCipher, NewBlockCipher}; use cipher::block::Block; @@ -10,9 +11,16 @@ use generic_array::{ArrayLength, GenericArray}; use crate::{Error, Result}; use crate::crypto::aead::{Aead, CipherOp}; +use crate::crypto::mem::secure_cmp; use crate::seal; use crate::types::{AEADAlgorithm, SymmetricAlgorithm}; +/// Disables authentication checks. +/// +/// This is DANGEROUS, and is only useful for debugging problems with +/// malformed AEAD-encrypted messages. +const DANGER_DISABLE_AUTHENTICATION: bool = false; + trait GenericArrayExt> { const LEN: usize; @@ -37,26 +45,29 @@ where Cipher: BlockCipher + NewBlockCipher + Clone, Cipher::ParBlocks: ArrayLength>, { - fn update(&mut self, ad: &[u8]) { - self.update_assoc(ad) + fn update(&mut self, ad: &[u8]) -> Result<()> { + self.update_assoc(ad); + Ok(()) } fn digest_size(&self) -> usize { eax::Tag::LEN } - fn digest(&mut self, digest: &mut [u8]) { + fn digest(&mut self, digest: &mut [u8]) -> Result<()> { let tag = self.tag_clone(); digest[..tag.len()].copy_from_slice(&tag[..]); + Ok(()) } - fn encrypt(&mut self, dst: &mut [u8], src: &[u8]) { + fn encrypt(&mut self, dst: &mut [u8], src: &[u8]) -> Result<()> { let len = cmp::min(dst.len(), src.len()); dst[..len].copy_from_slice(&src[..len]); - Self::encrypt(self, &mut dst[..len]) + Self::encrypt(self, &mut dst[..len]); + Ok(()) } - fn decrypt(&mut self, _dst: &mut [u8], _src: &[u8]) { + fn decrypt_verify(&mut self, _dst: &mut [u8], _src: &[u8], _valid_digest: &[u8]) -> Result<()> { panic!("AEAD decryption called in the encryption context") } } @@ -66,27 +77,39 @@ where Cipher: BlockCipher + NewBlockCipher + Clone, Cipher::ParBlocks: ArrayLength>, { - fn update(&mut self, ad: &[u8]) { - self.update_assoc(ad) + fn update(&mut self, ad: &[u8]) -> Result<()> { + self.update_assoc(ad); + Ok(()) } fn digest_size(&self) -> usize { eax::Tag::LEN } - fn digest(&mut self, digest: &mut [u8]) { + fn digest(&mut self, digest: &mut [u8]) -> Result<()> { let tag = self.tag_clone(); digest[..tag.len()].copy_from_slice(&tag[..]); + Ok(()) } - fn encrypt(&mut self, _dst: &mut [u8], _src: &[u8]) { + fn encrypt(&mut self, _dst: &mut [u8], _src: &[u8]) -> Result<()> { panic!("AEAD encryption called in the decryption context") } - fn decrypt(&mut self, dst: &mut [u8], src: &[u8]) { + fn decrypt_verify(&mut self, dst: &mut [u8], src: &[u8], valid_digest: &[u8]) -> Result<()> { let len = core::cmp::min(dst.len(), src.len()); dst[..len].copy_from_slice(&src[..len]); - self.decrypt_unauthenticated_hazmat(&mut dst[..len]) + self.decrypt_unauthenticated_hazmat(&mut dst[..len]); + + let mut digest = vec![0u8; self.digest_size()]; + + self.digest(&mut digest)?; + if secure_cmp(&digest[..], valid_digest) + != Ordering::Equal && ! DANGER_DISABLE_AUTHENTICATION + { + return Err(Error::ManipulatedMessage.into()); + } + Ok(()) } } diff --git a/openpgp/src/packet/skesk.rs b/openpgp/src/packet/skesk.rs index 96cb0119..1a3a74f7 100644 --- a/openpgp/src/packet/skesk.rs +++ b/openpgp/src/packet/skesk.rs @@ -462,15 +462,15 @@ impl SKESK5 { // Prepare associated data. let ad = [0xc3, 5, esk_algo.into(), esk_aead.into()]; - ctx.update(&ad); + ctx.update(&ad)?; // We need to prefix the cipher specifier to the session key. let mut esk = vec![0u8; session_key.len()]; - ctx.encrypt(&mut esk, session_key); + ctx.encrypt(&mut esk, session_key)?; // Digest. let mut digest = vec![0u8; esk_aead.digest_size()?]; - ctx.digest(&mut digest); + ctx.digest(&mut digest)?; SKESK5::new(esk_algo, esk_aead, s2k, iv.into_boxed_slice(), esk.into(), digest.into_boxed_slice()) @@ -499,16 +499,10 @@ impl SKESK5 { let ad = [0xc3, 5 /* Version. */, self.symmetric_algo().into(), self.aead_algo.into()]; - cipher.update(&ad); + cipher.update(&ad)?; let mut plain: SessionKey = vec![0; esk.len()].into(); - let mut digest = vec![0; self.aead_algo.digest_size()?]; - cipher.decrypt(&mut plain, esk); - cipher.digest(&mut digest); - if digest[..] == self.aead_digest[..] { - Ok((SymmetricAlgorithm::Unencrypted, plain)) - } else { - Err(Error::ManipulatedMessage.into()) - } + cipher.decrypt_verify(&mut plain, esk, &self.aead_digest[..])?; + Ok((SymmetricAlgorithm::Unencrypted, plain)) } else { Err(Error::MalformedPacket( "No encrypted session key in v5 SKESK packet".into()) diff --git a/openpgp/src/serialize/stream.rs b/openpgp/src/serialize/stream.rs index 6c7d0958..49057cc1 100644 --- a/openpgp/src/serialize/stream.rs +++ b/openpgp/src/serialize/stream.rs @@ -3844,4 +3844,64 @@ mod test { Ok(()) } + + // Example copied from `Encryptor::aead_algo` and slightly + // adjusted since the doctest from `Encryptor::aead_algo` does not + // run. Additionally this test case utilizes + // `AEADAlgorithm::const_default` to detect which algorithm to + // use. + #[test] + fn experimental_aead_encryptor() -> Result<()> { + use std::io::Write; + use crate::types::AEADAlgorithm; + use crate::policy::StandardPolicy; + use crate::serialize::stream::{ + Message, Encryptor, LiteralWriter, + }; + use crate::parse::stream::{ + DecryptorBuilder, VerificationHelper, + DecryptionHelper, MessageStructure, + }; + + let mut sink = vec![]; + let message = Message::new(&mut sink); + let message = + Encryptor::with_passwords(message, Some("совершенно секретно")) + .aead_algo(AEADAlgorithm::const_default()) + .build()?; + let mut message = LiteralWriter::new(message).build()?; + message.write_all(b"Hello world.")?; + message.finalize()?; + + struct Helper; + + impl VerificationHelper for Helper { + fn get_certs(&mut self, _ids: &[crate::KeyHandle]) -> Result> where { + Ok(Vec::new()) + } + + fn check(&mut self, _structure: MessageStructure) -> Result<()> { + Ok(()) + } + } + + impl DecryptionHelper for Helper { + fn decrypt(&mut self, _: &[PKESK], skesks: &[SKESK], + _sym_algo: Option, + mut decrypt: D) -> Result> + where D: FnMut(SymmetricAlgorithm, &SessionKey) -> bool + { + skesks[0].decrypt(&"совершенно секретно".into()) + .map(|(algo, session_key)| decrypt(algo, &session_key))?; + Ok(None) + } + } + + let p = &StandardPolicy::new(); + let mut v = DecryptorBuilder::from_bytes(&sink)?.with_policy(p, None, Helper)?; + let mut content = vec![]; + v.read_to_end(&mut content)?; + assert_eq!(content, b"Hello world."); + Ok(()) + } } -- cgit v1.2.3