bignum: Make bignum_free safer

Signed-off-by: Aris Adamantiadis <aris@0xbadc0de.be>
Reviewed-by: Andreas Schneider <asn@cryptomilk.org>

include/libssh/libcrypto.h

@@ -67,7 +67,12 @@ typedef BIGNUM*  bignum;
 typedef BN_CTX* bignum_CTX;
 
 #define bignum_new() BN_new()
-#define bignum_free(num) BN_clear_free(num)
+#define bignum_safe_free(num) do { \
+    if ((num) != NULL) { \
+        BN_clear_free((num)); \
+        (num)=NULL; \
+    } \
+    } while(0)
 #define bignum_set_word(bn,n) BN_set_word(bn,n)
 #define bignum_bin2bn(bn,datalen,data) BN_bin2bn(bn,datalen,data)
 #define bignum_bn2dec(num) BN_bn2dec(num)

include/libssh/libgcrypt.h

@@ -61,7 +61,12 @@ int ssh_gcry_dec2bn(bignum *bn, const char *data);
 char *ssh_gcry_bn2dec(bignum bn);
 
 #define bignum_new() gcry_mpi_new(0)
-#define bignum_free(num) gcry_mpi_release(num)
+#define bignum_safe_free(num) do { \
+    if ((num) != NULL) { \
+        gcry_mpi_release((num)); \
+        (num)=NULL; \
+    } \
+    } while (0)
 #define bignum_set_word(bn,n) gcry_mpi_set_ui(bn,n)
 #define bignum_bin2bn(bn,datalen,data) gcry_mpi_scan(data,GCRYMPI_FMT_USG,bn,datalen,NULL)
 #define bignum_bn2dec(num) ssh_gcry_bn2dec(num)

include/libssh/libmbedcrypto.h

@@ -78,7 +78,12 @@ int ssh_mbedcry_rand(bignum rnd, int bits, int top, int bottom);
 int ssh_mbedcry_is_bit_set(bignum num, size_t pos);
 
 #define bignum_new() ssh_mbedcry_bn_new()
-#define bignum_free(num) ssh_mbedcry_bn_free(num);
+#define bignum_safe_free(num) do { \
+    if ((num) != NULL) { \
+        ssh_mbedcry_bn_free(num); \
+        (num)=NULL; \
+    } \
+    } while(0)
 #define bignum_set_word(bn, n) mbedtls_mpi_lset(bn, n) /* TODO fix
                                                           overflow/underflow */
 #define bignum_bin2bn(data, datalen, bn) mbedtls_mpi_read_binary(bn, data, \

src/dh.c

@@ -142,33 +142,31 @@ int ssh_dh_init(void)
 #if defined(HAVE_LIBGCRYPT)
     bignum_bin2bn(p_group1_value, P_GROUP1_LEN, &p_group1);
     if (p_group1 == NULL) {
-        bignum_free(g);
-        g = NULL;
-        return -1;
+        bignum_safe_free(g);
+
+        return SSH_ERROR;
     }
     bignum_bin2bn(p_group14_value, P_GROUP14_LEN, &p_group14);
     if (p_group14 == NULL) {
-        bignum_free(g);
-        bignum_free(p_group1);
-        g = NULL;
-        p_group1 = NULL;
-        return -1;
+        bignum_safe_free(g);
+        bignum_safe_free(p_group1);
+
+        return SSH_ERROR;
     }
 #elif defined(HAVE_LIBCRYPTO)
     p_group1 = bignum_new();
     if (p_group1 == NULL) {
-        bignum_free(g);
-        g = NULL;
-        return -1;
+        bignum_safe_free(g);
+
+        return SSH_ERROR;
     }
     bignum_bin2bn(p_group1_value, P_GROUP1_LEN, p_group1);
 
     p_group14 = bignum_new();
     if (p_group14 == NULL) {
-        bignum_free(g);
-        bignum_free(p_group1);
-        g = NULL;
-        p_group1 = NULL;
+        bignum_safe_free(g);
+        bignum_safe_free(p_group1);
+
         return SSH_ERROR;
     }
     bignum_bin2bn(p_group14_value, P_GROUP14_LEN, p_group14);
@@ -194,12 +192,9 @@ void ssh_dh_finalize(void)
         return;
     }
 
-    bignum_free(g);
-    g = NULL;
-    bignum_free(p_group1);
-    p_group1 = NULL;
-    bignum_free(p_group14);
-    p_group14 = NULL;
+    bignum_safe_free(g);
+    bignum_safe_free(p_group1);
+    bignum_safe_free(p_group14);
 
     dh_crypto_initialized = 0;
 }

src/gcrypt_missing.c

@@ -72,7 +72,7 @@ char *ssh_gcry_bn2dec(bignum bn) {
     num = bignum_new();
     if (num == NULL) {
       SAFE_FREE(ret);
-      bignum_free(ten);
+      bignum_safe_free(ten);
       return NULL;
     }
 
@@ -91,9 +91,9 @@ char *ssh_gcry_bn2dec(bignum bn) {
       ret[count2] = ret[count2 + count];
     }
     ret[count2] = 0;
-    bignum_free(num);
-    bignum_free(bndup);
-    bignum_free(ten);
+    bignum_safe_free(num);
+    bignum_safe_free(bndup);
+    bignum_safe_free(ten);
   }
 
   return ret;

src/pki.c

@@ -370,8 +370,8 @@ void ssh_signature_free(ssh_signature sig)
 #elif defined(HAVE_LIBCRYPTO) && defined(HAVE_OPENSSL_ECC)
             ECDSA_SIG_free(sig->ecdsa_sig);
 #elif defined HAVE_LIBMBEDCRYPTO
-            bignum_free(sig->ecdsa_sig.r);
-            bignum_free(sig->ecdsa_sig.s);
+            bignum_safe_free(sig->ecdsa_sig.r);
+            bignum_safe_free(sig->ecdsa_sig.s);
 #endif
             break;
         case SSH_KEYTYPE_ED25519:

src/pki_gcrypt.c

@@ -1321,19 +1321,19 @@ static int _bignum_cmp(const gcry_sexp_t s1,
 
     sexp = gcry_sexp_find_token(s2, what, 0);
     if (sexp == NULL) {
-        bignum_free(b1);
+        bignum_safe_free(b1);
         return 1;
     }
     b2 = gcry_sexp_nth_mpi(sexp, 1, GCRYMPI_FMT_USG);
     gcry_sexp_release(sexp);
     if (b2 == NULL) {
-        bignum_free(b1);
+        bignum_safe_free(b1);
         return 1;
     }
 
     result = !! bignum_cmp(b1, b2);
-    bignum_free(b1);
-    bignum_free(b2);
+    bignum_safe_free(b1);
+    bignum_safe_free(b2);
     return result;
 }
 

src/pki_mbedcrypto.c

@@ -1036,7 +1036,7 @@ ssh_signature pki_do_sign(const ssh_key privkey, const unsigned char *hash,
 
             sig->ecdsa_sig.s = bignum_new();
             if (sig->ecdsa_sig.s == NULL) {
-                bignum_free(sig->ecdsa_sig.r);
+                bignum_safe_free(sig->ecdsa_sig.r);
                 return NULL;
             }
 
@@ -1094,7 +1094,7 @@ ssh_signature pki_do_sign_sessionid(const ssh_key key, const unsigned char
 
             sig->ecdsa_sig.s = bignum_new();
             if (sig->ecdsa_sig.s == NULL) {
-                bignum_free(sig->ecdsa_sig.r);
+                bignum_safe_free(sig->ecdsa_sig.r);
                 return NULL;
             }
 

src/wrapper.c

@@ -157,11 +157,11 @@ void crypto_free(struct ssh_crypto_struct *crypto){
   cipher_free(crypto->in_cipher);
   cipher_free(crypto->out_cipher);
 
-  bignum_free(crypto->e);
-  bignum_free(crypto->f);
-  bignum_free(crypto->x);
-  bignum_free(crypto->y);
-  bignum_free(crypto->k);
+  bignum_safe_free(crypto->e);
+  bignum_safe_free(crypto->f);
+  bignum_safe_free(crypto->x);
+  bignum_safe_free(crypto->y);
+  bignum_safe_free(crypto->k);
 #ifdef HAVE_ECDH
   SAFE_FREE(crypto->ecdh_client_pubkey);
   SAFE_FREE(crypto->ecdh_server_pubkey);