Pad RSA signature blobs if they're made with SHA-2.

The "rsa-sha2-256" and "rsa-sha2-512" algorithms, as defined by RFC
8332, differ in one detail from "ssh-rsa" in addition to the change of
hash function. They also specify that the signature integer should be
encoded using the same number of bytes as the key modulus, even if
that means giving it a leading zero byte (or even more than one).

I hadn't noticed this, and had assumed that unrelated details wouldn't
have changed. But they had. Thanks to Ilia Mirkin for pointing this
out.

Nobody has previously reported a problem, so very likely most servers
are forgiving of people making this mistake! But now it's been pointed
out, we should comply with the spec. (Especially since the new spec is
more sensible, and only historical inertia justified sticking to the
old one.)

crypto/rsa.c

@@ -837,7 +837,36 @@ static void rsa2_sign(ssh_key *key, ptrlen data,
     mp_free(in);
 
     put_stringz(bs, sign_alg_name);
-    nbytes = (mp_get_nbits(out) + 7) / 8;
+    if (flags == 0) {
+        /*
+         * Original "ssh-rsa", per RFC 4253 section 6.6, stores the
+         * signature integer in a string without padding - not even
+         * the leading zero byte that an ordinary SSH-2 mpint would
+         * require to avoid looking like two's complement.
+         *
+         * "The value for 'rsa_signature_blob' is encoded as a string
+         * containing s (which is an integer, without lengths or
+         * padding, unsigned, and in network byte order)."
+         */
+        nbytes = (mp_get_nbits(out) + 7) / 8;
+    } else {
+        /*
+         * The SHA-256 and SHA-512 signature systems, per RFC 8332
+         * section 3, should be padded to the length of the key
+         * modulus.
+         *
+         * "The value for 'rsa_signature_blob' is encoded as a string
+         * that contains an octet string S (which is the output of
+         * RSASSA-PKCS1-v1_5) and that has the same length (in octets)
+         * as the RSA modulus."
+         *
+         * Awkwardly, RFC 8332 doesn't say whether that means the
+         * 'raw' length of the RSA modulus (that is, ceil(n/8) for an
+         * n-bit key) or the length it would occupy as an SSH-2 mpint.
+         * My interpretation is the former.
+         */
+        nbytes = (mp_get_nbits(rsa->modulus) + 7) / 8;
+    }
     put_uint32(bs, nbytes);
     for (size_t i = 0; i < nbytes; i++)
         put_byte(bs, mp_get_byte(out, nbytes - 1 - i));

test/cryptsuite.py

@@ -2708,6 +2708,62 @@ culpa qui officia deserunt mollit anim id est laborum.
                         self.assertFalse(ssh_key_verify(
                             key, badsig, test_message))
 
+    def testShortRSASignatures(self):
+        key = ssh_key_new_priv('rsa', b64("""
+AAAAB3NzaC1yc2EAAAADAQABAAABAQDeoTvwEDg46K7vYrQFFwbo2sBPahNoiw7i
+RMbpwuOIH8sAOFAWzDvIZpDODGkobwc2hM8FRlZUg3lTgDaqVuMJOupG0xzOqehu
+Fw3kXrm6ScWxaUXs+b5o88sqXBBYs91KmsWqYKTUUwDBuDHdo8Neq8h8SJqspCo4
+qctIoLoTrXYMqonfHXZp4bIn5WPN6jNL9pLi7Y+sl8aLe4w73aZzxMphecQfMMVJ
+ezmv9zgA7gKw5ErorIGKCF44YRbvNisZA5j2DyLLsd/ztw2ikIEnx9Ng33+tGEBC
+zq2RYb4ZtuT9dHXcNiHx3CqLLlFrjl13hQamjwoVy4ydDIdQZjYz
+"""), b64("""
+AAABADhDwXUrdDoVvFhttpduuWVSG7Y2Vc9fDZTr0uWzRnPZrSFSGhOY7Cb6nPAm
+PNFmNgl2SSfJHfpf++K5jZdBPEHR7PGXWzlzwXVJSE6GDiRhjqAGvhBlEdVOf/Ml
+r0/rrSq0sO4dXKr4i0FqPtgIElEz0whuBQFKwAzwBJtHW5+rBzGLvoHh560UN4IK
+SU3jv/nDMvPohEBfOA0puqYAfZM8PmU/kbgERPLyPp/dfnGBC4LlOnAcak+2RNu6
+yQ5v0NKksyYidCI76Ztf3B9asNNN4AbTg8JbzABwjtxR+0bDOOi0RwAJC2Wn2FOy
+WiJsQWjz/fMnJW8WVg3DR/va/4EAAACBAP8rwn1vy4Y/S1EixR1XZM9F1OvJQwzN
+EKhD1Qbr1YlLX3oZRnulJg/j0HupGnKCRh8DulNbrmXlMFdeZHDVFw87/o73DTCQ
+g2qnRNUwJdBkePrA563vVx6OXe5TSF3+3SRNMesAdN8ExvGeOFP10z0FZhS3Zuco
+y4WrxQBXVetBAAAAgQDfWmh5CRJEBCJbLHMdZK8QAw2QbKiD0VTw7PkDWJuSbYDB
+AvEhoFuXUO/yMIfCCFjUuzO+3iumk8F/lFTkJ620Aah5tzRcKCtyW4YuoQUYMgpW
+5/hGIL4M4bvUDGUGI3+SOn8qfAzCCsFD0FdR6ms0pMubaJQmeiI2wyM9ehOIcwAA
+AIEAmKEX1YZHNtx/D/SaTsl6z+KwmOluqjzyjrwL16QLpIR1/F7lAjSnMGz3yORp
++314D3yZzKutpalwwsS4+z7838pilVaV7iWqF4TMDKYZ/6/baRXpwxrwFqvWXwQ3
+cLerc7bpA/IeVovoTirt0RNxdMPIVv3XsXE7pqatJBwOcQE=
+"""))
+
+        def decode(data):
+            pos = 0
+            alg, data = ssh_decode_string(data, True)
+            sig, data = ssh_decode_string(data, True)
+            self.assertEqual(data, b'')
+            return alg, sig
+
+        # An RSA signature over each hash which comes out a byte short
+        # with this key. Found in the obvious manner, by signing
+        # "message0", "message1", ... until one was short.
+        #
+        # We expect the ssh-rsa signature to be stored short, and the
+        # other two to be padded with a zero byte.
+        blob = ssh_key_sign(key, "message79", 0)
+        alg, sig = decode(blob)
+        self.assertEqual(alg, b"ssh-rsa")
+        self.assertEqual(len(sig), 255) # one byte short
+        self.assertNotEqual(sig[0], 0)
+
+        blob = ssh_key_sign(key, "message208", 2)
+        alg, sig = decode(blob)
+        self.assertEqual(alg, b"rsa-sha2-256")
+        self.assertEqual(len(sig), 256) # full-length
+        self.assertEqual(sig[0], 0) # and has a leading zero byte
+
+        blob = ssh_key_sign(key, "message461", 4)
+        alg, sig = decode(blob)
+        self.assertEqual(alg, b"rsa-sha2-512")
+        self.assertEqual(len(sig), 256) # full-length
+        self.assertEqual(sig[0], 0) # and has a leading zero byte
+
     def testPPKLoadSave(self):
         # Stability test of PPK load/save functions.
         input_clear_key = b"""\