From ba8be05dab34e2580a7953dd5fcd76caf53e19d2 Mon Sep 17 00:00:00 2001
From: Noa Resare <noa@resare.com>
Date: Wed, 17 May 2023 15:51:56 +0100
Subject: [PATCH] Ensure DSA signatures are encoded according to spec

This change implements the raw encoding and decoding of DSA
signatures according to RFC4253 Section 6.6 instead of relying
on the internal representation in the dsa crate. See #114 for
additional detail.

This change also includes a test case, verifying that we
can verify the response from ssh-agent for a DSA signing request.
---
 ssh-key/src/signature.rs | 25 ++++++++++++++++++++++---
 ssh-key/tests/sshsig.rs  | 21 +++++++++++++++++++++
 2 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/ssh-key/src/signature.rs b/ssh-key/src/signature.rs
index 88b8867b..cfae2e69 100644
--- a/ssh-key/src/signature.rs
+++ b/ssh-key/src/signature.rs
@@ -12,6 +12,7 @@ use crate::{private::Ed25519Keypair, public::Ed25519PublicKey};
 #[cfg(feature = "dsa")]
 use {
     crate::{private::DsaKeypair, public::DsaPublicKey},
+    bigint::BigUint,
     sha1::Sha1,
     signature::{DigestSigner, DigestVerifier},
 };
@@ -320,9 +321,20 @@ impl Signer<Signature> for DsaKeypair {
             .try_sign_digest(Sha1::new_with_prefix(message))
             .map_err(|_| signature::Error::new())?;
 
+        // Note that we need to roll our own signature encoding, as [RFC4253 section 6.6]
+        // specifies two raw 80 bit integer but the dsa::SigningKey serialization
+        // encodes to a der format.
+        let mut buf: Vec<u8> = Vec::new();
+        buf.append(&mut signature.r().to_bytes_be());
+        buf.append(&mut signature.s().to_bytes_be());
+
+        if buf.len() != DSA_SIGNATURE_SIZE {
+            return Err(signature::Error::new());
+        }
+
         Ok(Signature {
             algorithm: Algorithm::Dsa,
-            data: signature.to_vec(),
+            data: buf,
         })
     }
 }
@@ -332,8 +344,15 @@ impl Verifier<Signature> for DsaPublicKey {
     fn verify(&self, message: &[u8], signature: &Signature) -> signature::Result<()> {
         match signature.algorithm {
             Algorithm::Dsa => {
-                let signature = dsa::Signature::try_from(signature.data.as_slice())?;
-
+                let data = signature.data.as_slice();
+                if data.len() != DSA_SIGNATURE_SIZE {
+                    return Err(signature::Error::new());
+                }
+                let (r, s) = data.split_at(DSA_SIGNATURE_SIZE / 2);
+                let signature = dsa::Signature::from_components(
+                    BigUint::from_bytes_be(r),
+                    BigUint::from_bytes_be(s),
+                )?;
                 dsa::VerifyingKey::try_from(self)?
                     .verify_digest(Sha1::new_with_prefix(message), &signature)
                     .map_err(|_| signature::Error::new())
diff --git a/ssh-key/tests/sshsig.rs b/ssh-key/tests/sshsig.rs
index 8e3dde19..fc425ed9 100644
--- a/ssh-key/tests/sshsig.rs
+++ b/ssh-key/tests/sshsig.rs
@@ -16,6 +16,9 @@ use ssh_key::PrivateKey;
 #[cfg(feature = "ed25519")]
 use ssh_key::Error;
 
+#[cfg(feature = "dsa")]
+use {encoding::Decode, signature::Verifier, ssh_key::Signature};
+
 /// DSA OpenSSH-formatted private key.
 #[cfg(feature = "dsa")]
 const DSA_PRIVATE_KEY: &str = include_str!("examples/id_dsa_1024");
@@ -76,6 +79,13 @@ const MSG_EXAMPLE: &[u8] = b"testing";
 /// Example domain/namespace used for the message.
 const NAMESPACE_EXAMPLE: &str = "example";
 
+/// An ssh-agent signature response signing MSG_EXAMPLE with DSA_PRIVATE_KEY
+#[cfg(feature = "dsa")]
+const DSA_SIGNATURE_BYTES: [u8; 55] = hex!(
+    "000000077373682d647373000000282d0c9613d9745c6088ae4d9e8dbf35a557"
+    "7bd0e6796acccb22ab4809d569e86ec619510ec48b6950"
+);
+
 #[test]
 fn decode_ed25519() {
     let sshsig = ED25519_SIGNATURE.parse::<SshSig>().unwrap();
@@ -134,6 +144,17 @@ fn sign_dsa() {
     );
 }
 
+#[test]
+#[cfg(feature = "dsa")]
+fn verify_dsa() {
+    let signature = Signature::decode(&mut DSA_SIGNATURE_BYTES.as_ref()).unwrap();
+    let verifying_key = DSA_PUBLIC_KEY.parse::<PublicKey>().unwrap();
+    verifying_key
+        .key_data()
+        .verify(MSG_EXAMPLE, &signature)
+        .unwrap();
+}
+
 #[test]
 #[cfg(feature = "p256")]
 fn sign_ecdsa_p256() {