StreamReader doesn't authenticate the end of the ciphertext when using SeekFrom::End

[oconnor663]

Seeks using SeekFrom::End need to know the total length of the plaintext to compute the target offset. To compute the plaintext length, StreamReader first calls self.inner.seek(SeekFrom::End(0)) to get the ciphertext length. This raises a security issue: The inner reader represents untrusted ciphertext, which could have been truncated or extended in transit. That means that the ciphertext EOF offset returned by inner.seek might not be the authentic ciphertext length that the sender intended, and using it to compute the plaintext length allows an attacker to control the target offset of the seek.

To prevent this attack, StreamReader needs to authenticate the ciphertext EOF, by decrypting and authenticating the final chunk (even if the caller's intended offset is in some earlier chunk). If an attacker has truncated or extended the ciphertext, the final chunk will fail to authenticate. Once the final chunk has been authenticated, StreamReader can compute the caller's intended plaintext offset (and probably cache the authentic plaintext length).

Here's a demonstration of this attack:

use std::io::prelude::*;
use std::io::SeekFrom;

fn main() -> Result<(), Box<dyn std::error::Error>> {
    // The plaintext is the string "hello" followed by 65536 zeros, just enough to give us some
    // bytes to play with in the second chunk.
    let mut plaintext: Vec<u8> = b"hello".to_vec();
    plaintext.extend_from_slice(&[0; 65536]);

    // Encrypt the plaintext just like the example code in the docs.
    let key = age::x25519::Identity::generate();
    let pubkey = key.to_public();
    let encryptor = age::Encryptor::with_recipients(vec![Box::new(pubkey)]);
    let mut encrypted = vec![];
    let mut writer = encryptor.wrap_output(&mut encrypted)?;
    writer.write_all(&plaintext)?;
    writer.finish()?;

    // First check the correct behavior of seeks relative to EOF. Create a decrypting reader, and
    // move it one byte forward from the start, using SeekFrom::End. Confirm that reading 4 bytes
    // from that point gives us "ello", as it should.
    let cursor = std::io::Cursor::new(&encrypted[..]);
    let decryptor = match age::Decryptor::new(cursor)? {
        age::Decryptor::Recipients(d) => d,
        _ => unreachable!(),
    };
    let mut reader = decryptor.decrypt(std::iter::once(
        Box::new(key.clone()) as Box<dyn age::Identity>
    ))?;
    let eof_relative_offset = 1 as i64 - plaintext.len() as i64;
    reader.seek(SeekFrom::End(eof_relative_offset))?;
    let mut buf = [0; 4];
    reader.read_exact(&mut buf)?;
    assert_eq!(&buf, b"ello", "This is correct.");

    // BUG: Do the same thing again, except this time truncate the ciphertext by one byte first.
    // This should cause some sort of error, but instead it's a successful read that returns the
    // wrong plaintext.
    let truncated_ciphertext = &encrypted[..encrypted.len() - 1];
    let truncated_cursor = std::io::Cursor::new(&truncated_ciphertext[..]);
    let truncated_decryptor = match age::Decryptor::new(truncated_cursor)? {
        age::Decryptor::Recipients(d) => d,
        _ => unreachable!(),
    };
    let mut truncated_reader = truncated_decryptor.decrypt(std::iter::once(
        Box::new(key.clone()) as Box<dyn age::Identity>,
    ))?;
    // Use the same seek target as above.
    truncated_reader.seek(SeekFrom::End(eof_relative_offset))?;
    let mut truncated_buf = [0; 4];
    truncated_reader.read_exact(&mut truncated_buf)?;
    assert_eq!(&truncated_buf, b"hell", "This is a security issue.");

    Ok(())
}

[str4d]

Ooh, this is a great catch, thanks! I'll dig into it more later today.

[str4d]

Fixed in #197.