Merge pull request #281 from str4d/fuzzer-fixes

Fix panic in `AgeStanza::body`
str4d · Dec 27, 2021 · c950780 · c950780
2 parents 5207877 + 7a2ff3a
commit c950780
Show file tree

Hide file tree

Showing 14 changed files with 206 additions and 23 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/age-core/CHANGELOG.md b/age-core/CHANGELOG.md
@@ -8,6 +8,16 @@ to 1.0.0 are beta releases.
 
 ## [Unreleased]
 
+## [0.7.1] - 2021-12-27
+### Fixed
+- In 0.7.0, Base64 decoding was moved to the `AgeStanza::body` method, with the
+  stanza parser only checking for valid Base64 characters. This caused the
+  parser to start accepting stanzas with non-canonical last body lines (where
+  the Base64 encoding would have trailing bits that could not be decoded into
+  full bytes); calling `AgeStanza::body` on these stanzas would cause a panic.
+  This release fixes the parser to reject non-canonical last body lines, turning
+  the panic back into an error.
+
 ## [0.7.0] - 2021-10-18
 ### Added
 - `age_core::secrecy`, which re-exports the `secrecy` crate.

diff --git a/age-core/Cargo.toml b/age-core/Cargo.toml
@@ -1,7 +1,7 @@
 [package]
 name = "age-core"
 description = "[BETA] Common functions used across the age crates"
-version = "0.7.0"
+version = "0.7.1"
 authors = ["Jack Grigg <thestr4d@gmail.com>"]
 repository = "https://github.com/str4d/rage"
 readme = "README.md"

diff --git a/age-core/src/format.rs b/age-core/src/format.rs
@@ -134,7 +134,7 @@ pub mod read {
         branch::alt,
         bytes::streaming::{tag, take_while1, take_while_m_n},
         character::streaming::newline,
-        combinator::{map, map_opt, opt},
+        combinator::{map, map_opt, opt, verify},
         multi::{many_till, separated_list1},
         sequence::{pair, preceded, terminated},
         IResult,
@@ -151,6 +151,30 @@ pub mod read {
         )
     }
 
+    /// Returns true if the byte is one of the specific ASCII values of the standard
+    /// Base64 character set which leave trailing bits when they occur as the last
+    /// character in an encoding of length 2 mod 4.
+    fn base64_has_no_trailing_bits_2(c: &u8) -> bool {
+        // With two trailing characters, the last character has up to four trailing bits.
+        matches!(
+            c,
+            // A | Q | g | w
+            65 | 81 | 103 | 119,
+        )
+    }
+
+    /// Returns true if the byte is one of the specific ASCII values of the standard
+    /// Base64 character set which leave trailing bits when they occur as the last
+    /// character in an encoding of length 3 mod 4.
+    fn base64_has_no_trailing_bits_3(c: &u8) -> bool {
+        // With three trailing characters, the last character has up to two trailing bits.
+        matches!(
+            c,
+            // A | E | I | M | Q | U | Y | c | g | k | o | s | w | 0 | 4 | 8
+            65 | 69 | 73 | 77 | 81 | 85 | 89 | 99 | 103 | 107 | 111 | 115 | 119 | 48 | 52 | 56,
+        )
+    }
+
     /// Reads an age "arbitrary string".
     ///
     /// From the age specification:
@@ -169,8 +193,21 @@ pub mod read {
             many_till(
                 // Any body lines before the last MUST be full-length.
                 terminated(take_while_m_n(64, 64, is_base64_char), newline),
-                // Last body line MUST be short (empty if necessary).
-                terminated(take_while_m_n(0, 63, is_base64_char), newline),
+                // Last body line:
+                // - MUST be short (empty if necessary).
+                // - MUST be a valid Base64 length (i.e. the length must not be 1 mod 4).
+                // - MUST NOT leave trailing bits (if the length is 2 or 3 mod 4).
+                verify(
+                    terminated(take_while_m_n(0, 63, is_base64_char), newline),
+                    |line: &[u8]| match line.len() % 4 {
+                        0 => true,
+                        1 => false,
+                        2 => base64_has_no_trailing_bits_2(line.last().unwrap()),
+                        3 => base64_has_no_trailing_bits_3(line.last().unwrap()),
+                        // No other cases, but Rust wants an exhaustive match on u8.
+                        _ => unreachable!(),
+                    },
+                ),
             ),
             |(full_chunks, partial_chunk): (Vec<&[u8]>, &[u8])| {
                 let mut chunks = full_chunks;
@@ -185,9 +222,16 @@ pub mod read {
             separated_list1(newline, take_while1(is_base64_char)),
             |chunks: Vec<&[u8]>| {
                 // Enforce that the only chunk allowed to be shorter than 64 characters
-                // is the last chunk.
+                // is the last chunk, and that its length must not be 1 mod 4.
                 let (partial_chunk, full_chunks) = chunks.split_last().unwrap();
-                if full_chunks.iter().any(|s| s.len() != 64) || partial_chunk.len() > 64 {
+                if full_chunks.iter().any(|s| s.len() != 64)
+                    || partial_chunk.len() > 64
+                    || partial_chunk.len() % 4 == 1
+                    || (partial_chunk.len() % 4 == 2
+                        && !base64_has_no_trailing_bits_2(partial_chunk.last().unwrap()))
+                    || (partial_chunk.len() % 4 == 3
+                        && !base64_has_no_trailing_bits_3(partial_chunk.last().unwrap()))
+                {
                     None
                 } else {
                     Some(chunks)
@@ -333,6 +377,8 @@ pub mod write {
 
 #[cfg(test)]
 mod tests {
+    use nom::error::ErrorKind;
+
     use super::{read, write};
 
     #[test]
@@ -435,4 +481,114 @@ xD7o4VEOu1t7KZQ1gDgq2FPzBEeSRqbnqvQEXdLRYy143BxR6oFxsUUJCRB0ErXA
         assert_eq!(stanza.args, test_args);
         assert_eq!(stanza.body(), test_body);
     }
+
+    #[test]
+    fn age_stanza_invalid_last_line() {
+        // Artifact found by cargo-fuzz on commit 81f91581bf7e21075519dc23e4a28b4d201dd784
+        // We add an extra newline to the artifact so that we would "correctly" trigger
+        // the bug in the legacy part of `read::legacy_age_stanza`.
+        let artifact = "-> H
+/
+
+";
+
+        // The stanza parser requires the last body line is short (possibly empty), so
+        // should reject this artifact.
+        match read::age_stanza(artifact.as_bytes()) {
+            Err(nom::Err::Error(e)) => assert_eq!(e.code, ErrorKind::TakeWhileMN),
+            Err(e) => panic!("Unexpected error: {}", e),
+            Ok((rest, stanza)) => {
+                assert_eq!(rest, b"\n");
+                // This is where the fuzzer triggered a panic.
+                let _ = stanza.body();
+                // We should never reach here either before or after the bug was fixed,
+                // because the body length is invalid.
+                panic!("Invalid test case was parsed without error");
+            }
+        }
+
+        // The legacy parser accepts this artifact by ignoring the invalid body line,
+        // because bodies were allowed to be omitted.
+        let (rest, stanza) = read::legacy_age_stanza(artifact.as_bytes()).unwrap();
+        // The remainder should the invalid body line. If the standard parser were fixed
+        // but the legacy parser was not, this would only contain a single newline.
+        assert_eq!(rest, b"/\n\n");
+        // This is where the fuzzer would have triggered a panic if it were using the
+        // legacy parser.
+        let body = stanza.body();
+        assert!(body.is_empty());
+    }
+
+    #[test]
+    fn age_stanza_last_line_two_trailing_chars() {
+        // Artifact found by cargo-fuzz on commit 8da15148fc005a48ffeb43eb76dab478eb2fdf72
+        // We add an extra newline to the artifact so that we would "correctly" trigger
+        // the bug in the legacy part of `read::legacy_age_stanza`.
+        let artifact = "-> '
+dy
+
+";
+
+        // The stanza parser requires the last body line is short (possibly empty), so
+        // should reject this artifact.
+        match read::age_stanza(artifact.as_bytes()) {
+            Err(nom::Err::Error(e)) => assert_eq!(e.code, ErrorKind::TakeWhileMN),
+            Err(e) => panic!("Unexpected error: {}", e),
+            Ok((rest, stanza)) => {
+                assert_eq!(rest, b"\n");
+                // This is where the fuzzer triggered a panic.
+                let _ = stanza.body();
+                // We should never reach here either before or after the bug was fixed,
+                // because the last body line has trailing bits.
+                panic!("Invalid test case was parsed without error");
+            }
+        }
+
+        // The legacy parser accepts this artifact by ignoring the invalid body line,
+        // because bodies were allowed to be omitted.
+        let (rest, stanza) = read::legacy_age_stanza(artifact.as_bytes()).unwrap();
+        // The remainder should the invalid body line. If the standard parser were fixed
+        // but the legacy parser was not, this would only contain a single newline.
+        assert_eq!(rest, b"dy\n\n");
+        // This is where the fuzzer would have triggered a panic if it were using the
+        // legacy parser.
+        let body = stanza.body();
+        assert!(body.is_empty());
+    }
+
+    #[test]
+    fn age_stanza_last_line_three_trailing_chars() {
+        // Artifact found by cargo-fuzz after age_stanza_last_line_two_trailing_chars was
+        // incorrectly fixed.
+        let artifact = "-> h
+ddd
+
+";
+
+        // The stanza parser requires the last body line is short (possibly empty), so
+        // should reject this artifact.
+        match read::age_stanza(artifact.as_bytes()) {
+            Err(nom::Err::Error(e)) => assert_eq!(e.code, ErrorKind::TakeWhileMN),
+            Err(e) => panic!("Unexpected error: {}", e),
+            Ok((rest, stanza)) => {
+                assert_eq!(rest, b"\n");
+                // This is where the fuzzer triggered a panic.
+                let _ = stanza.body();
+                // We should never reach here either before or after the bug was fixed,
+                // because the last body line has trailing bits.
+                panic!("Invalid test case was parsed without error");
+            }
+        }
+
+        // The legacy parser accepts this artifact by ignoring the invalid body line,
+        // because bodies were allowed to be omitted.
+        let (rest, stanza) = read::legacy_age_stanza(artifact.as_bytes()).unwrap();
+        // The remainder should the invalid body line. If the standard parser were fixed
+        // but the legacy parser was not, this would only contain a single newline.
+        assert_eq!(rest, b"ddd\n\n");
+        // This is where the fuzzer would have triggered a panic if it were using the
+        // legacy parser.
+        let body = stanza.body();
+        assert!(body.is_empty());
+    }
 }
diff --git a/age-plugin/CHANGELOG.md b/age-plugin/CHANGELOG.md
@@ -10,6 +10,11 @@ to 1.0.0 are beta releases.
 
 ## [Unreleased]
 
+## [0.2.1] - 2021-12-27
+### Fixed
+- Bumped `age-core` to 0.7.1 to fix a bug where non-canonical recipient stanza
+  bodies in an age file header would cause a panic instead of being rejected.
+
 ## [0.2.0] - 2021-10-18
 ### Changed
 - MSRV is now 1.51.0.

diff --git a/age-plugin/Cargo.toml b/age-plugin/Cargo.toml
@@ -1,15 +1,15 @@
 [package]
 name = "age-plugin"
 description = "[BETA] API for writing age plugins."
-version = "0.2.0"
+version = "0.2.1"
 authors = ["Jack Grigg <thestr4d@gmail.com>"]
 repository = "https://github.com/str4d/rage"
 readme = "README.md"
 license = "MIT OR Apache-2.0"
 edition = "2018"
 
 [dependencies]
-age-core = { version = "0.7.0", path = "../age-core", features = ["plugin"] }
+age-core = { version = "0.7.1", path = "../age-core", features = ["plugin"] }
 bech32 = "0.8"
 chrono = "0.4"
 

diff --git a/age/CHANGELOG.md b/age/CHANGELOG.md
@@ -10,6 +10,11 @@ to 1.0.0 are beta releases.
 
 ## [Unreleased]
 
+## [0.7.1] - 2021-12-27
+### Fixed
+- Bumped `age-core` to 0.7.1 to fix a bug where non-canonical recipient stanza
+  bodies in an age file header would cause a panic instead of being rejected.
+
 ## [0.7.0] - 2021-10-18
 ### Added
 - `age::encrypted::Identity`, for decrypting files with passphrase-encrypted

diff --git a/age/Cargo.toml b/age/Cargo.toml
@@ -1,7 +1,7 @@
 [package]
 name = "age"
 description = "[BETA] A simple, secure, and modern encryption library."
-version = "0.7.0"
+version = "0.7.1"
 authors = ["Jack Grigg <thestr4d@gmail.com>"]
 repository = "https://github.com/str4d/rage"
 readme = "README.md"
@@ -14,7 +14,7 @@ edition = "2018"
 maintenance = { status = "experimental" }
 
 [dependencies]
-age-core = { version = "0.7.0", path = "../age-core" }
+age-core = { version = "0.7.1", path = "../age-core" }
 
 # Dependencies required by the age specification:
 # - Base64 from RFC 4648

diff --git a/fuzz-afl/Cargo.lock b/fuzz-afl/Cargo.lock
diff --git a/fuzz/Cargo.lock b/fuzz/Cargo.lock
diff --git a/fuzz/fuzz_targets/age_stanza.rs b/fuzz/fuzz_targets/age_stanza.rs
@@ -7,7 +7,7 @@ fuzz_target!(|data: &[u8]| {
     if let Ok((leftover, stanza)) = read::age_stanza(data) {
         let mut buf = Vec::with_capacity(data.len());
         gen(
-            write::age_stanza(stanza.tag, &stanza.args, &stanza.body),
+            write::age_stanza(stanza.tag, &stanza.args, &stanza.body()),
             &mut buf,
         )
         .expect("can write to Vec");

diff --git a/fuzz/fuzz_targets/decrypt.rs b/fuzz/fuzz_targets/decrypt.rs
@@ -1,16 +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(data) {
         match decryptor {
             Decryptor::Recipients(d) => {
-                let _ = d.decrypt(&[]);
+                let _ = d.decrypt(iter::empty());
             }
             // Don't pay the cost of scrypt while fuzzing.
-            Decryptor::Passphrase(_) => ()
+            Decryptor::Passphrase(_) => (),
         }
     }
 });