From f8fc8961e2da62a97bd8328f752cf39717958759 Mon Sep 17 00:00:00 2001 From: "Neal H. Walfield" Date: Fri, 11 Oct 2019 09:13:25 +0200 Subject: openpgp: More carefully handle the last chunk in an AEAD message. - AEAD data is of the form: [ chunk1 ][ tag1 ] ... [ chunkN ][ tagN ][ tag ] All chunks are the same size except for the last chunk, which may be shorter. In `Decryptor::read_helper`, we read a chunk and a tag worth of data at a time. 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. - Fixes #345 --- openpgp/src/crypto/aead.rs | 112 ++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 105 insertions(+), 7 deletions(-) (limited to 'openpgp/src/crypto/aead.rs') diff --git a/openpgp/src/crypto/aead.rs b/openpgp/src/crypto/aead.rs index 5be5ebd0..03c02da2 100644 --- a/openpgp/src/crypto/aead.rs +++ b/openpgp/src/crypto/aead.rs @@ -17,6 +17,69 @@ use crate::Result; use crate::crypto::SessionKey; use crate::crypto::mem::secure_cmp; +// A helper to allow us to determine whether a Reader has reached EOF. +struct Peekable where R: io::Read { + data: Option, + inner: R, +} + +impl Peekable where R: io::Read { + fn new(inner: R) -> Self { + Peekable { + data: None, + inner: inner, + } + } + + // Returns the next byte in the file without consuming that byte. + // + // That is, the next call to read will still return that byte. + // Returns None if there is no data left to read. + fn peek(&mut self) -> Option { + if let Some(data) = self.data { + Some(data) + } else { + let mut buffer = Vec::new(); + match (&mut self.inner).take(1).read_to_end(&mut buffer) { + Ok(0) => None, // EOF + Ok(1) => { + self.data = Some(buffer[0]); + self.data + } + Ok(_) => unreachable!(), + Err(_) => None, + } + } + } + + // Returns whether the end of file has been reached. + fn eof(&mut self) -> bool { + self.peek().is_none() + } +} + +impl io::Read for Peekable where R: io::Read { + fn read(&mut self, buf: &mut [u8]) -> io::Result { + if buf.len() == 0 { + // Zero length read. + return Ok(0); + } + + let start = if let Some(data) = self.data.take() { + buf[0] = data; + 1 + } else { + 0 + }; + + match self.inner.read(&mut buf[start..]) { + Ok(n) => Ok(start + n), + Err(_) if start > 0 => Ok(start), + Err(e) => Err(e), + } + } +} + impl AEADAlgorithm { /// Returns the digest size of the AEAD algorithm. pub fn digest_size(&self) -> Result { @@ -79,7 +142,7 @@ const AD_PREFIX_LEN: usize = 5; /// A `Read`er for decrypting AEAD-encrypted data. pub struct Decryptor { // The encrypted data. - source: R, + source: Peekable, sym_algo: SymmetricAlgorithm, aead: AEADAlgorithm, @@ -103,7 +166,7 @@ impl Decryptor { chunk_size: usize, iv: &[u8], key: &SessionKey, source: R) -> Result { Ok(Decryptor { - source: source, + source: Peekable::new(source), sym_algo: sym_algo, aead: aead, key: key.clone(), @@ -211,7 +274,25 @@ impl Decryptor { return Ok(pos); } - short_read = amount < to_copy; + // 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); }, @@ -290,7 +371,24 @@ impl Decryptor { return Ok(pos); } - short_read = amount < chunk_digest_size; + // 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. @@ -480,16 +578,16 @@ impl, C> BufferedReader } fn get_mut(&mut self) -> Option<&mut dyn BufferedReader> { - Some(&mut self.reader.reader.source) + Some(&mut self.reader.reader.source.inner) } fn get_ref(&self) -> Option<&dyn BufferedReader> { - Some(&self.reader.reader.source) + Some(&self.reader.reader.source.inner) } fn into_inner<'b>(self: Box) -> Option + 'b>> where Self: 'b { - Some(Box::new(self.reader.reader.source)) + Some(Box::new(self.reader.reader.source.inner)) } fn cookie_set(&mut self, cookie: C) -> C { -- cgit v1.2.3