Skip to content
This repository has been archived by the owner on Apr 19, 2024. It is now read-only.

Commit

Permalink
Tighten up a lot of casts from unsigned to int which are read by one
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
simon committed Jul 14, 2013
1 parent 2661129 commit fb488f2
Show file tree
Hide file tree
Showing 11 changed files with 185 additions and 70 deletions.
9 changes: 5 additions & 4 deletions conf.c
View file
Expand Up @@ -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);
Expand All @@ -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:
Expand All @@ -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:
Expand Down
41 changes: 30 additions & 11 deletions import.c
View file
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand All @@ -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)
Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
}
Expand All @@ -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;
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down
23 changes: 23 additions & 0 deletions misc.c
View file
Expand Up @@ -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.
*
Expand Down
2 changes: 2 additions & 0 deletions misc.h
View file
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion sftp.c
View file
Expand Up @@ -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;
Expand Down

0 comments on commit fb488f2

Please sign in to comment.