From 7ef416aaa3ca55db9659d59cb63c8ed2a06be0b9 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Mon, 20 Feb 2023 03:51:25 +0000 Subject: [PATCH] age: Add `Decryptor::new_buffered` This is significantly more efficient than `Decryptor::new` at parsing headers, due to avoiding repeated short reads. --- age/CHANGELOG.md | 2 + age/benches/parser.rs | 2 +- age/benches/throughput.rs | 2 +- age/src/format.rs | 30 +++++- age/src/protocol.rs | 31 +++++- age/tests/testkit.rs | 139 ++++++++++++++++++++++++++ fuzz/Cargo.toml | 6 ++ fuzz/fuzz_targets/decrypt_buffered.rs | 18 ++++ 8 files changed, 226 insertions(+), 4 deletions(-) create mode 100644 fuzz/fuzz_targets/decrypt_buffered.rs diff --git a/age/CHANGELOG.md b/age/CHANGELOG.md index 8bdb264f..f77ee549 100644 --- a/age/CHANGELOG.md +++ b/age/CHANGELOG.md @@ -10,6 +10,8 @@ to 1.0.0 are beta releases. ## [Unreleased] ### Added +- `age::Decryptor::new_buffered`, which is more efficient for types implementing + `std::io::BufRead` (which includes `&[u8]` slices). - `impl std::io::BufRead for age::armor::ArmoredReader` ## [0.9.1] - 2022-03-24 diff --git a/age/benches/parser.rs b/age/benches/parser.rs index 6384351e..e67fb579 100644 --- a/age/benches/parser.rs +++ b/age/benches/parser.rs @@ -30,7 +30,7 @@ fn bench(c: &mut Criterion) { output.write_all(&[]).unwrap(); output.finish().unwrap(); - b.iter(|| Decryptor::new(&encrypted[..])) + b.iter(|| Decryptor::new_buffered(&encrypted[..])) }); } diff --git a/age/benches/throughput.rs b/age/benches/throughput.rs index 2e7e4483..7a3c8e5f 100644 --- a/age/benches/throughput.rs +++ b/age/benches/throughput.rs @@ -70,7 +70,7 @@ fn bench(c: &mut Criterion_) { output.finish().unwrap(); b.iter(|| { - let decryptor = match Decryptor::new(&ct_buf[..]).unwrap() { + let decryptor = match Decryptor::new_buffered(&ct_buf[..]).unwrap() { Decryptor::Recipients(decryptor) => decryptor, _ => panic!(), }; diff --git a/age/src/format.rs b/age/src/format.rs index 7bd499f4..b4759138 100644 --- a/age/src/format.rs +++ b/age/src/format.rs @@ -1,7 +1,7 @@ //! The age file format. use age_core::format::Stanza; -use std::io::{self, Read, Write}; +use std::io::{self, BufRead, Read, Write}; use crate::{ error::DecryptError, @@ -88,6 +88,34 @@ impl Header { } } + pub(crate) fn read_buffered(mut input: R) -> Result { + let mut data = vec![]; + loop { + match read::header(&data) { + Ok((_, mut header)) => { + if let Header::V1(h) = &mut header { + h.encoded_bytes = Some(data); + } + break Ok(header); + } + Err(nom::Err::Incomplete(nom::Needed::Size(_))) => { + // As we have a buffered reader, we can leverage the fact that the + // currently-defined header formats are newline-separated, to more + // efficiently read data for the parser to consume. + if input.read_until(b'\n', &mut data)? == 0 { + break Err(DecryptError::Io(io::Error::new( + io::ErrorKind::UnexpectedEof, + "Incomplete header", + ))); + } + } + Err(_) => { + break Err(DecryptError::InvalidHeader); + } + } + } + } + #[cfg(feature = "async")] #[cfg_attr(docsrs, doc(cfg(feature = "async")))] pub(crate) async fn read_async( diff --git a/age/src/protocol.rs b/age/src/protocol.rs index b5229df5..6c6df056 100644 --- a/age/src/protocol.rs +++ b/age/src/protocol.rs @@ -2,7 +2,7 @@ use age_core::{format::grease_the_joint, secrecy::SecretString}; use rand::{rngs::OsRng, RngCore}; -use std::io::{self, Read, Write}; +use std::io::{self, BufRead, Read, Write}; use crate::{ error::{DecryptError, EncryptError}, @@ -181,6 +181,13 @@ impl Decryptor { /// Attempts to create a decryptor for an age file. /// /// Returns an error if the input does not contain a valid age file. + /// + /// # Performance + /// + /// This constructor will work with any type implementing [`io::Read`], and uses a + /// slower parser and internal buffering to ensure no overreading occurs. Consider + /// using [`Decryptor::new_buffered`] for types implementing `std::io::BufRead`, which + /// includes `&[u8]` slices. pub fn new(mut input: R) -> Result { let header = Header::read(&mut input)?; @@ -194,6 +201,28 @@ impl Decryptor { } } +impl Decryptor { + /// Attempts to create a decryptor for an age file. + /// + /// Returns an error if the input does not contain a valid age file. + /// + /// # Performance + /// + /// This constructor is more performant than [`Decryptor::new`] for types implementing + /// [`io::BufRead`], which includes `&[u8]` slices. + pub fn new_buffered(mut input: R) -> Result { + let header = Header::read_buffered(&mut input)?; + + match header { + Header::V1(v1_header) => { + let nonce = Nonce::read(&mut input)?; + Decryptor::from_v1_header(input, v1_header, nonce) + } + Header::Unknown(_) => Err(DecryptError::UnknownFormat), + } + } +} + #[cfg(feature = "async")] #[cfg_attr(docsrs, doc(cfg(feature = "async")))] impl Decryptor { diff --git a/age/tests/testkit.rs b/age/tests/testkit.rs index 66e0df6c..afb85edd 100644 --- a/age/tests/testkit.rs +++ b/age/tests/testkit.rs @@ -264,6 +264,145 @@ fn testkit(filename: &str) { #[test_case("x25519_not_canonical_body")] #[test_case("x25519_not_canonical_share")] #[test_case("x25519_short_share")] +fn testkit_buffered(filename: &str) { + let testfile = TestFile::parse(filename); + let comment = format_testkit_comment(&testfile); + + match Decryptor::new_buffered(ArmoredReader::new(&testfile.age_file[..])).and_then( + |d| match d { + Decryptor::Recipients(d) => { + let identities = get_testkit_identities(filename, &testfile); + d.decrypt(identities.iter().map(|i| i as &dyn Identity)) + } + Decryptor::Passphrase(d) => { + let passphrase = get_testkit_passphrase(&testfile, &comment); + d.decrypt(&passphrase, Some(16)) + } + }, + ) { + Ok(mut r) => { + let mut payload = vec![]; + let res = io::Read::read_to_end(&mut r, &mut payload); + check_decrypt_success(filename, testfile, &comment, res, &payload); + } + Err(e) => check_decrypt_error(filename, testfile, e), + } +} + +#[test_case("armor")] +#[test_case("armor_crlf")] +#[test_case("armor_empty_line_begin")] +#[test_case("armor_empty_line_end")] +#[test_case("armor_eol_between_padding")] +#[test_case("armor_full_last_line")] +#[test_case("armor_garbage_encoded")] +#[test_case("armor_garbage_leading")] +#[test_case("armor_garbage_trailing")] +#[test_case("armor_header_crlf")] +#[test_case("armor_headers")] +#[test_case("armor_invalid_character_header")] +#[test_case("armor_invalid_character_payload")] +#[test_case("armor_long_line")] +#[test_case("armor_lowercase")] +#[test_case("armor_no_end_line")] +#[test_case("armor_no_eol")] +#[test_case("armor_no_match")] +#[test_case("armor_no_padding")] +#[test_case("armor_not_canonical")] +#[test_case("armor_pgp_checksum")] +#[test_case("armor_short_line")] +#[test_case("armor_whitespace_begin")] +#[test_case("armor_whitespace_end")] +#[test_case("armor_whitespace_eol")] +#[test_case("armor_whitespace_last_line")] +#[test_case("armor_whitespace_line_start")] +#[test_case("armor_whitespace_outside")] +#[test_case("armor_wrong_type")] +#[test_case("header_crlf")] +#[test_case("hmac_bad")] +#[test_case("hmac_extra_space")] +#[test_case("hmac_garbage")] +#[test_case("hmac_missing")] +#[test_case("hmac_no_space")] +#[test_case("hmac_not_canonical")] +#[test_case("hmac_trailing_space")] +#[test_case("hmac_truncated")] +#[test_case("scrypt")] +#[test_case("scrypt_and_x25519")] +#[test_case("scrypt_bad_tag")] +#[test_case("scrypt_double")] +#[test_case("scrypt_extra_argument")] +#[test_case("scrypt_long_file_key")] +#[test_case("scrypt_no_match")] +#[test_case("scrypt_not_canonical_body")] +#[test_case("scrypt_not_canonical_salt")] +#[test_case("scrypt_salt_long")] +#[test_case("scrypt_salt_missing")] +#[test_case("scrypt_salt_short")] +#[test_case("scrypt_uppercase")] +#[test_case("scrypt_work_factor_23")] +#[test_case("scrypt_work_factor_hex")] +#[test_case("scrypt_work_factor_leading_garbage")] +#[test_case("scrypt_work_factor_leading_plus")] +#[test_case("scrypt_work_factor_leading_zero_decimal")] +#[test_case("scrypt_work_factor_leading_zero_octal")] +#[test_case("scrypt_work_factor_missing")] +#[test_case("scrypt_work_factor_negative")] +#[test_case("scrypt_work_factor_overflow")] +#[test_case("scrypt_work_factor_trailing_garbage")] +#[test_case("scrypt_work_factor_wrong")] +#[test_case("scrypt_work_factor_zero")] +#[test_case("stanza_bad_start")] +#[test_case("stanza_base64_padding")] +#[test_case("stanza_empty_argument")] +#[test_case("stanza_empty_body")] +#[test_case("stanza_empty_last_line")] +#[test_case("stanza_invalid_character")] +#[test_case("stanza_long_line")] +#[test_case("stanza_missing_body")] +#[test_case("stanza_missing_final_line")] +#[test_case("stanza_multiple_short_lines")] +#[test_case("stanza_no_arguments")] +#[test_case("stanza_not_canonical")] +#[test_case("stanza_spurious_cr")] +#[test_case("stanza_valid_characters")] +#[test_case("stream_bad_tag")] +#[test_case("stream_bad_tag_second_chunk")] +#[test_case("stream_bad_tag_second_chunk_full")] +#[test_case("stream_empty_payload")] +#[test_case("stream_last_chunk_empty")] +#[test_case("stream_last_chunk_full")] +#[test_case("stream_last_chunk_full_second")] +#[test_case("stream_missing_tag")] +#[test_case("stream_no_chunks")] +#[test_case("stream_no_final")] +#[test_case("stream_no_final_full")] +#[test_case("stream_no_final_two_chunks")] +#[test_case("stream_no_final_two_chunks_full")] +#[test_case("stream_no_nonce")] +#[test_case("stream_short_chunk")] +#[test_case("stream_short_nonce")] +#[test_case("stream_short_second_chunk")] +#[test_case("stream_three_chunks")] +#[test_case("stream_trailing_garbage_long")] +#[test_case("stream_trailing_garbage_short")] +#[test_case("stream_two_chunks")] +#[test_case("stream_two_final_chunks")] +#[test_case("version_unsupported")] +#[test_case("x25519")] +#[test_case("x25519_bad_tag")] +#[test_case("x25519_extra_argument")] +#[test_case("x25519_grease")] +#[test_case("x25519_identity")] +#[test_case("x25519_long_file_key")] +#[test_case("x25519_long_share")] +#[test_case("x25519_lowercase")] +#[test_case("x25519_low_order")] +#[test_case("x25519_multiple_recipients")] +#[test_case("x25519_no_match")] +#[test_case("x25519_not_canonical_body")] +#[test_case("x25519_not_canonical_share")] +#[test_case("x25519_short_share")] #[tokio::test] async fn testkit_async(filename: &str) { let testfile = TestFile::parse(filename); diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml index 00496938..c1709665 100644 --- a/fuzz/Cargo.toml +++ b/fuzz/Cargo.toml @@ -34,3 +34,9 @@ path = "fuzz_targets/header.rs" [[bin]] name = "decrypt" path = "fuzz_targets/decrypt.rs" + +[[bin]] +name = "decrypt_buffered" +path = "fuzz_targets/decrypt_buffered.rs" +test = false +doc = false diff --git a/fuzz/fuzz_targets/decrypt_buffered.rs b/fuzz/fuzz_targets/decrypt_buffered.rs new file mode 100644 index 00000000..553525e8 --- /dev/null +++ b/fuzz/fuzz_targets/decrypt_buffered.rs @@ -0,0 +1,18 @@ +#![no_main] +use libfuzzer_sys::fuzz_target; + +use std::iter; + +use age::Decryptor; + +fuzz_target!(|data: &[u8]| { + if let Ok(decryptor) = Decryptor::new_buffered(data) { + match decryptor { + Decryptor::Recipients(d) => { + let _ = d.decrypt(iter::empty()); + } + // Don't pay the cost of scrypt while fuzzing. + Decryptor::Passphrase(_) => (), + } + } +});