Tighten up a lot of casts from unsigned to int which are read by one

of the GET_32BIT macros and then used as length fields. Missing bounds
checks against zero have been added, and also I've introduced a helper
function toint() which casts from unsigned to int in such a way as to
avoid C undefined behaviour, since I'm not sure I trust compilers any
more to do the obviously sensible thing.


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

Author: simon

conf.c

@@ -522,14 +522,15 @@ int conf_deserialise(Conf *conf, void *vdata, int maxsize)
     unsigned char *data = (unsigned char *)vdata;
     unsigned char *start = data;
     struct conf_entry *entry;
-    int primary, used;
+    unsigned primary;
+    int used;
     unsigned char *zero;
 
     while (maxsize >= 4) {
 	primary = GET_32BIT_MSB_FIRST(data);
 	data += 4, maxsize -= 4;
 
-	if ((unsigned)primary >= N_CONFIG_OPTIONS)
+	if (primary >= N_CONFIG_OPTIONS)
 	    break;
 
 	entry = snew(struct conf_entry);
@@ -541,7 +542,7 @@ int conf_deserialise(Conf *conf, void *vdata, int maxsize)
 		sfree(entry);
 		goto done;
 	    }
-	    entry->key.secondary.i = GET_32BIT_MSB_FIRST(data);
+	    entry->key.secondary.i = toint(GET_32BIT_MSB_FIRST(data));
 	    data += 4, maxsize -= 4;
 	    break;
 	  case TYPE_STR:
@@ -564,7 +565,7 @@ int conf_deserialise(Conf *conf, void *vdata, int maxsize)
 		sfree(entry);
 		goto done;
 	    }
-	    entry->value.u.intval = GET_32BIT_MSB_FIRST(data);
+	    entry->value.u.intval = toint(GET_32BIT_MSB_FIRST(data));
 	    data += 4, maxsize -= 4;
 	    break;
 	  case TYPE_STR:

import.c

@@ -289,7 +289,7 @@ static int ssh2_read_mpint(void *data, int len, struct mpint_pos *ret)
 
     if (len < 4)
         goto error;
-    bytes = GET_32BIT(d);
+    bytes = toint(GET_32BIT(d));
     if (bytes < 0 || len-4 < bytes)
         goto error;
 
@@ -745,6 +745,10 @@ int openssh_write(const Filename *filename, struct ssh2_userkey *key,
         struct mpint_pos n, e, d, p, q, iqmp, dmp1, dmq1;
         Bignum bd, bp, bq, bdmp1, bdmq1;
 
+        /*
+         * These blobs were generated from inside PuTTY, so we needn't
+         * treat them as untrusted.
+         */
         pos = 4 + GET_32BIT(pubblob);
         pos += ssh2_read_mpint(pubblob+pos, publen-pos, &e);
         pos += ssh2_read_mpint(pubblob+pos, publen-pos, &n);
@@ -798,6 +802,10 @@ int openssh_write(const Filename *filename, struct ssh2_userkey *key,
         int pos;
         struct mpint_pos p, q, g, y, x;
 
+        /*
+         * These blobs were generated from inside PuTTY, so we needn't
+         * treat them as untrusted.
+         */
         pos = 4 + GET_32BIT(pubblob);
         pos += ssh2_read_mpint(pubblob+pos, publen-pos, &p);
         pos += ssh2_read_mpint(pubblob+pos, publen-pos, &q);
@@ -1216,11 +1224,12 @@ int sshcom_encrypted(const Filename *filename, char **comment)
     pos = 8;
     if (key->keyblob_len < pos+4)
         goto done;                     /* key is far too short */
-    pos += 4 + GET_32BIT(key->keyblob + pos);   /* skip key type */
-    if (key->keyblob_len < pos+4)
+    len = toint(GET_32BIT(key->keyblob + pos));
+    if (len < 0 || len > key->keyblob_len - pos - 4)
         goto done;                     /* key is far too short */
-    len = GET_32BIT(key->keyblob + pos);   /* find cipher-type length */
-    if (key->keyblob_len < pos+4+len)
+    pos += 4 + len;                    /* skip key type */
+    len = toint(GET_32BIT(key->keyblob + pos)); /* find cipher-type length */
+    if (len < 0 || len > key->keyblob_len - pos - 4)
         goto done;                     /* cipher type string is incomplete */
     if (len != 4 || 0 != memcmp(key->keyblob + pos + 4, "none", 4))
         answer = 1;
@@ -1236,8 +1245,7 @@ int sshcom_encrypted(const Filename *filename, char **comment)
 
 static int sshcom_read_mpint(void *data, int len, struct mpint_pos *ret)
 {
-    int bits;
-    int bytes;
+    unsigned bits, bytes;
     unsigned char *d = (unsigned char *) data;
 
     if (len < 4)
@@ -1309,7 +1317,8 @@ struct ssh2_userkey *sshcom_read(const Filename *filename, char *passphrase,
      */
     pos = 8;
     if (key->keyblob_len < pos+4 ||
-        (len = GET_32BIT(key->keyblob + pos)) > key->keyblob_len - pos - 4) {
+        (len = toint(GET_32BIT(key->keyblob + pos))) < 0 ||
+        len > key->keyblob_len - pos - 4) {
         errmsg = "key blob does not contain a key type string";
         goto error;
     }
@@ -1329,7 +1338,8 @@ struct ssh2_userkey *sshcom_read(const Filename *filename, char *passphrase,
      * Determine the cipher type.
      */
     if (key->keyblob_len < pos+4 ||
-        (len = GET_32BIT(key->keyblob + pos)) > key->keyblob_len - pos - 4) {
+        (len = toint(GET_32BIT(key->keyblob + pos))) < 0 ||
+        len > key->keyblob_len - pos - 4) {
         errmsg = "key blob does not contain a cipher type string";
         goto error;
     }
@@ -1347,7 +1357,8 @@ struct ssh2_userkey *sshcom_read(const Filename *filename, char *passphrase,
      * Get hold of the encrypted part of the key.
      */
     if (key->keyblob_len < pos+4 ||
-        (len = GET_32BIT(key->keyblob + pos)) > key->keyblob_len - pos - 4) {
+        (len = toint(GET_32BIT(key->keyblob + pos))) < 0 ||
+        len > key->keyblob_len - pos - 4) {
         errmsg = "key blob does not contain actual key data";
         goto error;
     }
@@ -1411,7 +1422,7 @@ struct ssh2_userkey *sshcom_read(const Filename *filename, char *passphrase,
     /*
      * Strip away the containing string to get to the real meat.
      */
-    len = GET_32BIT(ciphertext);
+    len = toint(GET_32BIT(ciphertext));
     if (len < 0 || len > cipherlen-4) {
         errmsg = "containing string was ill-formed";
         goto error;
@@ -1540,6 +1551,10 @@ int sshcom_write(const Filename *filename, struct ssh2_userkey *key,
         int pos;
         struct mpint_pos n, e, d, p, q, iqmp;
 
+        /*
+         * These blobs were generated from inside PuTTY, so we needn't
+         * treat them as untrusted.
+         */
         pos = 4 + GET_32BIT(pubblob);
         pos += ssh2_read_mpint(pubblob+pos, publen-pos, &e);
         pos += ssh2_read_mpint(pubblob+pos, publen-pos, &n);
@@ -1565,6 +1580,10 @@ int sshcom_write(const Filename *filename, struct ssh2_userkey *key,
         int pos;
         struct mpint_pos p, q, g, y, x;
 
+        /*
+         * These blobs were generated from inside PuTTY, so we needn't
+         * treat them as untrusted.
+         */
         pos = 4 + GET_32BIT(pubblob);
         pos += ssh2_read_mpint(pubblob+pos, publen-pos, &p);
         pos += ssh2_read_mpint(pubblob+pos, publen-pos, &q);

misc.c

@@ -208,6 +208,29 @@ void burnstr(char *string)             /* sfree(str), only clear it first */
     }
 }
 
+int toint(unsigned u)
+{
+    /*
+     * Convert an unsigned to an int, without running into the
+     * undefined behaviour which happens by the strict C standard if
+     * the value overflows. You'd hope that sensible compilers would
+     * do the sensible thing in response to a cast, but actually I
+     * don't trust modern compilers not to do silly things like
+     * assuming that _obviously_ you wouldn't have caused an overflow
+     * and so they can elide an 'if (i < 0)' test immediately after
+     * the cast.
+     *
+     * Sensible compilers ought of course to optimise this entire
+     * function into 'just return the input value'!
+     */
+    if (u <= (unsigned)INT_MAX)
+        return (int)u;
+    else if (u >= (unsigned)INT_MIN)   /* wrap in cast _to_ unsigned is OK */
+        return INT_MIN + (int)(u - (unsigned)INT_MIN);
+    else
+        return INT_MIN; /* fallback; should never occur on binary machines */
+}
+
 /*
  * Do an sprintf(), but into a custom-allocated buffer.
  * 

misc.h

@@ -30,6 +30,8 @@ char *dupprintf(const char *fmt, ...);
 char *dupvprintf(const char *fmt, va_list ap);
 void burnstr(char *string);
 
+int toint(unsigned);
+
 char *fgetline(FILE *fp);
 
 void base64_encode_atom(unsigned char *data, int n, char *out);

sftp.c

@@ -150,7 +150,7 @@ static int sftp_pkt_getstring(struct sftp_packet *pkt,
     *p = NULL;
     if (pkt->length - pkt->savedpos < 4)
 	return 0;
-    *length = GET_32BIT(pkt->data + pkt->savedpos);
+    *length = toint(GET_32BIT(pkt->data + pkt->savedpos));
     pkt->savedpos += 4;
     if ((int)(pkt->length - pkt->savedpos) < *length || *length < 0) {
 	*length = 0;