Following the recent advisory about attacks on PGP keys based on

tampering with the unencrypted public part of the key but leaving
the private part intact ... we are now ultra-paranoid about RSA key
files, and we check that the public part matches the private part
_before_ we generate any signatures with them.

git-svn-id: svn://svn.tartarus.org/sgt/putty@1021 cda61777-01e9-0310-a592-d414129be87e

diff --git a/sshpubk.c b/sshpubk.c
index b353570..30be7d3 100644 (file)
--- a/sshpubk.c
+++ b/sshpubk.c
@@ -125,7 +125,12 @@ static int loadrsakey_main(FILE *fp, struct RSAKey *key,
     i += ssh1_read_bignum(buf+i, &key->p);
     if (len-i < 0) goto end;
 
-    ret = 1;
+    if (!rsa_verify(key)) {
+       freersakey(key);
+       ret = 0;
+    } else
+       ret = 1;
+
     end:
     memset(buf, 0, sizeof(buf));       /* burn the evidence */
     return ret;
@@ -328,6 +333,16 @@ int saversakey(char *filename, struct RSAKey *key, char *passphrase) {
  * 
  * where the sequence-number increases from zero. As many of these
  * hashes are used as necessary.
+ * 
+ * NOTE! It is important that all _public_ data can be verified
+ * with reference to the _private_ data. There exist attacks based
+ * on modifying the public key but leaving the private section
+ * intact.
+ * 
+ * With RSA, this is easy: verify that n = p*q, and also verify
+ * that e*d == 1 modulo (p-1)(q-1). With DSA (if we were ever to
+ * support it), we would need to store extra data in the private
+ * section other than just x.
  */
 
 static int read_header(FILE *fp, char *header) {
@@ -613,6 +628,11 @@ struct ssh2_userkey *ssh2_load_userkey(char *filename, char *passphrase) {
     ret->comment = comment;
     ret->data = alg->createkey(public_blob, public_blob_len,
                               private_blob, private_blob_len);
+    if (!ret->data) {
+       sfree(ret->comment);
+       sfree(ret);
+       ret = NULL;
+    }
     sfree(public_blob);
     sfree(private_blob);
     return ret;

diff --git a/sshrsa.c b/sshrsa.c
index aeafec3..1938623 100644 (file)
--- a/sshrsa.c
+++ b/sshrsa.c
@@ -151,6 +151,37 @@ void rsa_fingerprint(char *str, int len, struct RSAKey *key) {
     }
 }
 
+/*
+ * Verify that the public data in an RSA key matches the private
+ * data.
+ */
+int rsa_verify(struct RSAKey *key) {
+    Bignum n, ed, pm1, qm1, pm1qm1;
+    int cmp;
+
+    /* n must equal pq. */
+    n = bigmul(key->p, key->q);
+    cmp = bignum_cmp(n, key->modulus);
+    freebn(n);
+    if (cmp != 0)
+       return 0;
+
+    /* e * d must be congruent to 1, modulo (p-1)(q-1). */
+    pm1 = copybn(key->p);
+    decbn(pm1);
+    qm1 = copybn(key->q);
+    decbn(qm1);
+    pm1qm1 = bigmul(pm1, qm1);
+    freebn(pm1);
+    freebn(qm1);
+    ed = modmul(key->exponent, key->private_exponent, pm1qm1);
+    sfree(pm1qm1);
+    cmp = bignum_cmp(ed, One);
+    sfree(ed);
+    if (cmp != 0)
+       return 0;
+}
+
 void freersakey(struct RSAKey *key) {
     if (key->modulus) freebn(key->modulus);
     if (key->exponent) freebn(key->exponent);
@@ -231,7 +262,7 @@ static char *rsa2_fmtkey(void *key) {
     
     len = rsastr_len(rsa);
     p = smalloc(len);
-    rsastr_fmt(p, rsa);
+     rsastr_fmt(p, rsa);
     return p;
 }
 
@@ -304,6 +335,11 @@ static void *rsa2_createkey(unsigned char *pub_blob, int pub_len,
     rsa->q = getmp(&pb, &priv_len);
     rsa->iqmp = getmp(&pb, &priv_len);
 
+    if (!rsa_verify(rsa)) {
+       rsa2_freekey(rsa);
+       return NULL;
+    }
+
     return rsa;
 }