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).

1 files changed, 159 insertions, 193 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;
             }
         }