fix: correctly deal with multi buffer reads in dearmoring

dignifiedquire · hko-s · web-flow · commit 7637f18e7827 · 2025-01-04T15:01:08.000+01:00
Based on #428 Closes #352 --------- Co-authored-by: Heiko Schaefer <heiko@schaefer.name>
diff --git a/src/armor/reader.rs b/src/armor/reader.rs
@@ -328,9 +328,11 @@ pub struct Dearmor<R: BufRead> {
     pub headers: Headers,
     /// Optional crc checksum
     pub checksum: Option<u64>,
-    /// current state
+    /// Current state
     current_part: Part<R>,
     crc: crc24::Crc24Hasher,
+    /// Maximum buffer limit
+    max_buffer_limit: usize,
 }
 
 /// Internal indicator, where in the parsing phase we are
@@ -345,13 +347,20 @@ enum Part<R: BufRead> {
 }
 
 impl<R: BufRead> Dearmor<R> {
+    /// Creates a new `Dearmor`, with the default limit of 1GiB.
     pub fn new(input: R) -> Self {
+        Self::with_limit(input, 1024 * 1024 * 1024)
+    }
+
+    /// Creates a new `Dearmor` with the provided maximum buffer size.
+    pub fn with_limit(input: R, limit: usize) -> Self {
         Dearmor {
             typ: None,
             headers: BTreeMap::new(),
             checksum: None,
             current_part: Part::Header(input),
             crc: Default::default(),
+            max_buffer_limit: limit,
         }
     }
 
@@ -370,30 +379,38 @@ impl<R: BufRead> Dearmor<R> {
         (typ, headers, checksum, b)
     }
 
+    /// The current maximum buffer limit.
+    pub fn max_buffer_limit(&self) -> usize {
+        self.max_buffer_limit
+    }
+
     pub fn read_only_header(mut self) -> Result<(BlockType, Headers, bool, R)> {
         let header = std::mem::replace(&mut self.current_part, Part::Temp);
         if let Part::Header(mut b) = header {
-            let (typ, headers, leading) = Self::read_header_internal(&mut b)?;
+            let (typ, headers, leading) =
+                Self::read_header_internal(&mut b, self.max_buffer_limit)?;
             return Ok((typ, headers, leading, b));
         }
 
         bail!("invalid state, cannot read header");
     }
 
-    pub fn after_header(typ: BlockType, headers: Headers, input: R) -> Self {
+    pub fn after_header(typ: BlockType, headers: Headers, input: R, limit: usize) -> Self {
         Self {
             typ: Some(typ),
             headers,
             checksum: None,
             current_part: Part::Body(Base64Decoder::new(Base64Reader::new(input))),
             crc: Default::default(),
+            max_buffer_limit: limit,
         }
     }
 
     pub fn read_header(&mut self) -> Result<()> {
         let header = std::mem::replace(&mut self.current_part, Part::Temp);
         if let Part::Header(mut b) = header {
-            let (typ, headers, _has_leading_data) = Self::read_header_internal(&mut b)?;
+            let (typ, headers, _has_leading_data) =
+                Self::read_header_internal(&mut b, self.max_buffer_limit)?;
             self.typ = Some(typ);
             self.headers = headers;
             self.current_part = Part::Body(Base64Decoder::new(Base64Reader::new(b)));
@@ -403,8 +420,8 @@ impl<R: BufRead> Dearmor<R> {
         bail!("invalid state, cannot read header");
     }
 
-    fn read_header_internal(b: &mut R) -> Result<(BlockType, Headers, bool)> {
-        let (typ, headers, leading) = read_from_buf(b, "armor header", header_parser)?;
+    fn read_header_internal(b: &mut R, limit: usize) -> Result<(BlockType, Headers, bool)> {
+        let (typ, headers, leading) = read_from_buf(b, "armor header", limit, header_parser)?;
         Ok((typ, headers, leading))
     }
 
@@ -423,7 +440,8 @@ impl<R: BufRead> Dearmor<R> {
     }
 
     fn read_footer(&mut self, mut b: BufReader<R>) -> Result<()> {
-        let (checksum, footer_typ) = read_from_buf(&mut b, "armor footer", footer_parser)?;
+        let (checksum, footer_typ) =
+            read_from_buf(&mut b, "armor footer", self.max_buffer_limit, footer_parser)?;
         if let Some(ref header_typ) = self.typ {
             if header_typ != &footer_typ {
                 self.current_part = Part::Done(b);
@@ -456,8 +474,9 @@ impl<R: BufRead> Read for Dearmor<R> {
             let current_part = std::mem::replace(&mut self.current_part, Part::Temp);
             match current_part {
                 Part::Header(mut b) => {
-                    let (typ, headers, _leading) = Self::read_header_internal(&mut b)
-                        .map_err(|e| io::Error::new(io::ErrorKind::Other, e.to_string()))?;
+                    let (typ, headers, _leading) =
+                        Self::read_header_internal(&mut b, self.max_buffer_limit)
+                            .map_err(|e| io::Error::new(io::ErrorKind::Other, e.to_string()))?;
                     self.typ = Some(typ);
                     self.headers = headers;
                     self.current_part = Part::Body(Base64Decoder::new(Base64Reader::new(b)));
@@ -499,28 +518,60 @@ impl<R: BufRead> Read for Dearmor<R> {
     }
 }
 
-pub(crate) fn read_from_buf<B: BufRead, T, P: Fn(&[u8]) -> IResult<&[u8], T>>(
+pub(crate) fn read_from_buf<B: BufRead, T, P>(
     b: &mut B,
     ctx: &str,
+    limit: usize,
     parser: P,
-) -> Result<T> {
-    let mut last_was_incomplete = false;
+) -> Result<T>
+where
+    P: Fn(&[u8]) -> IResult<&[u8], T>,
+{
+    // Zero copy, single buffer
+    let buf = b.fill_buf()?;
+    if buf.is_empty() {
+        bail!("not enough bytes in buffer: {}", ctx);
+    }
+    match parser(buf) {
+        Ok((remaining, res)) => {
+            let consumed = buf.len() - remaining.len();
+            b.consume(consumed);
+            return Ok(res);
+        }
+        Err(nom::Err::Incomplete(_)) => {}
+        Err(err) => {
+            bail!("failed reading: {} {:?}", ctx, err);
+        }
+    };
+
+    // incomplete
+    let mut back_buffer = buf.to_vec();
+    let len = back_buffer.len();
+    b.consume(len);
+
+    let mut last_buffer_len;
+
     loop {
+        // Safety check to not consume too much
+        if back_buffer.len() >= limit {
+            bail!("input too large");
+        }
+
         let buf = b.fill_buf()?;
         if buf.is_empty() {
             bail!("not enough bytes in buffer: {}", ctx);
         }
-        match parser(buf) {
+        last_buffer_len = buf.len();
+        back_buffer.extend_from_slice(buf);
+
+        match parser(&back_buffer) {
             Ok((remaining, res)) => {
-                let consumed = buf.len() - remaining.len();
+                let consumed = last_buffer_len - remaining.len();
                 b.consume(consumed);
                 return Ok(res);
             }
             Err(nom::Err::Incomplete(_)) => {
-                if last_was_incomplete {
-                    bail!("failed reading: {}: not enough bytes", ctx);
-                }
-                last_was_incomplete = true;
+                b.consume(last_buffer_len);
                 continue;
             }
             Err(err) => {
diff --git a/src/composed/any.rs b/src/composed/any.rs
@@ -33,27 +33,27 @@ impl Any {
     /// Parse armored ascii data.
     pub fn from_armor_buf<R: BufRead>(input: R) -> Result<(Self, armor::Headers)> {
         let dearmor = armor::Dearmor::new(input);
+        let limit = dearmor.max_buffer_limit();
         let (typ, headers, has_leading_data, rest) = dearmor.read_only_header()?;
-
         match typ {
             // Standard PGP types
             BlockType::PublicKey => {
-                let dearmor = Dearmor::after_header(typ, headers.clone(), rest);
+                let dearmor = Dearmor::after_header(typ, headers.clone(), rest, limit);
                 let key = SignedPublicKey::from_bytes(dearmor)?;
                 Ok((Self::PublicKey(key), headers))
             }
             BlockType::PrivateKey => {
-                let dearmor = Dearmor::after_header(typ, headers.clone(), rest);
+                let dearmor = Dearmor::after_header(typ, headers.clone(), rest, limit);
                 let key = SignedSecretKey::from_bytes(dearmor)?;
                 Ok((Self::SecretKey(key), headers))
             }
             BlockType::Message => {
-                let dearmor = Dearmor::after_header(typ, headers.clone(), rest);
+                let dearmor = Dearmor::after_header(typ, headers.clone(), rest, limit);
                 let msg = Message::from_bytes(dearmor)?;
                 Ok((Self::Message(msg), headers))
             }
             BlockType::Signature => {
-                let dearmor = Dearmor::after_header(typ, headers.clone(), rest);
+                let dearmor = Dearmor::after_header(typ, headers.clone(), rest, limit);
                 let sig = StandaloneSignature::from_bytes(dearmor)?;
                 Ok((Self::Signature(sig), headers))
             }
@@ -63,7 +63,7 @@ impl Any {
                     "must not have leading data for a cleartext message"
                 );
                 let (sig, headers) =
-                    CleartextSignedMessage::from_armor_after_header(rest, headers)?;
+                    CleartextSignedMessage::from_armor_after_header(rest, headers, limit)?;
                 Ok((Self::Cleartext(sig), headers))
             }
             _ => unimplemented_err!("unsupported block type: {}", typ),
diff --git a/src/composed/cleartext.rs b/src/composed/cleartext.rs
@@ -6,11 +6,6 @@ use std::io::{BufRead, Read};
 use buffer_redux::BufReader;
 use chrono::SubsecRound;
 use log::debug;
-use nom::branch::alt;
-use nom::bytes::streaming::take_until1;
-use nom::character::streaming::line_ending;
-use nom::combinator::{complete, map_res};
-use nom::IResult;
 
 use crate::armor::{self, header_parser, read_from_buf, BlockType, Headers};
 use crate::crypto::hash::HashAlgorithm;
@@ -19,7 +14,7 @@ use crate::line_writer::LineBreak;
 use crate::normalize_lines::Normalized;
 use crate::packet::{SignatureConfig, SignatureType, Subpacket, SubpacketData};
 use crate::types::{KeyVersion, PublicKeyTrait, SecretKeyTrait};
-use crate::{ArmorOptions, Deserializable, Signature, StandaloneSignature};
+use crate::{ArmorOptions, Deserializable, Signature, StandaloneSignature, MAX_BUFFER_SIZE};
 
 /// Implementation of a Cleartext Signed Message.
 ///
@@ -169,42 +164,44 @@ impl CleartextSignedMessage {
 
     /// Parse from an arbitrary reader, containing the text of the message.
     pub fn from_armor<R: Read>(bytes: R) -> Result<(Self, Headers)> {
-        Self::from_armor_buf(BufReader::new(bytes))
+        Self::from_armor_buf(BufReader::new(bytes), MAX_BUFFER_SIZE)
     }
 
     /// Parse from string, containing the text of the message.
     pub fn from_string(input: &str) -> Result<(Self, Headers)> {
-        Self::from_armor_buf(input.as_bytes())
+        Self::from_armor_buf(input.as_bytes(), MAX_BUFFER_SIZE)
     }
 
     /// Parse from a buffered reader, containing the text of the message.
-    pub fn from_armor_buf<R: BufRead>(mut b: R) -> Result<(Self, Headers)> {
+    pub fn from_armor_buf<R: BufRead>(mut b: R, limit: usize) -> Result<(Self, Headers)> {
         debug!("parsing cleartext message");
         // Headers
         let (typ, headers, has_leading_data) =
-            read_from_buf(&mut b, "cleartext header", header_parser)?;
+            read_from_buf(&mut b, "cleartext header", limit, header_parser)?;
         ensure_eq!(typ, BlockType::CleartextMessage, "unexpected block type");
         ensure!(
             !has_leading_data,
             "must not have leading data for a cleartext message"
         );
 
-        Self::from_armor_after_header(b, headers)
+        Self::from_armor_after_header(b, headers, limit)
     }
 
     pub fn from_armor_after_header<R: BufRead>(
         mut b: R,
         headers: Headers,
+        limit: usize,
     ) -> Result<(Self, Headers)> {
         let hashes = validate_headers(headers)?;
 
         debug!("Found Hash headers: {:?}", hashes);
 
         // Cleartext Body
-        let csf_encoded_text = read_from_buf(&mut b, "cleartext body", cleartext_body)?;
+        let (csf_encoded_text, prefix) = read_cleartext_body(&mut b)?;
+        let b = std::io::Cursor::new(prefix).chain(b);
 
         // Signatures
-        let mut dearmor = armor::Dearmor::new(b);
+        let mut dearmor = armor::Dearmor::with_limit(b, limit);
         dearmor.read_header()?;
         // Safe to unwrap, as read_header succeeded.
         let typ = dearmor
@@ -353,21 +350,29 @@ fn has_rest<R: BufRead>(mut b: R) -> Result<bool> {
 
 const HEADER_LINE: &str = "-----BEGIN PGP SIGNED MESSAGE-----";
 
-fn to_string(b: &[u8]) -> std::result::Result<String, std::str::Utf8Error> {
-    std::str::from_utf8(b).map(|s| s.to_string())
-}
+fn read_cleartext_body<B: BufRead>(b: &mut B) -> Result<(String, String)> {
+    let mut out = String::new();
+
+    loop {
+        let read = b.read_line(&mut out)?;
+        // early end
+        if read == 0 {
+            bail!("unexpected early end");
+        }
 
-fn cleartext_body(i: &[u8]) -> IResult<&[u8], String> {
-    let (i, lines) = map_res(
-        alt((
-            complete(take_until1("\r\n-----")),
-            complete(take_until1("\n-----")),
-        )),
-        to_string,
-    )(i)?;
-    let (i, _) = line_ending(i)?;
-
-    Ok((i, lines))
+        // Look at the last line
+        if let Some(pos) = out.rfind("\n-----") {
+            // found our end
+            let rest = out.split_off(pos + 1);
+            // remove trailing line break
+            if let Some(pos) = out.rfind("\r\n") {
+                out.truncate(pos);
+            } else {
+                out.truncate(out.len() - 1);
+            }
+            return Ok((out, rest));
+        }
+    }
 }
 
 #[cfg(test)]
@@ -535,14 +540,16 @@ mod tests {
 
     #[test]
     fn test_cleartext_body() {
+        let mut data = std::io::Cursor::new(b"-- hello\n--world\n-----bla");
         assert_eq!(
-            cleartext_body(b"-- hello\n--world\n-----bla").unwrap(),
-            (&b"-----bla"[..], "-- hello\n--world".to_string())
+            read_cleartext_body(&mut data).unwrap(),
+            ("-- hello\n--world".to_string(), "-----bla".to_string()),
         );
 
+        let mut data = std::io::Cursor::new(b"-- hello\r\n--world\r\n-----bla");
         assert_eq!(
-            cleartext_body(b"-- hello\r\n--world\r\n-----bla").unwrap(),
-            (&b"-----bla"[..], "-- hello\r\n--world".to_string())
+            read_cleartext_body(&mut data).unwrap(),
+            ("-- hello\r\n--world".to_string(), "-----bla".to_string()),
         );
     }
 
@@ -616,6 +623,14 @@ mod tests {
         msg.verify(&key.public_key()).unwrap();
     }
 
+    #[test]
+    fn test_load_big_csf() {
+        let msg_data = std::fs::read_to_string("./tests/unit-tests/csf-puppet/InRelease").unwrap();
+
+        // FIXME: this fails to read -> buffer_redux problem!?
+        let (_msg, _) = CleartextSignedMessage::from_armor(msg_data.as_bytes()).unwrap();
+    }
+
     #[test]
     fn test_verify_csf_puppet() {
         // test data via https://github.com/rpgp/rpgp/issues/424
diff --git a/src/lib.rs b/src/lib.rs
@@ -58,3 +58,6 @@ pub use self::packet::Signature;
 
 /// The version of this crate.
 pub const VERSION: &str = env!("CARGO_PKG_VERSION");
+
+/// Default maximum size that gets buffered.
+pub const MAX_BUFFER_SIZE: usize = 1024 * 1024 * 1024;
diff --git a/src/packet/many.rs b/src/packet/many.rs
