Re: [PATCH] pki_crypto: pad RSA signature blobs

[Jon Simons <jon@xxxxxxxxxxxxx>]

On 1/20/14, 1:26 AM, Aris Adamantiadis wrote:
> Could you just tweak your patch to remove the NULL tests ?
> ssh_string_data is guaranteed to return non-NULL by construction. With
> this you can also remove the paderrout: label.

On 1/20/14, 1:28 AM, Andreas Schneider wrote: 
> Instead of memset(), please use the BURN_BUFFER() macro. It ensures that the 
> compiler doesn't optimize away the memset().

Thanks, Aris and Andreas for reviewing the change.  I've
attached an updated patch which applies both suggestions.


Thanks,
-Jon

From 842e6c68be183303143317b01311960bb858316a Mon Sep 17 00:00:00 2001
From: Jon Simons <jon@xxxxxxxxxxxxx>
Date: Sun, 19 Jan 2014 16:37:57 -0800
Subject: [PATCH] pki_crypto: pad RSA signature blobs

Pad RSA signature blobs to the expected RSA signature length
when processing via 'pki_signature_to_blob'.

Some clients, notably PuTTY, may send unpadded RSA signatures
during the public key exchange: before this change, one can
sometimes observe failure in signature validation when using
PuTTY's 'plink' client, along these lines:

   ssh_packet_process: ssh_packet_process: Dispatching handler for packet type 50
   ssh_packet_userauth_request: ssh_packet_userauth_request: Auth request for service ssh-connection, method publickey for user 'foo'
   ssh_pki_signature_verify_blob: ssh_pki_signature_verify_blob: Going to verify a ssh-rsa type signature
   pki_signature_verify: pki_signature_verify: RSA error: error:04091077:rsa routines:INT_RSA_VERIFY:wrong signature length
   ssh_packet_userauth_request: ssh_packet_userauth_request: Received an invalid  signature from peer

For cross-reference this issue once also existed between
PuTTY and OpenSSH:

  http://www.chiark.greenend.org.uk/~sgtatham/putty/wishlist/rsa-verify-failed.html

  http://www.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/ssh-rsa.c?rev=1.19;content-type=text%2Fx-cvsweb-markup

With the fix I am unable to reproduce the above failure mode when
testing with 'plink'.
---
 src/pki_crypto.c | 80 +++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 56 insertions(+), 24 deletions(-)

diff --git a/src/pki_crypto.c b/src/pki_crypto.c
index 1080a80..85471f9 100644
--- a/src/pki_crypto.c
+++ b/src/pki_crypto.c
@@ -1188,6 +1188,61 @@ ssh_string pki_signature_to_blob(const ssh_signature sig)
     return sig_blob;
 }
 
+static ssh_signature pki_signature_from_rsa_blob(const ssh_key pubkey,
+                                                 const ssh_string sig_blob,
+                                                 ssh_signature sig)
+{
+    uint32_t pad_len = 0;
+    char *blob_orig;
+    char *blob_padded_data;
+    ssh_string sig_blob_padded;
+
+    size_t len = ssh_string_len(sig_blob);
+    size_t rsalen= RSA_size(pubkey->rsa);
+
+    if (len > rsalen) {
+        ssh_pki_log("Signature is too big: %lu > %lu",
+                    (unsigned long)len, (unsigned long)rsalen);
+        goto errout;
+    }
+
+#ifdef DEBUG_CRYPTO
+    ssh_pki_log("RSA signature len: %lu", (unsigned long)len);
+    ssh_print_hexa("RSA signature", ssh_string_data(sig_blob), len);
+#endif
+
+    if (len == rsalen) {
+        sig->rsa_sig = ssh_string_copy(sig_blob);
+    } else {
+        /* pad the blob to the expected rsalen size */
+        ssh_pki_log("RSA signature len %lu < %lu",
+                    (unsigned long)len, (unsigned long)rsalen);
+
+        pad_len = rsalen - len;
+
+        sig_blob_padded = ssh_string_new(rsalen);
+        if (sig_blob_padded == NULL) {
+            goto errout;
+        }
+
+        blob_padded_data = (char *) ssh_string_data(sig_blob_padded);
+        blob_orig = (char *) ssh_string_data(sig_blob);
+
+        /* front-pad the buffer with zeroes */
+        BURN_BUFFER(blob_padded_data, pad_len);
+        /* fill the rest with the actual signature blob */
+        memcpy(blob_padded_data + pad_len, blob_orig, len);
+
+        sig->rsa_sig = sig_blob_padded;
+    }
+
+    return sig;
+
+errout:
+    ssh_signature_free(sig);
+    return NULL;
+}
+
 ssh_signature pki_signature_from_blob(const ssh_key pubkey,
                                       const ssh_string sig_blob,
                                       enum ssh_keytypes_e type)
@@ -1196,7 +1251,6 @@ ssh_signature pki_signature_from_blob(const ssh_key pubkey,
     ssh_string r;
     ssh_string s;
     size_t len;
-    size_t rsalen;
 
     sig = ssh_signature_new();
     if (sig == NULL) {
@@ -1260,29 +1314,7 @@ ssh_signature pki_signature_from_blob(const ssh_key pubkey,
             break;
         case SSH_KEYTYPE_RSA:
         case SSH_KEYTYPE_RSA1:
-            rsalen = RSA_size(pubkey->rsa);
-
-            if (len > rsalen) {
-                ssh_pki_log("Signature is to big size: %lu",
-                            (unsigned long)len);
-                ssh_signature_free(sig);
-                return NULL;
-            }
-
-            if (len < rsalen) {
-                ssh_pki_log("RSA signature len %lu < %lu",
-                            (unsigned long)len, (unsigned long)rsalen);
-            }
-
-#ifdef DEBUG_CRYPTO
-            ssh_pki_log("RSA signature len: %lu", (unsigned long)len);
-            ssh_print_hexa("RSA signature", ssh_string_data(sig_blob), len);
-#endif
-            sig->rsa_sig = ssh_string_copy(sig_blob);
-            if (sig->rsa_sig == NULL) {
-                ssh_signature_free(sig);
-                return NULL;
-            }
+            sig = pki_signature_from_rsa_blob(pubkey, sig_blob, sig);
             break;
         case SSH_KEYTYPE_ECDSA:
 #ifdef HAVE_OPENSSL_ECC
-- 
1.8.4.21.g992c386