diff options
author | Neal H. Walfield <neal@pep.foundation> | 2019-10-12 14:24:06 +0200 |
---|---|---|
committer | Neal H. Walfield <neal@pep.foundation> | 2019-10-22 23:03:02 +0200 |
commit | a3c4f05848d0d9d46a7b9cdc9b227ed27189e231 (patch) | |
tree | d905a95b2fccf3f03b3fedae9382489d3ed7773b /openpgp | |
parent | 7d6496aa0786c8576a9e3130dfd03d2328e7bf9b (diff) |
openpgp: Fix AEAD encryption.
- The AEAD implementation did not correctly handle messages where
the last chunk was a bit smaller than the chunk size.
Specifically, assume that the chunk size is 32 bytes and the
digest size is 16 bytes, and consider a message with 17 bytes of
data. That message will be encrypted as follows:
[ chunk1 ][ tag1 ][ tagF ]
17B 16B 16B
If we read a chunk and a digest, we'll successfully read 48
bytes of data. Unfortunately, we'll have over read: the
last 15 bytes are from the final tag.
To correctly handle this case, we have to make sure that
there are at least a tag worth of bytes left over when we
read a chunk and a tag.
- Test encrypting and decrypting more message sizes using AEAD.
- Also, check that the AEAD implementation correctly handles
corruption (specifically, a corrupted final tag).
Diffstat (limited to 'openpgp')
-rw-r--r-- | openpgp/src/crypto/aead.rs | 352 | ||||
-rw-r--r-- | openpgp/src/parse/parse.rs | 3 | ||||
-rw-r--r-- | openpgp/src/serialize/stream.rs | 124 |
3 files changed, 260 insertions, 219 deletions
diff --git a/openpgp/src/crypto/aead.rs b/openpgp/src/crypto/aead.rs index e27f493d..f5b8424e 100644 --- a/openpgp/src/crypto/aead.rs +++ b/openpgp/src/crypto/aead.rs @@ -1,6 +1,6 @@ use std::cmp; use std::fmt; -use std::io::{self, Read}; +use std::io; use nettle::{aead, cipher}; use buffered_reader::BufferedReader; @@ -198,221 +198,187 @@ impl<'a> Decryptor<'a> { 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.len() > 0 { let to_copy = cmp::min(self.buffer.len(), plaintext.len()); &plaintext[..to_copy].copy_from_slice(&self.buffer[..to_copy]); self.buffer.drain(..to_copy); - pos = to_copy; - } - if pos == plaintext.len() { - return Ok(pos); + pos = to_copy; + if pos == plaintext.len() { + return Ok(pos); + } } - // 2. Decrypt as many whole chunks as `plaintext` can hold. - let n_chunks = (plaintext.len() - pos) / self.chunk_size; + // 2. Decrypt the data a chunk at a time until we've filled + // `plaintext`. + // + // Unfortunately, framing is hard. + // + // Recall: AEAD data is of the form: + // + // [ chunk1 ][ tag1 ] ... [ chunkN ][ tagN ][ tagF ] + // + // And, all chunks are the same size except for the last + // chunk, which may be shorter. + // + // The naive approach to decryption is to read a chunk and a + // tag at a time. Unfortunately, this may not work if the + // last chunk is a partial chunk. + // + // Assume that the chunk size is 32 bytes and the digest size + // is 16 bytes, and consider a message with 17 bytes of data. + // That message will be encrypted as follows: + // + // [ chunk1 ][ tag1 ][ tagF ] + // 17B 16B 16B + // + // If we read a chunk and a digest, we'll successfully read 48 + // bytes of data. Unfortunately, we'll have over read: the + // last 15 bytes are from the final tag. + // + // To correctly handle this case, we have to make sure that + // there are at least a tag worth of bytes left over when we + // read a chunk and a tag. + + let n_chunks + = (plaintext.len() - pos + self.chunk_size - 1) / self.chunk_size; let chunk_digest_size = self.chunk_size + self.digest_size; - let mut to_copy = n_chunks * self.chunk_size; - let to_read = n_chunks * chunk_digest_size; - - let mut ciphertext = Vec::new(); - let result = (&mut self.source).take(to_read as u64) - .read_to_end(&mut ciphertext); - let short_read; - match result { - Ok(amount) => { - if to_read != 0 && amount == 0 { - // Exhausted source. - return Ok(pos); - } - - // Recall: AEAD data is of the form: - // - // [ chunk1 ][ tag1 ] ... [ chunkN ][ tagN ][ tag ] - // - // And, all chunks are the same size except for the - // last chunk, which may be shorter. - // - // Because only the last chunk can be shorter, if the - // amount read is less than `chunk_size + tag_size`, - // then we know that we've read the last chunk. - // - // Unfortunately, this is not sufficient: if the last - // chunk is `chunk_size - tag size` bytes large, then - // when we read it, we'll read `chunk_size + tag_size` - // bytes, because we'll have also read the final tag! - // - // We can detect this by also checking for EOF. - short_read = amount < to_copy - || (amount > 0 && self.source.eof()); - to_copy = amount; - ciphertext.truncate(to_copy); - }, - // We encountered an error, but we did read some. - Err(_) if pos > 0 => return Ok(pos), - Err(e) => return Err(e.into()), - } - - // Buffer to hold digests. - let mut digest = vec![0u8; self.digest_size]; + let final_digest_size = self.digest_size; - // At the end of the stream, there is an additional tag. Be - // careful not to consume this tag. - let ciphertext_end = if short_read { - ciphertext.len() - self.digest_size - } else { - ciphertext.len() - }; - - for chunk in (&ciphertext[..ciphertext_end]).chunks(chunk_digest_size) { + for _ in 0..n_chunks { let mut aead = self.make_aead()?; - // Digest the associated data. self.hash_associated_data(&mut aead, false); - // Decrypt the chunk. - aead.decrypt( - &mut plaintext[pos..pos + chunk.len() - self.digest_size], - &chunk[..chunk.len() - self.digest_size]); - self.bytes_decrypted += (chunk.len() - self.digest_size) as u64; - - // Check digest. - aead.digest(&mut digest); - let dig_ord = secure_cmp(&digest[..], - &chunk[chunk.len() - self.digest_size..]); - if dig_ord != Ordering::Equal { - return Err(Error::ManipulatedMessage.into()); - } - - // Increase index, update position in plaintext. - self.chunk_index += 1; - pos += chunk.len() - self.digest_size; - } - - if short_read { - // We read the whole ciphertext, now check the final digest. - let mut aead = self.make_aead()?; - self.hash_associated_data(&mut aead, true); - - let mut nada = [0; 0]; - aead.decrypt(&mut nada, b""); - aead.digest(&mut digest); - - let dig_ord = secure_cmp(&digest[..], &ciphertext[ciphertext_end..]); - if dig_ord != Ordering::Equal { - return Err(Error::ManipulatedMessage.into()); - } - } - - if short_read || pos == plaintext.len() { - return Ok(pos); - } - - // 3. The last bit is a partial chunk. Buffer it. - let mut to_copy = plaintext.len() - pos; - assert!(0 < to_copy); - assert!(to_copy < self.chunk_size); - - let mut ciphertext = Vec::new(); - let result = (&mut self.source).take(chunk_digest_size as u64) - .read_to_end(&mut ciphertext); - let short_read; - match result { - Ok(amount) => { - if amount == 0 { + // Do a little dance to avoid exclusively locking + // `self.source`. + let to_read = chunk_digest_size + final_digest_size; + let result = { + match self.source.data(to_read) { + Ok(_) => Ok(self.source.buffer()), + Err(err) => Err(err), + } + }; + + let check_final_tag; + let chunk = match result { + Ok(chunk) => { + if chunk.len() == 0 { + // Exhausted source. + return Ok(pos); + } + + if chunk.len() < final_digest_size { + if pos > 0 { + return Ok(pos); + } else { + return Err(Error::ManipulatedMessage.into()); + } + } + + check_final_tag = chunk.len() < to_read; + + // Return the chunk. + &chunk[..cmp::min(chunk.len(), to_read) - final_digest_size] + }, + // We encountered an error, but we've already read + // something, so return that. + Err(_) if pos > 0 => return Ok(pos), + Err(e) => return Err(e.into()), + }; + + assert!(chunk.len() <= chunk_digest_size); + + if chunk.len() == 0 { + // There is nothing to decrypt: all that is left is + // the final tag. + } else if chunk.len() <= self.digest_size { + // A chunk has to include at least one byte and a tag. + if pos > 0 { return Ok(pos); + } else { + return Err(Error::ManipulatedMessage.into()); + } + } else { + // Decrypt the chunk and check the tag. + let to_decrypt = chunk.len() - self.digest_size; + + // If plaintext doesn't have enough room for the whole + // chunk, then we have to double buffer. + let double_buffer = to_decrypt > plaintext.len() - pos; + let buffer = if double_buffer { + while self.buffer.len() < to_decrypt { + self.buffer.push(0u8); + } + &mut self.buffer[..] + } else { + &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 + { + if pos > 0 { + return Ok(pos); + } else { + return Err(Error::ManipulatedMessage.into()); + } } - // Recall: AEAD data is of the form: - // - // [ chunk1 ][ tag1 ] ... [ chunkN ][ tagN ][ tag ] - // - // And, all chunks are the same size except for the - // last chunk, which may be shorter. - // - // Because only the last chunk can be shorter, if the - // amount read is less than `chunk_size + tag_size`, - // then we know that we've read the last chunk. - // - // Unfortunately, this is not sufficient: if the last - // chunk is `chunk_size - tag size` bytes large, then - // when we read it, we'll read `chunk_size + tag_size` - // bytes, because we'll have also read the final tag! - // - // We can detect this by also checking for EOF. - short_read = amount < chunk_digest_size || self.source.eof(); - - // Make sure `ciphertext` is not larger than the - // amount of data that was actually read. - ciphertext.truncate(amount); - - // Make sure we don't read more than is available. - to_copy = cmp::min(to_copy, - ciphertext.len() - self.digest_size - - if short_read { - self.digest_size - } else { - 0 - }); - }, - // We encountered an error, but we did read some. - Err(_) if pos > 0 => return Ok(pos), - Err(e) => return Err(e.into()), - } - assert!(ciphertext.len() <= self.chunk_size + self.digest_size); - - let mut aead = self.make_aead()?; - - // Digest the associated data. - self.hash_associated_data(&mut aead, false); - - // At the end of the stream, there is an additional tag. Be - // careful not to consume this tag. - let ciphertext_end = if short_read { - ciphertext.len() - self.digest_size - } else { - ciphertext.len() - }; + if double_buffer { + let to_copy = plaintext.len() - pos; + assert!(0 < to_copy); + assert!(to_copy < self.chunk_size); + + &plaintext[pos..pos + to_copy] + .copy_from_slice(&self.buffer[..to_copy]); + self.buffer.drain(..to_copy); + pos += to_copy; + } else { + pos += to_decrypt; + } - while self.buffer.len() < ciphertext_end - self.digest_size { - self.buffer.push(0u8); - } - self.buffer.truncate(ciphertext_end - self.digest_size); - - // Decrypt the chunk. - aead.decrypt(&mut self.buffer, - &ciphertext[..ciphertext_end - self.digest_size]); - self.bytes_decrypted += (ciphertext_end - self.digest_size) as u64; - - // Check digest. - aead.digest(&mut digest); - let mac_ord = secure_cmp( - &digest[..], - &ciphertext[ciphertext_end - self.digest_size..ciphertext_end]); - if mac_ord != Ordering::Equal { - return Err(Error::ManipulatedMessage.into()); - } + // Increase index, update position in plaintext. + self.chunk_index += 1; + self.bytes_decrypted += to_decrypt as u64; - // Increase index. - self.chunk_index += 1; + // Consume the data only on success so that we keep + // returning the error. + self.source.consume(chunk.len()); + } - &plaintext[pos..pos + to_copy].copy_from_slice(&self.buffer[..to_copy]); - self.buffer.drain(..to_copy); - pos += to_copy; + if check_final_tag { + // We read the whole ciphertext, now check the final digest. + let mut aead = self.make_aead()?; + self.hash_associated_data(&mut aead, true); - if short_read { - // We read the whole ciphertext, now check the final digest. - let mut aead = self.make_aead()?; - self.hash_associated_data(&mut aead, true); + let mut nada = [0; 0]; + aead.decrypt(&mut nada, b""); + aead.digest(&mut digest); - let mut nada = [0; 0]; - aead.decrypt(&mut nada, b""); - 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 + { + if pos > 0 { + return Ok(pos); + } else { + return Err(Error::ManipulatedMessage.into()); + } + } - let dig_ord = secure_cmp(&digest[..], &ciphertext[ciphertext_end..]); - if dig_ord != Ordering::Equal { - return Err(Error::ManipulatedMessage.into()); + // Consume the data only on success so that we keep + // returning the error. + self.source.consume(final_digest_size); + break; } } diff --git a/openpgp/src/parse/parse.rs b/openpgp/src/parse/parse.rs index 2883ef3c..5a5a356e 100644 --- a/openpgp/src/parse/parse.rs +++ b/openpgp/src/parse/parse.rs @@ -3619,7 +3619,8 @@ impl<'a> PacketParser<'a> { // `aead::Decryptor` won't see EOF and think that // it has a partial block and it needs to verify // the final chunk. - let amount = aed.chunk_digest_size()? + 1; + let amount + = aed.chunk_digest_size()? + aed.aead().digest_size()?; let data = self.data(amount)?; let dec = aead::Decryptor::new( 1, aed.symmetric_algo(), aed.aead(), aed.chunk_size(), diff --git a/openpgp/src/serialize/stream.rs b/openpgp/src/serialize/stream.rs index cd4ebd15..aa8b30a5 100644 --- a/openpgp/src/serialize/stream.rs +++ b/openpgp/src/serialize/stream.rs @@ -1551,7 +1551,7 @@ mod test { } #[test] - fn aead_short_read() { + fn aead_messages() { // AEAD data is of the form: // // [ chunk1 ][ tag1 ] ... [ chunkN ][ tagN ][ tag ] @@ -1571,9 +1571,10 @@ mod test { // // Make sure we handle this situation correctly. + use std::cmp; + use crate::constants::KeyFlags; use crate::parse::{ - Parse, stream::{ Decryptor, DecryptionHelper, @@ -1581,9 +1582,13 @@ mod test { MessageStructure, }, }; + use crate::tpk::{TPKBuilder, CipherSuite}; + use crate::serialize::stream::{LiteralWriter, Message}; - let tsk = - TPK::from_bytes(crate::tests::key("testy-private.pgp")).unwrap(); + let (tsk, _) = TPKBuilder::new() + .set_cipher_suite(CipherSuite::Cv25519) + .add_encryption_subkey() + .generate().unwrap(); // Build a vector of recipients to hand to Encryptor. let recipients = @@ -1594,21 +1599,10 @@ mod test { .map(|(_, _, key)| key.into()) .collect::<Vec<_>>(); - let mut o = vec![]; - { - let m = Message::new(&mut o); - let encryptor = Encryptor::new( - m, &[], &recipients, None, AEADAlgorithm::EAX) - .unwrap(); - let mut literal = LiteralWriter::new(encryptor, None, None, None) - .unwrap(); - literal.write_all(&vec![0; 4071]).unwrap(); - } - - struct Helper { - tsk: TPK, + struct Helper<'a> { + tsk: &'a TPK, }; - impl VerificationHelper for Helper { + impl<'a> VerificationHelper for Helper<'a> { fn get_public_keys(&mut self, _ids: &[KeyID]) -> Result<Vec<TPK>> { Ok(Vec::new()) } @@ -1616,7 +1610,7 @@ mod test { Ok(()) } } - impl DecryptionHelper for Helper { + impl<'a> DecryptionHelper for Helper<'a> { fn decrypt<D>(&mut self, pkesks: &[PKESK], _skesks: &[SKESK], mut decrypt: D) -> Result<Option<crate::Fingerprint>> where D: FnMut(SymmetricAlgorithm, &SessionKey) -> Result<()> @@ -1624,7 +1618,6 @@ mod test { let mut keypair = self.tsk.keys_all() .key_flags( KeyFlags::default() - .set_encrypt_at_rest(true) .set_encrypt_for_transport(true)) .map(|(_, _, key)| key).next().unwrap() .clone().mark_parts_secret().into_keypair().unwrap(); @@ -1634,10 +1627,91 @@ mod test { } } - let h = Helper { tsk }; - let mut v = Decryptor::from_bytes(&o, h, None).unwrap(); - let mut content = Vec::new(); - v.read_to_end(&mut content).unwrap(); - assert_eq!(content.len(), 4071); + for chunks in 0..3 { + for msg_len in + cmp::max(24, chunks * Encryptor::AEAD_CHUNK_SIZE) - 24 + ..chunks * Encryptor::AEAD_CHUNK_SIZE + 24 + { + eprintln!("Encrypting message of size: {}", msg_len); + + let mut content : Vec<u8> = Vec::new(); + for i in 0..msg_len { + content.push(b'0' + ((i % 10) as u8)); + } + + let mut msg = vec![]; + { + let m = Message::new(&mut msg); + let encryptor = Encryptor::new( + m, &[], &recipients, None, AEADAlgorithm::EAX) + .unwrap(); + let mut literal = LiteralWriter::new(encryptor, None, None, None) + .unwrap(); + literal.write_all(&content).unwrap(); + // literal.finalize().unwrap(); + } + + for &read_len in [ + 37, + Encryptor::AEAD_CHUNK_SIZE - 1, + Encryptor::AEAD_CHUNK_SIZE, + 100 * Encryptor::AEAD_CHUNK_SIZE + ].into_iter() { + for &do_err in [ false, true ].into_iter() { + let mut msg = msg.clone(); + if do_err { + let l = msg.len() - 1; + if msg[l] == 0 { + msg[l] = 1; + } else { + msg[l] = 0; + } + } + + let h = Helper { tsk: &tsk }; + // Note: a corrupted message is only guaranteed + // to error out before it returns EOF. + let mut v = match Decryptor::from_bytes(&msg, h, None) { + Ok(v) => v, + Err(_) if do_err => continue, + Err(err) => panic!("Decrypting message: {}", err), + }; + + let mut buffer = Vec::new(); + while buffer.len() < read_len { + buffer.push(0); + } + + let mut decrypted_content = Vec::new(); + loop { + match v.read(&mut buffer[..read_len]) { + Ok(0) if do_err => + panic!("Expected an error, got EOF"), + Ok(0) => break, + Ok(len) => + decrypted_content.extend_from_slice( + &buffer[..len]), + Err(_) if do_err => break, + Err(err) => + panic!("Decrypting data: {:?}", err), + } + } + + if do_err { + // If we get an error once, we should get + // one again. + for _ in 0..3 { + assert!(v.read(&mut buffer[..read_len]).is_err()); + } + } + + // We only corrupted the final tag, so we + // should get all of the content. + assert_eq!(msg_len, decrypted_content.len()); + assert_eq!(content, decrypted_content); + } + } + } + } } } |