From 9cde4b27309582715edc0692501a5df9d62c9f17 Mon Sep 17 00:00:00 2001
From: Justus Winter <justus@sequoia-pgp.org>
Date: Thu, 6 Jul 2023 18:24:59 +0200
Subject: openpgp: Fix hashing v3 signatures.

  - The high-level hashing functions are implemented on
    SignatureFields (so that we can use them from the
    SignatureBuilder).  Unfortunately, when those functions invoke
    SignatureFields::hash, the type encoding the packet version has
    been erased.

  - Recover the version at runtime and dispatch to the right hashing
    function.
---
 openpgp/src/cert.rs                                |  28 ++++++++++++++
 openpgp/src/crypto/hash.rs                         |  42 ++++++++++++++++-----
 .../data/keys/pgp5-dsa-elg-v3-subkey-binding.pgp   | Bin 0 -> 701 bytes
 3 files changed, 61 insertions(+), 9 deletions(-)
 create mode 100644 openpgp/tests/data/keys/pgp5-dsa-elg-v3-subkey-binding.pgp

diff --git a/openpgp/src/cert.rs b/openpgp/src/cert.rs
index 23050580..0170b0e8 100644
--- a/openpgp/src/cert.rs
+++ b/openpgp/src/cert.rs
@@ -7247,4 +7247,32 @@ Pu1xwz57O4zo1VYf6TqHJzVC3OMvMUM2hhdecMUe5x6GorNaj6g=
 
         Ok(())
     }
+
+    /// Tests v3 binding signatures.
+    #[test]
+    fn v3_binding_signature() -> Result<()> {
+        if ! crate::types::PublicKeyAlgorithm::DSA.is_supported() {
+            eprintln!("Skipping because DSA is not supported");
+            return Ok(());
+        }
+
+        let c = Cert::from_bytes(
+            crate::tests::key("pgp5-dsa-elg-v3-subkey-binding.pgp"))?;
+        assert_eq!(c.bad_signatures().count(), 0);
+
+        let np = crate::policy::NullPolicy::new();
+
+        // The subkey is interesting because it is bound using a v3
+        // signature.
+        let vcert = c.with_policy(&np, None)?;
+        assert_eq!(vcert.keys().subkeys().count(), 1);
+
+        // XXX: Unfortunately, it being a v3 signature, the subkey has
+        // no keyflags, limiting its usefulness for now.
+
+        // The subkey is interesting because it is bound using a v3
+        // signature.
+        assert_eq!(c.keys().subkeys().with_policy(&np, None).count(), 1);
+        Ok(())
+    }
 }
diff --git a/openpgp/src/crypto/hash.rs b/openpgp/src/crypto/hash.rs
index 532e3d41..d22b0377 100644
--- a/openpgp/src/crypto/hash.rs
+++ b/openpgp/src/crypto/hash.rs
@@ -420,18 +420,28 @@ impl Hash for Signature {
 
 impl Hash for Signature3 {
     fn hash(&self, hash: &mut dyn Digest) {
+        Self::hash_signature(self, hash);
+    }
+}
+
+impl Signature3 {
+    /// Hashes this signature.
+    ///
+    /// Because we need to call this from SignatureFields::hash, we
+    /// provide this as associated method.
+    fn hash_signature(f: &signature::SignatureFields, hash: &mut dyn Digest) {
         // XXX: Annoyingly, we have no proper way of handling errors
         // here.
 
         let mut buffer = [0u8; 5];
 
         // Signature type.
-        buffer[0] = u8::from(self.typ());
+        buffer[0] = u8::from(f.typ());
 
         // Creation time.
         let creation_time: u32 =
             Timestamp::try_from(
-                self.signature_creation_time()
+                f.signature_creation_time()
                     .unwrap_or(std::time::UNIX_EPOCH))
             .unwrap_or_else(|_| Timestamp::from(0))
             .into();
@@ -447,17 +457,21 @@ impl Hash for Signature3 {
 
 impl Hash for Signature4 {
     fn hash(&self, hash: &mut dyn Digest) {
-        self.fields.hash(hash);
+        Self::hash_signature(self, hash);
     }
 }
 
-impl Hash for signature::SignatureFields {
-    fn hash(&self, hash: &mut dyn Digest) {
+impl Signature4 {
+    /// Hashes this signature.
+    ///
+    /// Because we need to call this from SignatureFields::hash, we
+    /// provide this as associated method.
+    fn hash_signature(f: &signature::SignatureFields, hash: &mut dyn Digest) {
         use crate::serialize::MarshalInto;
 
         // XXX: Annoyingly, we have no proper way of handling errors
         // here.
-        let hashed_area = self.hashed_area().to_vec()
+        let hashed_area = f.hashed_area().to_vec()
             .unwrap_or_else(|_| Vec::new());
 
         // A version 4 signature packet is laid out as follows:
@@ -474,9 +488,9 @@ impl Hash for signature::SignatureFields {
 
         // Version.
         header[0] = 4;
-        header[1] = self.typ().into();
-        header[2] = self.pk_algo().into();
-        header[3] = self.hash_algo().into();
+        header[1] = f.typ().into();
+        header[2] = f.pk_algo().into();
+        header[3] = f.hash_algo().into();
 
         // The length of the hashed area, as a 16-bit big endian number.
         let len = hashed_area.len() as u16;
@@ -509,6 +523,16 @@ impl Hash for signature::SignatureFields {
     }
 }
 
+impl Hash for signature::SignatureFields {
+    fn hash(&self, hash: &mut dyn Digest) {
+        match self.version() {
+            3 => Signature3::hash_signature(self, hash),
+            4 => Signature4::hash_signature(self, hash),
+            _ => (),
+        }
+    }
+}
+
 /// Hashing-related functionality.
 ///
 /// <a id="hashing-functions"></a>
diff --git a/openpgp/tests/data/keys/pgp5-dsa-elg-v3-subkey-binding.pgp b/openpgp/tests/data/keys/pgp5-dsa-elg-v3-subkey-binding.pgp
new file mode 100644
index 00000000..7a51c1db
Binary files /dev/null and b/openpgp/tests/data/keys/pgp5-dsa-elg-v3-subkey-binding.pgp differ
-- 
cgit v1.2.3