openpgp: Correctly handle text signatures when verifying.

  - Text signatures require normalizing the line endings to "\r\n"
    before the text is hashed.  This change implements this for the
    consumption of signatures.  The next commit will handle the
    production of such signatures.

  - See #530.

4 files changed, 274 insertions, 39 deletions

diff --git a/openpgp/src/crypto/mod.rs b/openpgp/src/crypto/mod.rs
index cbf23751..5deba504 100644
--- a/openpgp/src/crypto/mod.rs
+++ b/openpgp/src/crypto/mod.rs
@@ -27,6 +27,7 @@ use buffered_reader::BufferedReader;
 
 use crate::types::HashAlgorithm;
 use crate::Result;
+use crate::parse::HashingMode;
 
 pub(crate) mod aead;
 mod asymmetric;
@@ -218,8 +219,8 @@ impl Password {
 /// convenient method, see [`DetachedVerifier`].
 ///
 ///  [`DetachedVerifier`]: ../parse/stream/struct.DetachedVerifier.html
-pub fn hash_reader<R: Read>(reader: R, algos: &[HashAlgorithm])
-    -> Result<Vec<hash::Context>>
+pub fn hash_reader<R: Read>(reader: R, algos: &[HashingMode<HashAlgorithm>])
+    -> Result<Vec<HashingMode<hash::Context>>>
 {
     let reader
         = buffered_reader::Generic::with_cookie(
@@ -233,8 +234,9 @@ pub fn hash_reader<R: Read>(reader: R, algos: &[HashAlgorithm])
 /// convenient method, see [`DetachedVerifier`].
 ///
 ///  [`DetachedVerifier`]: ../parse/stream/struct.DetachedVerifier.html
-pub(crate) fn hash_buffered_reader<R>(reader: R, algos: &[HashAlgorithm])
-    -> Result<Vec<hash::Context>>
+pub(crate) fn hash_buffered_reader<R>(reader: R,
+                                      algos: &[HashingMode<HashAlgorithm>])
+    -> Result<Vec<HashingMode<hash::Context>>>
     where R: BufferedReader<crate::parse::Cookie>,
 {
     use std::mem;
@@ -266,10 +268,13 @@ fn hash_reader_test() {
 
     let result =
         hash_reader(std::io::Cursor::new(crate::tests::manifesto()),
-                    &expected.keys().cloned().collect::<Vec<HashAlgorithm>>())
+                    &expected.keys().cloned()
+                    .map(|algo| HashingMode::Binary(algo)).
+                    collect::<Vec<_>>())
         .unwrap();
 
-    for mut hash in result.into_iter() {
+    for mut mode in result.into_iter() {
+        let hash = mode.as_mut();
         let algo = hash.algo();
         let mut digest = vec![0u8; hash.digest_size()];
         hash.digest(&mut digest);
diff --git a/openpgp/src/parse.rs b/openpgp/src/parse.rs
index 9ef03594..fba0d956 100644
--- a/openpgp/src/parse.rs
+++ b/openpgp/src/parse.rs
@@ -713,12 +713,95 @@ pub(crate) struct SignatureGroup {
     ops_count: usize,
 
     /// The hash contexts.
-    pub(crate) hashes: Vec<crypto::hash::Context>,
+    pub(crate) hashes: Vec<HashingMode<crypto::hash::Context>>,
+}
+
+/// Controls line-ending normalization during hashing.
+///
+/// OpenPGP normalizes line endings when signing or verifying text
+/// signatures.
+pub enum HashingMode<T> {
+    /// Hash for a binary signature.
+    ///
+    /// The data is hashed as-is.
+    Binary(T),
+
+    /// Hash for a text signature.
+    ///
+    /// The data is hashed with line endings normalized to `\r\n`.
+    Text(T),
+}
+
+impl<T: Clone> Clone for HashingMode<T> {
+    fn clone(&self) -> Self {
+        use self::HashingMode::*;
+        match self {
+            Binary(t) => Binary(t.clone()),
+            Text(t) => Text(t.clone()),
+        }
+    }
+}
+
+impl<T: std::fmt::Debug> std::fmt::Debug for HashingMode<T> {
+    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+        use self::HashingMode::*;
+        match self {
+            Binary(t) => write!(f, "Binary({:?})", t),
+            Text(t) => write!(f, "Text({:?})", t),
+        }
+    }
+}
+
+impl<T: PartialEq> PartialEq for HashingMode<T> {
+    fn eq(&self, other: &Self) -> bool {
+        use self::HashingMode::*;
+        match (self, other) {
+            (Binary(s), Binary(o)) => s.eq(o),
+            (Text(s), Text(o)) => s.eq(o),
+            _ => false,
+        }
+    }
+}
+
+impl<T: Eq> Eq for HashingMode<T> { }
+
+impl<T> HashingMode<T> {
+    fn map<U, F: Fn(&T) -> U>(&self, f: F) -> HashingMode<U> {
+        use self::HashingMode::*;
+        match self {
+            Binary(t) => Binary(f(t)),
+            Text(t) => Text(f(t)),
+        }
+    }
+
+    pub(crate) fn as_ref(&self) -> &T {
+        use self::HashingMode::*;
+        match self {
+            Binary(t) => t,
+            Text(t) => t,
+        }
+    }
+
+    pub(crate) fn as_mut(&mut self) -> &mut T {
+        use self::HashingMode::*;
+        match self {
+            Binary(t) => t,
+            Text(t) => t,
+        }
+    }
+
+    fn for_signature(t: T, typ: SignatureType) -> Self {
+        if typ == SignatureType::Text {
+            HashingMode::Text(t)
+        } else {
+            HashingMode::Binary(t)
+        }
+    }
 }
 
 impl fmt::Debug for SignatureGroup {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        let algos = self.hashes.iter().map(|ctx| ctx.algo())
+        let algos = self.hashes.iter().map(|mode| mode.map(|ctx| ctx.algo()))
             .collect::<Vec<_>>();
 
         f.debug_struct("Cookie")
@@ -1244,8 +1327,10 @@ impl Signature4 {
             crypto::mpi::Signature::_parse(pk_algo, &mut php));
 
         let hash_algo = hash_algo.into();
+        let typ = typ.into();
+        let need_hash = HashingMode::for_signature(hash_algo, typ);
         let mut pp = php.ok(Packet::Signature(Signature4::new(
-            typ.into(), pk_algo.into(), hash_algo,
+            typ, pk_algo.into(), hash_algo,
             hashed_area,
             unhashed_area,
             [digest_prefix1, digest_prefix2],
@@ -1277,13 +1362,14 @@ impl Signature4 {
                         cookie.sig_group_mut().ops_count -= 1;
                         if let Some(hash) =
                             cookie.sig_group().hashes.iter().find_map(
-                                |ctx| if ctx.algo() == hash_algo {
-                                    Some(ctx)
+                                |mode|
+                                if mode.map(|ctx| ctx.algo()) == need_hash {
+                                    Some(mode.as_ref())
                                 } else {
                                     None
                                 })
                         {
-                            t!("popped a {:?} HashedReader", hash_algo);
+                            t!("found a {:?} HashedReader", need_hash);
                             computed_digest = Some((cookie.signature_level(),
                                                     hash.clone()));
                         }
@@ -1738,11 +1824,13 @@ impl OnePassSig3 {
         let last = php_try!(php.parse_u8("last"));
 
         let hash_algo = hash_algo.into();
-        let mut sig = OnePassSig3::new(typ.into());
+        let typ = typ.into();
+        let mut sig = OnePassSig3::new(typ);
         sig.set_hash_algo(hash_algo);
         sig.set_pk_algo(pk_algo.into());
         sig.set_issuer(KeyID::from_bytes(&issuer));
         sig.set_last_raw(last);
+        let need_hash = HashingMode::for_signature(hash_algo, typ);
 
         let recursion_depth = php.recursion_depth();
 
@@ -1772,11 +1860,14 @@ impl OnePassSig3 {
                                 // Make sure that it uses the required
                                 // hash algorithm.
                                 if ! cookie.sig_group().hashes.iter()
-                                    .any(|ctx| ctx.algo() == hash_algo)
+                                    .any(|mode| {
+                                        mode.map(|ctx| ctx.algo()) == need_hash
+                                    })
                                 {
                                     if let Ok(ctx) = hash_algo.context() {
-                                        cookie.sig_group_mut()
-                                            .hashes.push(ctx);
+                                        cookie.sig_group_mut().hashes.push(
+                                            HashingMode::for_signature(ctx, typ)
+                                        );
                                     }
                                 }
 
@@ -1808,10 +1899,8 @@ impl OnePassSig3 {
         // the hash algorithm so that we have something to match
         // against when we get to the Signature packet.
         let mut algos = Vec::new();
-        let hash_algo = HashAlgorithm::from(hash_algo);
-
         if hash_algo.is_supported() {
-            algos.push(hash_algo);
+            algos.push(HashingMode::for_signature(hash_algo, typ));
         }
 
         // We can't push the HashedReader on the BufferedReader stack:
@@ -2589,9 +2678,11 @@ impl MDC {
                         if state.sig_group().hashes.len() > 0 {
                             let h = state.sig_group_mut().hashes
                                 .iter_mut().find_map(
-                                    |ctx|
-                                    if ctx.algo() == HashAlgorithm::SHA1 {
-                                        Some(ctx)
+                                    |mode|
+                                    if mode.map(|ctx| ctx.algo()) ==
+                                        HashingMode::Binary(HashAlgorithm::SHA1)
+                                    {
+                                        Some(mode.as_mut())
                                     } else {
                                         None
                                     }).unwrap();
@@ -4888,7 +4979,8 @@ impl<'a> PacketParser<'a> {
 
                 // And the hasher.
                 let mut reader = HashedReader::new(
-                    reader, HashesFor::MDC, vec![HashAlgorithm::SHA1]);
+                    reader, HashesFor::MDC,
+                    vec![HashingMode::Binary(HashAlgorithm::SHA1)]);
                 reader.cookie_mut().level = Some(self.recursion_depth());
 
                 t!("Pushing HashedReader, level {:?}.",
@@ -5642,4 +5734,76 @@ mod test {
         assert_eq!(packet2, packet3);
         Ok(())
     }
+
+    /// Checks that newlines are properly normalized when verifying
+    /// text signatures.
+    #[test]
+    fn issue_530_verifying() -> Result<()> {
+        use std::io::Write;
+        use crate::*;
+        use crate::packet::signature;
+        use crate::serialize::stream::{Message, Signer};
+
+        use crate::policy::StandardPolicy;
+        use crate::{Result, Cert};
+        use crate::parse::Parse;
+        use crate::parse::stream::*;
+
+        let data = b"one\r\ntwo\r\nthree";
+
+        let p = &StandardPolicy::new();
+        let cert: Cert =
+            Cert::from_bytes(crate::tests::key("testy-new-private.pgp"))?;
+        let signing_keypair = cert.keys().secret()
+            .with_policy(p, None).alive().revoked(false).for_signing()
+            .nth(0).unwrap()
+            .key().clone().into_keypair()?;
+        let mut signature = vec![];
+        {
+            let message = Message::new(&mut signature);
+            let mut message = Signer::with_template(
+                message, signing_keypair,
+                signature::SignatureBuilder::new(SignatureType::Text)
+            ).detached().build()?;
+            message.write_all(data)?;
+            message.finalize()?;
+        }
+
+        struct Helper {};
+        impl VerificationHelper for Helper {
+            fn get_certs(&mut self, _ids: &[KeyHandle]) -> Result<Vec<Cert>> {
+                Ok(vec![Cert::from_bytes(crate::tests::key("testy-new.pgp"))?])
+            }
+            fn check(&mut self, structure: MessageStructure) -> Result<()> {
+                for (i, layer) in structure.iter().enumerate() {
+                    assert_eq!(i, 0);
+                    if let MessageLayer::SignatureGroup { results } = layer {
+                        assert_eq!(results.len(), 1);
+                        results[0].as_ref().unwrap();
+                        assert!(results[0].is_ok());
+                        return Ok(());
+                    } else {
+                        unreachable!();
+                    }
+                }
+                unreachable!()
+            }
+        }
+
+        let h = Helper {};
+        let mut v = DetachedVerifierBuilder::from_bytes(&signature)?
+            .with_policy(p, None, h)?;
+
+        for data in &[
+            &b"one\r\ntwo\r\nthree"[..], // dos
+            b"one\ntwo\nthree",          // unix
+            b"one\ntwo\r\nthree",        // mixed
+            b"one\r\ntwo\nthree",
+            b"one\rtwo\rthree",          // classic mac
+        ] {
+            v.verify_bytes(data)?;
+        }
+
+        Ok(())
+    }
 }
diff --git a/openpgp/src/parse/hashed_reader.rs b/openpgp/src/parse/hashed_reader.rs
index 090f85f7..c904ebcd 100644
--- a/openpgp/src/parse/hashed_reader.rs
+++ b/openpgp/src/parse/hashed_reader.rs
@@ -7,7 +7,7 @@ use buffered_reader::BufferedReader;
 use buffered_reader::buffered_reader_generic_read_impl;
 
 use crate::HashAlgorithm;
-use crate::parse::{Cookie, HashesFor, Hashing};
+use crate::parse::{Cookie, HashesFor, Hashing, HashingMode};
 
 const TRACE : bool = false;
 
@@ -35,12 +35,13 @@ impl<R: BufferedReader<Cookie>> HashedReader<R> {
     /// Instantiates a new hashed reader.  `hashes_for` is the hash's
     /// purpose.  `algos` is a list of algorithms for which we should
     /// compute the hash.
-    pub fn new(reader: R, hashes_for: HashesFor, algos: Vec<HashAlgorithm>)
+    pub fn new(reader: R, hashes_for: HashesFor,
+               algos: Vec<HashingMode<HashAlgorithm>>)
             -> Self {
         let mut cookie = Cookie::default();
-        for &algo in &algos {
+        for mode in &algos {
             cookie.sig_group_mut().hashes
-                .push(algo.context().unwrap());
+                .push(mode.map(|algo| algo.context().unwrap())); // XXX: Don't unwrap.
         }
         cookie.hashes_for = hashes_for;
 
@@ -51,6 +52,36 @@ impl<R: BufferedReader<Cookie>> HashedReader<R> {
     }
 }
 
+/// Updates the given hash context normalizing line endings to "\r\n"
+/// on the fly.
+fn hash_update_text(h: &mut crate::crypto::hash::Context, text: &[u8]) {
+    let mut line = text;
+    while ! line.is_empty() {
+        let mut next = 0;
+        for (i, c) in line.iter().cloned().enumerate() {
+            match c {
+                b'\r' | b'\n' => {
+                    h.update(&line[..i]);
+                    h.update(b"\r\n");
+                    next = i + 1;
+                    if c == b'\r' && line.get(next) == Some(&b'\n') {
+                        next += 1;
+                    }
+                    break;
+                },
+                _ => (),
+            }
+        }
+
+        if next > 0 {
+            line = &line[next..];
+        } else {
+            h.update(line);
+            break;
+        }
+    }
+}
+
 impl Cookie {
     fn hash_update(&mut self, data: &[u8]) {
         let level = self.level.unwrap_or(0);
@@ -69,12 +100,16 @@ impl Cookie {
             // We fix that here by hashing the stashed data into the
             // former topmost signature-group's hash.
             assert!(ngroups > 1);
-            for h in self.sig_groups[ngroups-2].hashes.iter_mut()
+            for mode in self.sig_groups[ngroups-2].hashes.iter_mut()
             {
                 t!("({:?}): group {} {:?} hashing {} stashed bytes.",
-                   hashes_for, ngroups-2, h.algo(), data.len());
+                   hashes_for, ngroups-2, mode.map(|ctx| ctx.algo()),
+                   data.len());
 
-                h.update(&stashed_data);
+                match mode {
+                    HashingMode::Binary(h) => h.update(&stashed_data),
+                    HashingMode::Text(h) => hash_update_text(h, &stashed_data),
+                }
             }
         }
 
@@ -100,10 +135,13 @@ impl Cookie {
                 return;
             }
 
-            for h in sig_group.hashes.iter_mut() {
+            for mode in sig_group.hashes.iter_mut() {
                 t!("{:?}): group {} {:?} hashing {} bytes.",
-                   hashes_for, i, h.algo(), data.len());
-                h.update(data);
+                   hashes_for, i, mode.map(|ctx| ctx.algo()), data.len());
+                match mode {
+                    HashingMode::Binary(h) => h.update(&data),
+                    HashingMode::Text(h) => hash_update_text(h, &data),
+                }
             }
         }
     }
@@ -266,7 +304,9 @@ mod test {
                     test.data, None, Default::default());
             let mut reader
                 = HashedReader::new(reader, HashesFor::MDC,
-                                    test.expected.keys().cloned().collect());
+                                    test.expected.keys().cloned()
+                                    .map(|algo| HashingMode::Binary(algo))
+                                    .collect());
 
             assert_eq!(reader.steal_eof().unwrap(), test.data);
 
@@ -274,7 +314,8 @@ mod test {
 
             let mut hashes = mem::replace(&mut cookie.sig_group_mut().hashes,
                                           Default::default());
-            for hash in hashes.iter_mut() {
+            for mode in hashes.iter_mut() {
+                let hash = mode.as_mut();
                 let algo = hash.algo();
                 let mut digest = vec![0u8; hash.digest_size()];
                 hash.digest(&mut digest);
@@ -287,4 +328,23 @@ mod test {
             }
         }
     }
+
+    #[test]
+    fn hash_update_text() -> crate::Result<()> {
+        for text in &[
+            "one\r\ntwo\r\nthree",
+            "one\ntwo\nthree",
+            "one\rtwo\rthree",
+            "one\ntwo\r\nthree",
+        ] {
+            let mut ctx = HashAlgorithm::SHA256.context()?;
+            super::hash_update_text(&mut ctx, text.as_bytes());
+            let mut digest = vec![0; ctx.digest_size()];
+            ctx.digest(&mut digest);
+            assert_eq!(
+                &crate::fmt::hex::encode(&digest),
+                "5536758151607BB81CE8D6F49189B2E84763DA9EA84965AB7327E704DAE415EB");
+        }
+        Ok(())
+    }
 }
diff --git a/openpgp/src/parse/stream.rs b/openpgp/src/parse/stream.rs
index 980ef37f..cfeba6c6 100644
--- a/openpgp/src/parse/stream.rs
+++ b/openpgp/src/parse/stream.rs
@@ -138,6 +138,7 @@ use crate::{
 };
 use crate::parse::{
     Cookie,
+    HashingMode,
     PacketParser,
     PacketParserBuilder,
     PacketParserResult,
@@ -2400,18 +2401,23 @@ impl<'a, H: VerificationHelper + DecryptionHelper> Decryptor<'a, H> {
         };
 
         // Compute the necessary hashes.
-        let algos: Vec<_> = sigs.iter().map(|s| s.hash_algo()).collect();
+        let algos: Vec<_> = sigs.iter().map(|s| {
+            HashingMode::for_signature(s.hash_algo(), s.typ())
+        }).collect();
         let hashes = crate::crypto::hash_buffered_reader(data, &algos)?;
 
         // Attach the digests.
         for sig in sigs.iter_mut() {
-            let algo = sig.hash_algo();
+            let need_hash =
+                HashingMode::for_signature(sig.hash_algo(), sig.typ());
             // Note: |hashes| < 10, most likely 1.
-            for hash in hashes.iter().filter(|c| c.algo() == algo) {
+            for mode in hashes.iter()
+                .filter(|m| m.map(|c| c.algo()) == need_hash)
+            {
                 // Clone the hash context, update it with the
                 // signature.
                 use crate::crypto::hash::Hash;
-                let mut hash = hash.clone();
+                let mut hash = mode.as_ref().clone();
                 sig.hash(&mut hash);
 
                 // Attach digest to the signature.