diff keyimport.c @ 1316:2c9dac2d6707

merge 2016.74
author Matt Johnston <matt@ucc.asn.au>
date Thu, 21 Jul 2016 23:38:42 +0800
parents 750ec4ec4cbe 8678e2cc1e53
children 77c0d57a4410
line wrap: on
line diff
--- a/keyimport.c	Sun Jun 19 20:38:38 2016 +0800
+++ b/keyimport.c	Thu Jul 21 23:38:42 2016 +0800
@@ -62,6 +62,8 @@
 static int dropbear_write(const char*filename, sign_key * key);
 static sign_key *dropbear_read(const char* filename);
 
+static int toint(unsigned u);
+
 #if 0
 static int sshcom_encrypted(const char *filename, char **comment);
 static struct ssh2_userkey *sshcom_read(const char *filename, char *passphrase);
@@ -243,12 +245,11 @@
 	if ((*p & 0x1F) == 0x1F) {
 		*id = 0;
 		while (*p & 0x80) {
-			*id = (*id << 7) | (*p & 0x7F);
 			p++, sourcelen--;
 			if (sourcelen == 0)
 				return -1;
+			*id = (*id << 7) | (*p & 0x7F);
 		}
-		*id = (*id << 7) | (*p & 0x7F);
 		p++, sourcelen--;
 	} else {
 		*id = *p & 0x1F;
@@ -259,19 +260,26 @@
 		return -1;
 
 	if (*p & 0x80) {
+		unsigned len;
 		int n = *p & 0x7F;
 		p++, sourcelen--;
 		if (sourcelen < n)
 			return -1;
-		*length = 0;
+		len = 0;
 		while (n--)
-			*length = (*length << 8) | (*p++);
+			len = (len << 8) | (*p++);
 		sourcelen -= n;
+		*length = toint(len);
 	} else {
 		*length = *p;
 		p++, sourcelen--;
 	}
 
+	if (*length < 0) {
+		printf("Negative ASN.1 length\n");
+		return -1;
+	}
+
 	return p - (unsigned char *) source;
 }
 
@@ -469,7 +477,7 @@
 	m_burn(buffer, sizeof(buffer));
 	return ret;
 
-	error:
+error:
 	m_burn(buffer, sizeof(buffer));
 	if (ret) {
 		if (ret->keyblob) {
@@ -584,8 +592,9 @@
 	/* Expect the SEQUENCE header. Take its absence as a failure to decrypt. */
 	ret = ber_read_id_len(p, key->keyblob_len, &id, &len, &flags);
 	p += ret;
-	if (ret < 0 || id != 16) {
-		errmsg = "ASN.1 decoding failure - wrong password?";
+	if (ret < 0 || id != 16 || len < 0 ||
+		key->keyblob+key->keyblob_len-p < len) {
+				errmsg = "ASN.1 decoding failure";
 		goto error;
 	}
 
@@ -619,7 +628,7 @@
 		ret = ber_read_id_len(p, key->keyblob+key->keyblob_len-p,
 							  &id, &len, &flags);
 		p += ret;
-		if (ret < 0 || id != 2 ||
+		if (ret < 0 || id != 2 || len < 0 ||
 			key->keyblob+key->keyblob_len-p < len) {
 			errmsg = "ASN.1 decoding failure";
 			goto error;
@@ -685,7 +694,7 @@
 							  &id, &len, &flags);
 		p += ret;
 		/* id==4 for octet string */
-		if (ret < 0 || id != 4 ||
+				if (ret < 0 || id != 4 || len < 0 ||
 			key->keyblob+key->keyblob_len-p < len) {
 			errmsg = "ASN.1 decoding failure";
 			goto error;
@@ -699,7 +708,7 @@
 							  &id, &len, &flags);
 		p += ret;
 		/* id==0 */
-		if (ret < 0 || id != 0) {
+				if (ret < 0 || id != 0 || len < 0) {
 			errmsg = "ASN.1 decoding failure";
 			goto error;
 		}
@@ -708,7 +717,7 @@
 							  &id, &len, &flags);
 		p += ret;
 		/* id==6 for object */
-		if (ret < 0 || id != 6 ||
+				if (ret < 0 || id != 6 || len < 0 ||
 			key->keyblob+key->keyblob_len-p < len) {
 			errmsg = "ASN.1 decoding failure";
 			goto error;
@@ -747,7 +756,7 @@
 							  &id, &len, &flags);
 		p += ret;
 		/* id==1 */
-		if (ret < 0 || id != 1) {
+				if (ret < 0 || id != 1 || len < 0) {
 			errmsg = "ASN.1 decoding failure";
 			goto error;
 		}
@@ -756,7 +765,7 @@
 							  &id, &len, &flags);
 		p += ret;
 		/* id==3 for bit string */
-		if (ret < 0 || id != 3 ||
+				if (ret < 0 || id != 3 || len < 0 ||
 			key->keyblob+key->keyblob_len-p < len) {
 			errmsg = "ASN.1 decoding failure";
 			goto error;
@@ -1381,7 +1390,7 @@
 			memset(ret->keyblob, 0, ret->keyblob_size);
 			m_free(ret->keyblob);
 		}
-		memset(&ret, 0, sizeof(ret));
+		memset(ret, 0, sizeof(*ret));
 		m_free(ret);
 	}
 	return NULL;
@@ -1409,11 +1418,12 @@
 	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;
@@ -1422,15 +1432,14 @@
 	*comment = dupstr(key->comment);
 	memset(key->keyblob, 0, key->keyblob_size);
 	m_free(key->keyblob);
-	memset(&key, 0, sizeof(key));
+	memset(key, 0, sizeof(*key));
 	m_free(key);
 	return answer;
 }
 
 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)
@@ -1483,7 +1492,7 @@
 	struct ssh2_userkey *ret = NULL, *retkey;
 	const struct ssh_signkey *alg;
 	unsigned char *blob = NULL;
-	int blobsize, publen, privlen;
+	int blobsize = 0, publen, privlen;
 
 	if (!key)
 		return NULL;
@@ -1603,8 +1612,8 @@
 	/*
 	 * Strip away the containing string to get to the real meat.
 	 */
-	len = GET_32BIT(ciphertext);
-	if (len > cipherlen-4) {
+	len = toint(GET_32BIT(ciphertext));
+	if (len < 0 || len > cipherlen-4) {
 		errmsg = "containing string was ill-formed";
 		goto error;
 	}
@@ -1671,7 +1680,8 @@
 		publen = pos;
 		pos += put_mp(blob+pos, x.start, x.bytes);
 		privlen = pos - publen;
-	}
+	} else
+		return NULL;
 
 	dropbear_assert(privlen > 0);			   /* should have bombed by now if not */
 
@@ -1695,7 +1705,7 @@
 	}
 	memset(key->keyblob, 0, key->keyblob_size);
 	m_free(key->keyblob);
-	memset(&key, 0, sizeof(key));
+	memset(key, 0, sizeof(*key));
 	m_free(key);
 	return ret;
 }
@@ -1908,3 +1918,27 @@
 	return ret;
 }
 #endif /* ssh.com stuff disabled */
+
+/* From PuTTY misc.c */
+static 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 */
+}