INCOMPATIBLE CHANGE to the SSH2 private key file format. There is

now a passphrase-keyed MAC covering _all_ important data in the
file, including the public blob and the key comment. Should
conclusively scupper any attacks based on nobbling the key file in
an attempt to sucker the machine that decrypts it. MACing the
comment field also protects against a key-substitution attack (if
someone's worked out a way past our DSA protections and can extract
the private key from a signature, swapping key files and
substituting comments might just enable them to get the signature
they need to do this. Paranoid, but might as well).

[originally from svn r1413]

pageant.c

@@ -236,6 +236,26 @@ static int CALLBACK PassphraseProc(HWND hwnd, UINT msg,
     return 0;
 }
 
+/*
+ * Warn about the obsolescent key file format.
+ */
+void old_keyfile_warning(void)
+{
+    static const char mbtitle[] = "PuTTY Key File Warning";
+    static const char message[] =
+	"You are loading an SSH 2 private key which has an\n"
+	"old version of the file format. This means your key\n"
+	"file is not fully tamperproof. Future versions of\n"
+	"PuTTY may stop supporting this private key format,\n"
+	"so we recommend you convert your key to the new\n"
+	"format.\n"
+	"\n"
+	"You can perform this conversion by loading the key\n"
+	"into PuTTYgen and then saving it again.";
+
+    MessageBox(NULL, message, mbtitle, MB_OK);
+}
+
 /*
  * Update the visible key list.
  */

plink.c

@@ -162,6 +162,25 @@ void askcipher(char *ciphername, int cs)
     }
 }
 
+/*
+ * Warn about the obsolescent key file format.
+ */
+void old_keyfile_warning(void)
+{
+    static const char message[] =
+	"You are loading an SSH 2 private key which has an\n"
+	"old version of the file format. This means your key\n"
+	"file is not fully tamperproof. Future versions of\n"
+	"PuTTY may stop supporting this private key format,\n"
+	"so we recommend you convert your key to the new\n"
+	"format.\n"
+	"\n"
+	"Once the key is loaded into PuTTYgen, you can perform\n"
+	"this conversion simply by saving it again.\n";
+
+    fputs(message, stderr);
+}
+
 HANDLE inhandle, outhandle, errhandle;
 DWORD orig_console_mode;
 

psftp.c

@@ -1361,6 +1361,25 @@ void askcipher(char *ciphername, int cs)
     }
 }
 
+/*
+ * Warn about the obsolescent key file format.
+ */
+void old_keyfile_warning(void)
+{
+    static const char message[] =
+	"You are loading an SSH 2 private key which has an\n"
+	"old version of the file format. This means your key\n"
+	"file is not fully tamperproof. Future versions of\n"
+	"PuTTY may stop supporting this private key format,\n"
+	"so we recommend you convert your key to the new\n"
+	"format.\n"
+	"\n"
+	"Once the key is loaded into PuTTYgen, you can perform\n"
+	"this conversion simply by saving it again.\n";
+
+    fputs(message, stderr);
+}
+
 /*
  *  Print an error message and perform a fatal exit.
  */

puttygen.c

@@ -406,6 +406,26 @@ static int save_ssh1_pubkey(char *filename, struct RSAKey *key)
     return 1;
 }
 
+/*
+ * Warn about the obsolescent key file format.
+ */
+void old_keyfile_warning(void)
+{
+    static const char mbtitle[] = "PuTTY Key File Warning";
+    static const char message[] =
+	"You are loading an SSH 2 private key which has an\n"
+	"old version of the file format. This means your key\n"
+	"file is not fully tamperproof. Future versions of\n"
+	"PuTTY may stop supporting this private key format,\n"
+	"so we recommend you convert your key to the new\n"
+	"format.\n"
+	"\n"
+	"Once the key is loaded into PuTTYgen, you can perform\n"
+	"this conversion simply by saving it again.";
+
+    MessageBox(NULL, message, mbtitle, MB_OK);
+}
+
 static int save_ssh2_pubkey(char *filename, struct ssh2_userkey *key)
 {
     unsigned char *pub_blob;

scp.c

@@ -226,6 +226,25 @@ void askcipher(char *ciphername, int cs)
     }
 }
 
+/*
+ * Warn about the obsolescent key file format.
+ */
+void old_keyfile_warning(void)
+{
+    static const char message[] =
+	"You are loading an SSH 2 private key which has an\n"
+	"old version of the file format. This means your key\n"
+	"file is not fully tamperproof. Future versions of\n"
+	"PuTTY may stop supporting this private key format,\n"
+	"so we recommend you convert your key to the new\n"
+	"format.\n"
+	"\n"
+	"Once the key is loaded into PuTTYgen, you can perform\n"
+	"this conversion simply by saving it again.\n";
+
+    fputs(message, stderr);
+}
+
 /* GUI Adaptation - Sept 2000 */
 static void send_msg(HWND h, UINT message, WPARAM wParam)
 {

ssh.h

@@ -355,3 +355,9 @@ int zlib_decompress_block(unsigned char *block, int len,
 #define SSH2_AGENTC_ADD_IDENTITY                17
 #define SSH2_AGENTC_REMOVE_IDENTITY             18
 #define SSH2_AGENTC_REMOVE_ALL_IDENTITIES       19
+
+/*
+ * Need this to warn about support for the original SSH2 keyfile
+ * format.
+ */
+void old_keyfile_warning(void);

sshdss.c

@@ -361,25 +361,15 @@ static unsigned char *dss_private_blob(void *key, int *len)
     xlen = (bignum_bitcount(dss->x) + 8) / 8;
 
     /*
-     * mpint x, string[20] the SHA of p||q||g. Total 28 + xlen.
+     * mpint x, string[20] the SHA of p||q||g. Total 4 + xlen.
-     * (two length fields and twenty bytes, 20+8=28).
      */
-    bloblen = 28 + xlen;
+    bloblen = 4 + xlen;
     blob = smalloc(bloblen);
     p = blob;
     PUT_32BIT(p, xlen);
     p += 4;
     for (i = xlen; i--;)
 	*p++ = bignum_byte(dss->x, i);
-    PUT_32BIT(p, 20);
-    SHA_Init(&s);
-    sha_mpint(&s, dss->p);
-    sha_mpint(&s, dss->q);
-    sha_mpint(&s, dss->g);
-    SHA_Final(&s, digest);
-    p += 4;
-    for (i = 0; i < 20; i++)
-	*p++ = digest[i];
     assert(p == blob + bloblen);
     *len = bloblen;
     return blob;
@@ -398,24 +388,22 @@ static void *dss_createkey(unsigned char *pub_blob, int pub_len,
 
     dss = dss_newkey((char *) pub_blob, pub_len);
     dss->x = getmp(&pb, &priv_len);
-    getstring(&pb, &priv_len, &hash, &hashlen);
 
     /*
-     * Verify details of the key. First check that the hash is
+     * Check the obsolete hash in the old DSS key format.
-     * indeed a hash of p||q||g.
      */
-    if (hashlen != 20) {
+    hashlen = -1;
-	dss_freekey(dss);
+    getstring(&pb, &priv_len, &hash, &hashlen);
-	return NULL;
+    if (hashlen == 20) {
-    }
+	SHA_Init(&s);
-    SHA_Init(&s);
+	sha_mpint(&s, dss->p);
-    sha_mpint(&s, dss->p);
+	sha_mpint(&s, dss->q);
-    sha_mpint(&s, dss->q);
+	sha_mpint(&s, dss->g);
-    sha_mpint(&s, dss->g);
+	SHA_Final(&s, digest);
-    SHA_Final(&s, digest);
+	if (0 != memcmp(hash, digest, 20)) {
-    if (0 != memcmp(hash, digest, 20)) {
+	    dss_freekey(dss);
-	dss_freekey(dss);
+	    return NULL;
-	return NULL;
+	}
     }
 
     /*

sshpubk.c

@@ -10,6 +10,7 @@
 #include <assert.h>
 
 #include "ssh.h"
+#include "misc.h"
 
 #define PUT_32BIT(cp, value) do { \
   (cp)[3] = (value); \
@@ -306,7 +307,7 @@ int saversakey(char *filename, struct RSAKey *key, char *passphrase)
  * The file is text. Lines are terminated by CRLF, although CR-only
  * and LF-only are tolerated on input.
  *
- * The first line says "PuTTY-User-Key-File-1: " plus the name of the
+ * The first line says "PuTTY-User-Key-File-2: " plus the name of the
  * algorithm ("ssh-dss", "ssh-rsa" etc).
  *
  * The next line says "Encryption: " plus an encryption type.
@@ -339,19 +340,18 @@ int saversakey(char *filename, struct RSAKey *key, char *passphrase)
  * And for "ssh-dss", it will be composed of
  *
  *    mpint  x                  (the private key parameter)
- *    string hash               (20-byte hash of mpints p || q || g)
+ *  [ string hash   20-byte hash of mpints p || q || g   only in old format ]
- *
- * Finally, there is a line saying _either_
- *
- *  - "Private-Hash: " plus a hex representation of a SHA-1 hash of
- *    the plaintext version of the private part, including the
- *    final padding.
  * 
- * or
+ * Finally, there is a line saying "Private-MAC: " plus a hex
- * 
+ * representation of a HMAC-SHA-1 of:
- *  - "Private-MAC: " plus a hex representation of a HMAC-SHA-1 of
+ *
- *    the plaintext version of the private part, including the
+ *    string  name of algorithm ("ssh-dss", "ssh-rsa")
- *    final padding.
+ *    string  encryption type
+ *    string  comment
+ *    string  public-blob
+ *    string  private-plaintext (the plaintext version of the
+ *                               private part, including the final
+ *                               padding)
  * 
  * The key to the MAC is itself a SHA-1 hash of:
  * 
@@ -371,16 +371,15 @@ 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
+ * For backwards compatibility with snapshots between 0.51 and
- * with reference to the _private_ data. There exist attacks based
+ * 0.52, we also support the older key file format, which begins
- * on modifying the public key but leaving the private section
+ * with "PuTTY-User-Key-File-1" (version number differs). In this
- * intact.
+ * format the Private-MAC: field only covers the private-plaintext
- *
+ * field and nothing else (and without the 4-byte string length on
- * With RSA, this is easy: verify that n = p*q, and also verify
+ * the front too). Moreover, for RSA keys the Private-MAC: field
- * that e*d == 1 modulo (p-1)(q-1). With DSA, we need to store
+ * can be replaced with a Private-Hash: field which is a plain
- * extra data in the private section other than just x, namely a
+ * SHA-1 hash instead of an HMAC. This is not allowable in DSA
- * hash of p||q||g. (It's then easy to verify that y is equal to
+ * keys. (Yes, the old format was a mess. Guess why it changed :-)
- * g^x mod p.)
  */
 
 static int read_header(FILE * fp, char *header)
@@ -535,13 +534,13 @@ struct ssh2_userkey ssh2_wrong_passphrase = {
 struct ssh2_userkey *ssh2_load_userkey(char *filename, char *passphrase)
 {
     FILE *fp;
-    char header[40], *b, *comment, *mac;
+    char header[40], *b, *encryption, *comment, *mac;
     const struct ssh_signkey *alg;
     struct ssh2_userkey *ret;
     int cipher, cipherblk;
     unsigned char *public_blob, *private_blob;
     int public_blob_len, private_blob_len;
-    int i, is_mac;
+    int i, is_mac, old_fmt;
     int passlen = passphrase ? strlen(passphrase) : 0;
 
     ret = NULL;			       /* return NULL for most errors */
@@ -553,8 +552,15 @@ struct ssh2_userkey *ssh2_load_userkey(char *filename, char *passphrase)
 	goto error;
 
     /* Read the first header line which contains the key type. */
-    if (!read_header(fp, header)
+    if (!read_header(fp, header))
-	|| 0 != strcmp(header, "PuTTY-User-Key-File-1"))
+	goto error;
+    if (0 == strcmp(header, "PuTTY-User-Key-File-2")) {
+	old_fmt = 0;
+    } else if (0 == strcmp(header, "PuTTY-User-Key-File-1")) {
+	/* this is an old key file; warn and then continue */
+	old_keyfile_warning();
+	old_fmt = 1;
+    } else
 	goto error;
     if ((b = read_body(fp)) == NULL)
 	goto error;
@@ -572,19 +578,18 @@ struct ssh2_userkey *ssh2_load_userkey(char *filename, char *passphrase)
     /* Read the Encryption header line. */
     if (!read_header(fp, header) || 0 != strcmp(header, "Encryption"))
 	goto error;
-    if ((b = read_body(fp)) == NULL)
+    if ((encryption = read_body(fp)) == NULL)
 	goto error;
-    if (!strcmp(b, "aes256-cbc")) {
+    if (!strcmp(encryption, "aes256-cbc")) {
 	cipher = 1;
 	cipherblk = 16;
-    } else if (!strcmp(b, "none")) {
+    } else if (!strcmp(encryption, "none")) {
 	cipher = 0;
 	cipherblk = 1;
     } else {
-	sfree(b);
+	sfree(encryption);
 	goto error;
     }
-    sfree(b);
 
     /* Read the Comment header line. */
     if (!read_header(fp, header) || 0 != strcmp(header, "Comment"))
@@ -619,7 +624,8 @@ struct ssh2_userkey *ssh2_load_userkey(char *filename, char *passphrase)
 	if ((mac = read_body(fp)) == NULL)
 	    goto error;
 	is_mac = 1;
-    } else if (0 == strcmp(header, "Private-Hash")) {
+    } else if (0 == strcmp(header, "Private-Hash") &&
+			   alg == &ssh_rsa && old_fmt) {
 	if ((mac = read_body(fp)) == NULL)
 	    goto error;
 	is_mac = 0;
@@ -653,33 +659,66 @@ struct ssh2_userkey *ssh2_load_userkey(char *filename, char *passphrase)
     }
 
     /*
-     * Verify the private hash.
+     * Verify the MAC.
      */
     {
 	char realmac[41];
 	unsigned char binary[20];
+	unsigned char *macdata;
+	int maclen;
+	int free_macdata;
+
+	if (old_fmt) {
+	    /* MAC (or hash) only covers the private blob. */
+	    macdata = private_blob;
+	    maclen = private_blob_len;
+	    free_macdata = 0;
+	} else {
+	    unsigned char *p;
+	    int namelen = strlen(alg->name);
+	    int enclen = strlen(encryption);
+	    int commlen = strlen(comment);
+	    maclen = (4 + namelen +
+		      4 + enclen +
+		      4 + commlen +
+		      4 + public_blob_len +
+		      4 + private_blob_len);
+	    macdata = smalloc(maclen);
+	    p = macdata;
+#define DO_STR(s,len) PUT_32BIT(p,(len));memcpy(p+4,(s),(len));p+=4+(len)
+	    DO_STR(alg->name, namelen);
+	    DO_STR(encryption, enclen);
+	    DO_STR(comment, commlen);
+	    DO_STR(public_blob, public_blob_len);
+	    DO_STR(private_blob, private_blob_len);
+
+	    free_macdata = 1;
+	}
 
 	if (is_mac) {
 	    SHA_State s;
 	    unsigned char mackey[20];
 	    char header[] = "putty-private-key-file-mac-key";
 
-	    if (!passphrase)	       /* can't have MAC in unencrypted key */
-		goto error;
-
 	    SHA_Init(&s);
 	    SHA_Bytes(&s, header, sizeof(header)-1);
-	    SHA_Bytes(&s, passphrase, passlen);
+	    if (passphrase)
+		SHA_Bytes(&s, passphrase, passlen);
 	    SHA_Final(&s, mackey);
 
-	    hmac_sha1_simple(mackey, 20, private_blob, private_blob_len,
+	    hmac_sha1_simple(mackey, 20, macdata, maclen, binary);
-			     binary);
 
 	    memset(mackey, 0, sizeof(mackey));
 	    memset(&s, 0, sizeof(s));
 	} else {
-	    SHA_Simple(private_blob, private_blob_len, binary);
+	    SHA_Simple(macdata, maclen, binary);
 	}
+
+	if (free_macdata) {
+	    memset(macdata, 0, maclen);
+	    sfree(macdata);
+	}
+
 	for (i = 0; i < 20; i++)
 	    sprintf(realmac + 2 * i, "%02x", binary[i]);
 
@@ -707,6 +746,7 @@ struct ssh2_userkey *ssh2_load_userkey(char *filename, char *passphrase)
     }
     sfree(public_blob);
     sfree(private_blob);
+    sfree(encryption);
     return ret;
 
     /*
@@ -717,6 +757,8 @@ struct ssh2_userkey *ssh2_load_userkey(char *filename, char *passphrase)
 	fclose(fp);
     if (comment)
 	sfree(comment);
+    if (encryption)
+	sfree(encryption);
     if (mac)
 	sfree(mac);
     if (public_blob)
@@ -744,7 +786,8 @@ char *ssh2_userkey_loadpub(char *filename, char **algorithm,
 
     /* Read the first header line which contains the key type. */
     if (!read_header(fp, header)
-	|| 0 != strcmp(header, "PuTTY-User-Key-File-1"))
+	|| (0 != strcmp(header, "PuTTY-User-Key-File-2") &&
+	    0 != strcmp(header, "PuTTY-User-Key-File-1")))
 	goto error;
     if ((b = read_body(fp)) == NULL)
 	goto error;
@@ -812,7 +855,8 @@ int ssh2_userkey_encrypted(char *filename, char **commentptr)
     if (!fp)
 	return 0;
     if (!read_header(fp, header)
-	|| 0 != strcmp(header, "PuTTY-User-Key-File-1")) {
+	|| (0 != strcmp(header, "PuTTY-User-Key-File-2") &&
+	    0 != strcmp(header, "PuTTY-User-Key-File-1"))) {
 	fclose(fp);
 	return 0;
     }
@@ -914,7 +958,7 @@ int ssh2_save_userkey(char *filename, struct ssh2_userkey *key,
     int pub_blob_len, priv_blob_len, priv_encrypted_len;
     int passlen;
     int cipherblk;
-    int i, is_mac;
+    int i;
     char *cipherstr;
     unsigned char priv_mac[20];
 
@@ -951,29 +995,42 @@ int ssh2_save_userkey(char *filename, struct ssh2_userkey *key,
     memcpy(priv_blob_encrypted + priv_blob_len, priv_mac,
 	   priv_encrypted_len - priv_blob_len);
 
-    /* Now create the private MAC. */
+    /* Now create the MAC. */
-    if (passphrase) {
+    {
+	unsigned char *macdata;
+	int maclen;
+	unsigned char *p;
+	int namelen = strlen(key->alg->name);
+	int enclen = strlen(cipherstr);
+	int commlen = strlen(key->comment);
 	SHA_State s;
 	unsigned char mackey[20];
 	char header[] = "putty-private-key-file-mac-key";
 
-	passlen = strlen(passphrase);
+	maclen = (4 + namelen +
+		  4 + enclen +
+		  4 + commlen +
+		  4 + pub_blob_len +
+		  4 + priv_encrypted_len);
+	macdata = smalloc(maclen);
+	p = macdata;
+#define DO_STR(s,len) PUT_32BIT(p,(len));memcpy(p+4,(s),(len));p+=4+(len)
+	DO_STR(key->alg->name, namelen);
+	DO_STR(cipherstr, enclen);
+	DO_STR(key->comment, commlen);
+	DO_STR(pub_blob, pub_blob_len);
+	DO_STR(priv_blob_encrypted, priv_encrypted_len);
 
 	SHA_Init(&s);
 	SHA_Bytes(&s, header, sizeof(header)-1);
-	SHA_Bytes(&s, passphrase, passlen);
+	if (passphrase)
+	    SHA_Bytes(&s, passphrase, strlen(passphrase));
 	SHA_Final(&s, mackey);
-
+	hmac_sha1_simple(mackey, 20, macdata, maclen, priv_mac);
-	hmac_sha1_simple(mackey, 20,
+	memset(macdata, 0, maclen);
-			 priv_blob_encrypted, priv_encrypted_len,
+	sfree(macdata);
-			 priv_mac);
-	is_mac = 1;
-
 	memset(mackey, 0, sizeof(mackey));
 	memset(&s, 0, sizeof(s));
-    } else {
-	SHA_Simple(priv_blob_encrypted, priv_encrypted_len, priv_mac);
-	is_mac = 0;
     }
 
     if (passphrase) {
@@ -1000,21 +1057,23 @@ int ssh2_save_userkey(char *filename, struct ssh2_userkey *key,
     fp = fopen(filename, "w");
     if (!fp)
 	return 0;
-    fprintf(fp, "PuTTY-User-Key-File-1: %s\n", key->alg->name);
+    fprintf(fp, "PuTTY-User-Key-File-2: %s\n", key->alg->name);
     fprintf(fp, "Encryption: %s\n", cipherstr);
     fprintf(fp, "Comment: %s\n", key->comment);
     fprintf(fp, "Public-Lines: %d\n", base64_lines(pub_blob_len));
     base64_encode(fp, pub_blob, pub_blob_len);
     fprintf(fp, "Private-Lines: %d\n", base64_lines(priv_encrypted_len));
     base64_encode(fp, priv_blob_encrypted, priv_encrypted_len);
-    if (is_mac)
+    fprintf(fp, "Private-MAC: ");
-	fprintf(fp, "Private-MAC: ");
-    else
-	fprintf(fp, "Private-Hash: ");
     for (i = 0; i < 20; i++)
 	fprintf(fp, "%02x", priv_mac[i]);
     fprintf(fp, "\n");
     fclose(fp);
+
+    sfree(pub_blob);
+    memset(priv_blob, 0, priv_blob_len);
+    sfree(priv_blob);
+    sfree(priv_blob_encrypted);
     return 1;
 }
 

windlg.c

@@ -2952,3 +2952,23 @@ int askappend(char *filename)
     else
 	return 0;
 }
+
+/*
+ * Warn about the obsolescent key file format.
+ */
+void old_keyfile_warning(void)
+{
+    static const char mbtitle[] = "PuTTY Key File Warning";
+    static const char message[] =
+	"You are loading an SSH 2 private key which has an\n"
+	"old version of the file format. This means your key\n"
+	"file is not fully tamperproof. Future versions of\n"
+	"PuTTY may stop supporting this private key format,\n"
+	"so we recommend you convert your key to the new\n"
+	"format.\n"
+	"\n"
+	"You can perform this conversion by loading the key\n"
+	"into PuTTYgen and then saving it again.";
+
+    MessageBox(NULL, message, mbtitle, MB_OK);
+}