add m_mp_free_multi, be more careful freeing when failing to load keys

5 files changed, 41 insertions, 54 deletions

diff --git a/bignum.c b/bignum.c
index 3758052..b3e2b99 100644
--- a/bignum.c
+++ b/bignum.c
@@ -68,6 +68,22 @@ void m_mp_alloc_init_multi(mp_int **mp, ...)
 	va_end(args);
 }
 
+void m_mp_free_multi(mp_int **mp, ...) 
+{
+	mp_int** cur_arg = mp;
+	va_list args;
+
+	va_start(args, mp);        /* init args to next argument from caller */
+	while (cur_arg != NULL) {
+		if (*cur_arg) {
+			mp_clear(*cur_arg);
+		}
+		m_free(*cur_arg);
+		cur_arg = va_arg(args, mp_int**);
+	}
+	va_end(args);
+}
+
 void bytes_to_mp(mp_int *mp, const unsigned char* bytes, unsigned int len) {
 
 	if (mp_read_unsigned_bin(mp, (unsigned char*)bytes, len) != MP_OKAY) {
diff --git a/bignum.h b/bignum.h
index 59702c3..bab65ef 100644
--- a/bignum.h
+++ b/bignum.h
@@ -30,6 +30,7 @@
 void m_mp_init(mp_int *mp);
 void m_mp_init_multi(mp_int *mp, ...) ATTRIB_SENTINEL;
 void m_mp_alloc_init_multi(mp_int **mp, ...) ATTRIB_SENTINEL;
+void m_mp_free_multi(mp_int **mp, ...)  ATTRIB_SENTINEL;
 void bytes_to_mp(mp_int *mp, const unsigned char* bytes, unsigned int len);
 void hash_process_mp(const struct ltc_hash_descriptor *hash_desc, 
 				hash_state *hs, mp_int *mp);
diff --git a/dss.c b/dss.c
index 7754107..1b15cf2 100644
--- a/dss.c
+++ b/dss.c
@@ -44,6 +44,7 @@
  * These should be freed with dss_key_free.
  * Returns DROPBEAR_SUCCESS or DROPBEAR_FAILURE */
 int buf_get_dss_pub_key(buffer* buf, dropbear_dss_key *key) {
+	int ret = DROPBEAR_FAILURE;
 
 	TRACE(("enter buf_get_dss_pub_key"))
 	dropbear_assert(key != NULL);
@@ -56,17 +57,24 @@ int buf_get_dss_pub_key(buffer* buf, dropbear_dss_key *key) {
 	 || buf_getmpint(buf, key->g) == DROPBEAR_FAILURE
 	 || buf_getmpint(buf, key->y) == DROPBEAR_FAILURE) {
 		TRACE(("leave buf_get_dss_pub_key: failed reading mpints"))
-		return DROPBEAR_FAILURE;
+		ret = DROPBEAR_FAILURE;
+		goto out;
 	}
 
 	if (mp_count_bits(key->p) < MIN_DSS_KEYLEN) {
 		dropbear_log(LOG_WARNING, "DSS key too short");
 		TRACE(("leave buf_get_dss_pub_key: short key"))
-		return DROPBEAR_FAILURE;
+		ret = DROPBEAR_FAILURE;
+		goto out;
 	}
 
+	ret = DROPBEAR_SUCCESS;
 	TRACE(("leave buf_get_dss_pub_key: success"))
-	return DROPBEAR_SUCCESS;
+out:
+	if (ret == DROPBEAR_FAILURE) {
+		m_mp_free_multi(&key->p, &key->q, &key->g, &key->y, NULL);
+	}
+	return ret;
 }
 
 /* Same as buf_get_dss_pub_key, but reads a private "x" key at the end.
@@ -86,7 +94,7 @@ int buf_get_dss_priv_key(buffer* buf, dropbear_dss_key *key) {
 	m_mp_alloc_init_multi(&key->x, NULL);
 	ret = buf_getmpint(buf, key->x);
 	if (ret == DROPBEAR_FAILURE) {
-		m_free(key->x);
+		m_mp_free_multi(&key->x);
 	}
 
 	return ret;
@@ -101,26 +109,7 @@ void dss_key_free(dropbear_dss_key *key) {
 		TRACE2(("enter dsa_key_free: key == NULL"))
 		return;
 	}
-	if (key->p) {
-		mp_clear(key->p);
-		m_free(key->p);
-	}
-	if (key->q) {
-		mp_clear(key->q);
-		m_free(key->q);
-	}
-	if (key->g) {
-		mp_clear(key->g);
-		m_free(key->g);
-	}
-	if (key->y) {
-		mp_clear(key->y);
-		m_free(key->y);
-	}
-	if (key->x) {
-		mp_clear(key->x);
-		m_free(key->x);
-	}
+	m_mp_free_multi(&key->p, &key->q, &key->g, &key->y, &key->x, NULL);
 	m_free(key);
 	TRACE2(("leave dsa_key_free"))
 }
diff --git a/rsa.c b/rsa.c
index c09447f..1eac8ec 100644
--- a/rsa.c
+++ b/rsa.c
@@ -72,8 +72,7 @@ int buf_get_rsa_pub_key(buffer* buf, dropbear_rsa_key *key) {
 	ret = DROPBEAR_SUCCESS;
 out:
 	if (ret == DROPBEAR_FAILURE) {
-		m_free(key->e);
-		m_free(key->n);
+		m_mp_free_multi(&key->e, &key->n, NULL);
 	}
 	return ret;
 }
@@ -121,9 +120,7 @@ int buf_get_rsa_priv_key(buffer* buf, dropbear_rsa_key *key) {
 	ret = DROPBEAR_SUCCESS;
 out:
 	if (ret == DROPBEAR_FAILURE) {
-		m_free(key->d);
-		m_free(key->p);
-		m_free(key->q);
+		m_mp_free_multi(&key->d, &key->p, &key->q, NULL);
 	}
 	TRACE(("leave buf_get_rsa_priv_key"))
 	return ret;
@@ -139,26 +136,7 @@ void rsa_key_free(dropbear_rsa_key *key) {
 		TRACE2(("leave rsa_key_free: key == NULL"))
 		return;
 	}
-	if (key->d) {
-		mp_clear(key->d);
-		m_free(key->d);
-	}
-	if (key->e) {
-		mp_clear(key->e);
-		m_free(key->e);
-	}
-	if (key->n) {
-		 mp_clear(key->n);
-		 m_free(key->n);
-	}
-	if (key->p) {
-		mp_clear(key->p);
-		m_free(key->p);
-	}
-	if (key->q) {
-		mp_clear(key->q);
-		m_free(key->q);
-	}
+	m_mp_free_multi(&key->d, &key->e, &key->p, &key->q, &key->n, NULL);
 	m_free(key);
 	TRACE2(("leave rsa_key_free"))
 }
diff --git a/signkey.c b/signkey.c
index 2c29431..bc63632 100644
--- a/signkey.c
+++ b/signkey.c
@@ -167,7 +167,8 @@ int buf_get_pub_key(buffer *buf, sign_key *key, enum signkey_type *type) {
 		key->dsskey = m_malloc(sizeof(*key->dsskey));
 		ret = buf_get_dss_pub_key(buf, key->dsskey);
 		if (ret == DROPBEAR_FAILURE) {
-			m_free(key->dsskey);
+			dss_key_free(key->dsskey);
+			key->dsskey = NULL;
 		}
 	}
 #endif
@@ -177,7 +178,8 @@ int buf_get_pub_key(buffer *buf, sign_key *key, enum signkey_type *type) {
 		key->rsakey = m_malloc(sizeof(*key->rsakey));
 		ret = buf_get_rsa_pub_key(buf, key->rsakey);
 		if (ret == DROPBEAR_FAILURE) {
-			m_free(key->rsakey);
+			rsa_key_free(key->rsakey);
+			key->rsakey = NULL;
 		}
 	}
 #endif
@@ -201,7 +203,6 @@ int buf_get_pub_key(buffer *buf, sign_key *key, enum signkey_type *type) {
 	TRACE2(("leave buf_get_pub_key"))
 
 	return ret;
-	
 }
 
 /* returns DROPBEAR_SUCCESS on success, DROPBEAR_FAILURE on fail.
@@ -236,7 +237,8 @@ int buf_get_priv_key(buffer *buf, sign_key *key, enum signkey_type *type) {
 		key->dsskey = m_malloc(sizeof(*key->dsskey));
 		ret = buf_get_dss_priv_key(buf, key->dsskey);
 		if (ret == DROPBEAR_FAILURE) {
-			m_free(key->dsskey);
+			dss_key_free(key->dsskey);
+			key->dsskey = NULL;
 		}
 	}
 #endif
@@ -246,7 +248,8 @@ int buf_get_priv_key(buffer *buf, sign_key *key, enum signkey_type *type) {
 		key->rsakey = m_malloc(sizeof(*key->rsakey));
 		ret = buf_get_rsa_priv_key(buf, key->rsakey);
 		if (ret == DROPBEAR_FAILURE) {
-			m_free(key->rsakey);
+			rsa_key_free(key->rsakey);
+			key->rsakey = NULL;
 		}
 	}
 #endif