Give a sensible error when using a too-short RSA key.

The ssh_signkey vtable has grown a new method ssh_key_invalid(), which
checks whether the key is going to be usable for constructing a
signature at all. Currently the only way this can fail is if it's an
RSA key so short that there isn't room to put all the PKCS#1
formatting in the signature preimage integer, but the return value is
an arbitrary error message just in case more reasons are needed later.

This is tested separately rather than at key-creation time because of
the signature flags system: an RSA key of intermediate length could be
valid for SHA-1 signing but not for SHA-512. So really this method
should be called at the point where you've decided what sig flags you
want to use, and you're checking if _those flags_ are OK.

On the verification side, there's no need for a separate check. If
someone presents us with an RSA key so short that it's impossible to
encode a valid signature using it, then we simply regard all
signatures as invalid.

misc.h

@@ -170,6 +170,8 @@ int string_length_for_printf(size_t);
  * string. */
 #define PTRLEN_LITERAL(stringlit) \
     TYPECHECK("" stringlit "", make_ptrlen(stringlit, sizeof(stringlit)-1))
+/* Make a ptrlen out of a constant byte array. */
+#define PTRLEN_FROM_CONST_BYTES(a) make_ptrlen(a, sizeof(a))
 
 /* Wipe sensitive data out of memory that's about to be freed. Simpler
  * than memset because we don't need the fill char parameter; also

pageant.c

@@ -349,6 +349,15 @@ void pageant_handle_msg(BinarySink *bs,
                 return;
             }
 
+            char *invalid = ssh_key_invalid(key->key, flags);
+            if (invalid) {
+                char *msg = dupprintf("key invalid: %s", invalid);
+                pageant_failure_msg(bs, msg, logctx, logfn);
+                sfree(msg);
+                sfree(invalid);
+                return;
+            }
+
             signature = strbuf_new();
             ssh_key_sign(key->key, sigdata, flags,
                          BinarySink_UPCAST(signature));

ssh.h

@@ -706,6 +706,7 @@ struct ssh_keyalg {
 
     /* Methods that operate on an existing ssh_key */
     void (*freekey) (ssh_key *key);
+    char *(*invalid) (ssh_key *key, unsigned flags);
     void (*sign) (ssh_key *key, ptrlen data, unsigned flags, BinarySink *);
     bool (*verify) (ssh_key *key, ptrlen sig, ptrlen data);
     void (*public_blob)(ssh_key *key, BinarySink *);
@@ -728,6 +729,7 @@ struct ssh_keyalg {
 #define ssh_key_new_priv_openssh(alg, bs) ((alg)->new_priv_openssh(alg, bs))
 
 #define ssh_key_free(key) ((key)->vt->freekey(key))
+#define ssh_key_invalid(key, flags) ((key)->vt->invalid(key, flags))
 #define ssh_key_sign(key, data, flags, bs) \
     ((key)->vt->sign(key, data, flags, bs))
 #define ssh_key_verify(key, sig, data) ((key)->vt->verify(key, sig, data))

ssh2userauth.c

@@ -850,9 +850,26 @@ static void ssh2_userauth_process_queue(PacketProtocolLayer *ppl)
                             ppl_printf("Unable to load private key (%s)\r\n",
                                        error);
                             key = NULL;
+                            s->suppress_wait_for_response_packet = true;
                             break; /* try something else */
                         }
                     }
+
+                    /* FIXME: if we ever support variable signature
+                     * flags, this is somewhere they'll need to be
+                     * put */
+                    char *invalid = ssh_key_invalid(key->key, 0);
+                    if (invalid) {
+                        ppl_printf("Cannot use this private key (%s)\r\n",
+                                   invalid);
+                        ssh_key_free(key->key);
+                        sfree(key->comment);
+                        sfree(key);
+                        sfree(invalid);
+                        key = NULL;
+                        s->suppress_wait_for_response_packet = true;
+                        break; /* try something else */
+                    }
                 }
 
                 if (key) {

sshdss.c

@@ -83,6 +83,12 @@ static char *dss_cache_str(ssh_key *key)
     return strbuf_to_str(sb);
 }
 
+static char *dss_invalid(ssh_key *key, unsigned flags)
+{
+    /* No validity criterion will stop us from using a DSA key at all */
+    return NULL;
+}
+
 static bool dss_verify(ssh_key *key, ptrlen sig, ptrlen data)
 {
     struct dss_key *dss = container_of(key, struct dss_key, sshk);
@@ -459,6 +465,7 @@ const ssh_keyalg ssh_dss = {
     dss_new_priv_openssh,
 
     dss_freekey,
+    dss_invalid,
     dss_sign,
     dss_verify,
     dss_public_blob,

sshecc.c

@@ -549,6 +549,13 @@ static void eddsa_freekey(ssh_key *key)
     sfree(ek);
 }
 
+static char *ec_signkey_invalid(ssh_key *key, unsigned flags)
+{
+    /* All validity criteria for both ECDSA and EdDSA were checked
+     * when we loaded the key in the first place */
+    return NULL;
+}
+
 static ssh_key *ecdsa_new_pub(const ssh_keyalg *alg, ptrlen data)
 {
     const struct ecsign_extra *extra =
@@ -1126,6 +1133,7 @@ const ssh_keyalg ssh_ecdsa_ed25519 = {
     eddsa_new_priv_openssh,
 
     eddsa_freekey,
+    ec_signkey_invalid,
     eddsa_sign,
     eddsa_verify,
     eddsa_public_blob,
@@ -1155,6 +1163,7 @@ const ssh_keyalg ssh_ecdsa_nistp256 = {
     ecdsa_new_priv_openssh,
 
     ecdsa_freekey,
+    ec_signkey_invalid,
     ecdsa_sign,
     ecdsa_verify,
     ecdsa_public_blob,
@@ -1184,6 +1193,7 @@ const ssh_keyalg ssh_ecdsa_nistp384 = {
     ecdsa_new_priv_openssh,
 
     ecdsa_freekey,
+    ec_signkey_invalid,
     ecdsa_sign,
     ecdsa_verify,
     ecdsa_public_blob,
@@ -1213,6 +1223,7 @@ const ssh_keyalg ssh_ecdsa_nistp521 = {
     ecdsa_new_priv_openssh,
 
     ecdsa_freekey,
+    ec_signkey_invalid,
     ecdsa_sign,
     ecdsa_verify,
     ecdsa_public_blob,

sshrsa.c

@@ -551,75 +551,109 @@ static int rsa2_pubkey_bits(const ssh_keyalg *self, ptrlen pub)
     return ret;
 }
 
-/*
- * This is the magic ASN.1/DER prefix that goes in the decoded
- * signature, between the string of FFs and the actual SHA hash
- * value. The meaning of it is:
- * 
- * 00 -- this marks the end of the FFs; not part of the ASN.1 bit itself
- * 
- * 30 21 -- a constructed SEQUENCE of length 0x21
- *    30 09 -- a constructed sub-SEQUENCE of length 9
- *       06 05 -- an object identifier, length 5
- *          2B 0E 03 02 1A -- object id { 1 3 14 3 2 26 }
- *                            (the 1,3 comes from 0x2B = 43 = 40*1+3)
- *       05 00 -- NULL
- *    04 14 -- a primitive OCTET STRING of length 0x14
- *       [0x14 bytes of hash data follows]
- * 
- * The object id in the middle there is listed as `id-sha1' in
- * ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-1/pkcs-1v2-1d2.asn (the
- * ASN module for PKCS #1) and its expanded form is as follows:
- * 
- * id-sha1                OBJECT IDENTIFIER ::= {
- *    iso(1) identified-organization(3) oiw(14) secsig(3)
- *    algorithms(2) 26 }
- */
-static const unsigned char sha1_asn1_prefix[] = {
-    0x00, 0x30, 0x21, 0x30, 0x09, 0x06, 0x05, 0x2B,
-    0x0E, 0x03, 0x02, 0x1A, 0x05, 0x00, 0x04, 0x14,
-};
+static inline const ssh_hashalg *rsa2_hash_alg_for_flags(
+    unsigned flags, const char **protocol_id_out)
+{
+    const ssh_hashalg *halg;
+    const char *protocol_id;
 
-/*
- * Two more similar pieces of ASN.1 used for signatures using SHA-256
- * and SHA-512, in the same format but differing only in various
- * length fields and OID.
- */
-static const unsigned char sha256_asn1_prefix[] = {
-    0x00, 0x30, 0x31, 0x30, 0x0d, 0x06, 0x09, 0x60,
-    0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x01,
-    0x05, 0x00, 0x04, 0x20,
-};
-static const unsigned char sha512_asn1_prefix[] = {
-    0x00, 0x30, 0x51, 0x30, 0x0d, 0x06, 0x09, 0x60,
-    0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x03,
-    0x05, 0x00, 0x04, 0x40,
-};
+    if (flags & SSH_AGENT_RSA_SHA2_256) {
+        halg = &ssh_sha256;
+        protocol_id = "rsa-sha2-256";
+    } else if (flags & SSH_AGENT_RSA_SHA2_512) {
+        halg = &ssh_sha512;
+        protocol_id = "rsa-sha2-512";
+    } else {
+        halg = &ssh_sha1;
+        protocol_id = "ssh-rsa";
+    }
 
-#define SHA1_ASN1_PREFIX_LEN sizeof(sha1_asn1_prefix)
+    if (protocol_id_out)
+        *protocol_id_out = protocol_id;
+
+    return halg;
+}
+
+static inline ptrlen rsa_pkcs1_prefix_for_hash(const ssh_hashalg *halg)
+{
+    if (halg == &ssh_sha1) {
+        /*
+         * This is the magic ASN.1/DER prefix that goes in the decoded
+         * signature, between the string of FFs and the actual SHA-1
+         * hash value. The meaning of it is:
+         *
+         * 00 -- this marks the end of the FFs; not part of the ASN.1
+         * bit itself
+         *
+         * 30 21 -- a constructed SEQUENCE of length 0x21
+         *    30 09 -- a constructed sub-SEQUENCE of length 9
+         *       06 05 -- an object identifier, length 5
+         *          2B 0E 03 02 1A -- object id { 1 3 14 3 2 26 }
+         *                            (the 1,3 comes from 0x2B = 43 = 40*1+3)
+         *       05 00 -- NULL
+         *    04 14 -- a primitive OCTET STRING of length 0x14
+         *       [0x14 bytes of hash data follows]
+         *
+         * The object id in the middle there is listed as `id-sha1' in
+         * ftp://ftp.rsasecurity.com/pub/pkcs/pkcs-1/pkcs-1v2-1d2.asn
+         * (the ASN module for PKCS #1) and its expanded form is as
+         * follows:
+         *
+         * id-sha1                OBJECT IDENTIFIER ::= {
+         *    iso(1) identified-organization(3) oiw(14) secsig(3)
+         *    algorithms(2) 26 }
+         */
+        static const unsigned char sha1_asn1_prefix[] = {
+            0x00, 0x30, 0x21, 0x30, 0x09, 0x06, 0x05, 0x2B,
+            0x0E, 0x03, 0x02, 0x1A, 0x05, 0x00, 0x04, 0x14,
+        };
+        return PTRLEN_FROM_CONST_BYTES(sha1_asn1_prefix);
+    }
+
+    if (halg == &ssh_sha256) {
+        /*
+         * A similar piece of ASN.1 used for signatures using SHA-256,
+         * in the same format but differing only in various length
+         * fields and OID.
+         */
+        static const unsigned char sha256_asn1_prefix[] = {
+            0x00, 0x30, 0x31, 0x30, 0x0d, 0x06, 0x09, 0x60,
+            0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x01,
+            0x05, 0x00, 0x04, 0x20,
+        };
+        return PTRLEN_FROM_CONST_BYTES(sha256_asn1_prefix);
+    }
+
+    if (halg == &ssh_sha512) {
+        /*
+         * And one more for SHA-512.
+         */
+        static const unsigned char sha512_asn1_prefix[] = {
+            0x00, 0x30, 0x51, 0x30, 0x0d, 0x06, 0x09, 0x60,
+            0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x03,
+            0x05, 0x00, 0x04, 0x40,
+        };
+        return PTRLEN_FROM_CONST_BYTES(sha512_asn1_prefix);
+    }
+
+    unreachable("bad hash algorithm for RSA PKCS#1");
+}
+
+static inline size_t rsa_pkcs1_length_of_fixed_parts(const ssh_hashalg *halg)
+{
+    ptrlen asn1_prefix = rsa_pkcs1_prefix_for_hash(halg);
+    return halg->hlen + asn1_prefix.len + 2;
+}
 
 static unsigned char *rsa_pkcs1_signature_string(
     size_t nbytes, const ssh_hashalg *halg, ptrlen data)
 {
-    const unsigned char *asn1_prefix;
-    unsigned asn1_prefix_size;
-
-    if (halg == &ssh_sha256) {
-        asn1_prefix = sha256_asn1_prefix;
-        asn1_prefix_size = sizeof(sha256_asn1_prefix);
-    } else if (halg == &ssh_sha512) {
-        asn1_prefix = sha512_asn1_prefix;
-        asn1_prefix_size = sizeof(sha512_asn1_prefix);
-    } else {
-        assert(halg == &ssh_sha1);
-        asn1_prefix = sha1_asn1_prefix;
-        asn1_prefix_size = sizeof(sha1_asn1_prefix);
-    }
-
-    size_t fixed_parts = halg->hlen + asn1_prefix_size + 2;
+    size_t fixed_parts = rsa_pkcs1_length_of_fixed_parts(halg);
     assert(nbytes >= fixed_parts);
     size_t padding = nbytes - fixed_parts;
 
+    ptrlen asn1_prefix = rsa_pkcs1_prefix_for_hash(halg);
+
     unsigned char *bytes = snewn(nbytes, unsigned char);
 
     bytes[0] = 0;
@@ -627,11 +661,11 @@ static unsigned char *rsa_pkcs1_signature_string(
 
     memset(bytes + 2, 0xFF, padding);
 
-    memcpy(bytes + 2 + padding, asn1_prefix, asn1_prefix_size);
+    memcpy(bytes + 2 + padding, asn1_prefix.ptr, asn1_prefix.len);
 
     ssh_hash *h = ssh_hash_new(halg);
     put_datapl(h, data);
-    ssh_hash_final(h, bytes + 2 + padding + asn1_prefix_size);
+    ssh_hash_final(h, bytes + 2 + padding + asn1_prefix.len);
 
     return bytes;
 }
@@ -643,6 +677,15 @@ static bool rsa2_verify(ssh_key *key, ptrlen sig, ptrlen data)
     ptrlen type, in_pl;
     mp_int *in, *out;
 
+    /* If we need to support variable flags on verify, this is where they go */
+    const ssh_hashalg *halg = rsa2_hash_alg_for_flags(0, NULL);
+
+    /* Start by making sure the key is even long enough to encode a
+     * signature. If not, everything fails to verify. */
+    size_t nbytes = (mp_get_nbits(rsa->modulus) + 7) / 8;
+    if (nbytes < rsa_pkcs1_length_of_fixed_parts(halg))
+        return false;
+
     BinarySource_BARE_INIT_PL(src, sig);
     type = get_string(src);
     /*
@@ -665,8 +708,7 @@ static bool rsa2_verify(ssh_key *key, ptrlen sig, ptrlen data)
 
     unsigned diff = 0;
 
-    size_t nbytes = (mp_get_nbits(rsa->modulus) + 7) / 8;
-    unsigned char *bytes = rsa_pkcs1_signature_string(nbytes, &ssh_sha1, data);
+    unsigned char *bytes = rsa_pkcs1_signature_string(nbytes, halg, data);
     for (size_t i = 0; i < nbytes; i++)
         diff |= bytes[nbytes-1 - i] ^ mp_get_byte(out, i);
     smemclr(bytes, nbytes);
@@ -686,16 +728,7 @@ static void rsa2_sign(ssh_key *key, ptrlen data,
     const ssh_hashalg *halg;
     const char *sign_alg_name;
 
-    if (flags & SSH_AGENT_RSA_SHA2_256) {
-        halg = &ssh_sha256;
-        sign_alg_name = "rsa-sha2-256";
-    } else if (flags & SSH_AGENT_RSA_SHA2_512) {
-        halg = &ssh_sha512;
-        sign_alg_name = "rsa-sha2-512";
-    } else {
-        halg = &ssh_sha1;
-        sign_alg_name = "ssh-rsa";
-    }
+    halg = rsa2_hash_alg_for_flags(flags, &sign_alg_name);
 
     nbytes = (mp_get_nbits(rsa->modulus) + 7) / 8;
 
@@ -716,12 +749,28 @@ static void rsa2_sign(ssh_key *key, ptrlen data,
     mp_free(out);
 }
 
+char *rsa2_invalid(ssh_key *key, unsigned flags)
+{
+    RSAKey *rsa = container_of(key, RSAKey, sshk);
+    size_t bits = mp_get_nbits(rsa->modulus), nbytes = (bits + 7) / 8;
+    const char *sign_alg_name;
+    const ssh_hashalg *halg = rsa2_hash_alg_for_flags(flags, &sign_alg_name);
+    if (nbytes < rsa_pkcs1_length_of_fixed_parts(halg)) {
+        return dupprintf(
+            "%zu-bit RSA key is too short to generate %s signatures",
+            bits, sign_alg_name);
+    }
+
+    return NULL;
+}
+
 const ssh_keyalg ssh_rsa = {
     rsa2_new_pub,
     rsa2_new_priv,
     rsa2_new_priv_openssh,
 
     rsa2_freekey,
+    rsa2_invalid,
     rsa2_sign,
     rsa2_verify,
     rsa2_public_blob,

testcrypt.c

@@ -506,6 +506,14 @@ static void return_val_string_asciz(strbuf *out, char *s)
     return_val_string(out, sb);
 }
 
+static void return_opt_val_string_asciz(strbuf *out, char *s)
+{
+    if (!s)
+        strbuf_catf(out, "NULL\n");
+    else
+        return_val_string_asciz(out, s);
+}
+
 static void return_opt_val_cipher(strbuf *out, ssh_cipher *c)
 {
     if (!c)

testcrypt.h

@@ -147,6 +147,7 @@ FUNC1(val_string, ssh2_mac_genresult, val_mac)
 FUNC2(val_key, ssh_key_new_pub, keyalg, val_string_ptrlen)
 FUNC3(val_key, ssh_key_new_priv, keyalg, val_string_ptrlen, val_string_ptrlen)
 FUNC2(val_key, ssh_key_new_priv_openssh, keyalg, val_string_binarysource)
+FUNC2(opt_val_string_asciz, ssh_key_invalid, val_key, uint)
 FUNC4(void, ssh_key_sign, val_key, val_string_ptrlen, uint, out_val_string_binarysink)
 FUNC3(boolean, ssh_key_verify, val_key, val_string_ptrlen, val_string_ptrlen)
 FUNC2(void, ssh_key_public_blob, val_key, out_val_string_binarysink)

unix/uxserver.c

@@ -416,6 +416,12 @@ int main(int argc, char **argv)
                             "%s\n", appname, val, error);
                     exit(1);
                 }
+                char *invalid = ssh_key_invalid(uk->key, 0);
+                if (invalid) {
+                    fprintf(stderr, "%s: host key '%s' is unusable: "
+                            "%s\n", appname, val, invalid);
+                    exit(1);
+                }
                 key = uk->key;
                 sfree(uk->comment);
                 sfree(uk);