Remove OPENSSL_cleanse, BN_clear_free, etc.

Ultimately, it's better to invest effort in alternative forms of
protection of key material.

Calling `OPENSSL_cleanse` with a NULL pointer is not safe, but
`OPENSSL_cleanse` is often called in cleanup code, especially error-
handling code, where it is difficult to keep track of the NULLness of
things. The likelihood of getting this wrong is compounded by the fact
that, in OpenSSL upstream, calling `OPENSSL_cleanse(NULL, x)` for any
`x` is safe (a no-op). BoringSSL upstream doesn't want to change its
`OPENSSL_cleanse` to work like OpenSSL's. We don't want to worry about
the issue.

Apart from that, by inspection, it is clear that there are many places
in the code that don't call `OPENSSL_clease` where they "should". It
would be difficult to find all the places where a call to
`OPENSSL_clease` "should" be inserted. It is unlikely we'll ever get it
right. Actually, it's basically impossible to get it right using this
coding pattern. See
http://www.daemonology.net/blog/2014-09-06-zeroing-buffers-is-insufficient.html
and bitcoin-core/secp256k1#185.

Besides all that, the zeroization isn't free. Especially in the case of
non-MSVC platforms, it either interferes with the optimizer or it
doesn't work. More importantly, thinking about how to make this
approach work wastes a lot of time that could be spent actually
improving the fundementals of the security of the code.

crypto/bn/bn.c

@@ -99,27 +99,6 @@ void BN_free(BIGNUM *bn) {
   }
 }
 
-void BN_clear_free(BIGNUM *bn) {
-  char should_free;
-
-  if (bn == NULL) {
-    return;
-  }
-
-  if (bn->d != NULL) {
-    OPENSSL_cleanse(bn->d, bn->dmax * sizeof(bn->d[0]));
-    if ((bn->flags & BN_FLG_STATIC_DATA) == 0) {
-      OPENSSL_free(bn->d);
-    }
-  }
-
-  should_free = (bn->flags & BN_FLG_MALLOCED) != 0;
-  OPENSSL_cleanse(bn, sizeof(BIGNUM));
-  if (should_free) {
-    OPENSSL_free(bn);
-  }
-}
-
 BIGNUM *BN_dup(const BIGNUM *src) {
   BIGNUM *copy;
 
@@ -156,15 +135,6 @@ BIGNUM *BN_copy(BIGNUM *dest, const BIGNUM *src) {
   return dest;
 }
 
-void BN_clear(BIGNUM *bn) {
-  if (bn->d != NULL) {
-    memset(bn->d, 0, bn->dmax * sizeof(bn->d[0]));
-  }
-
-  bn->top = 0;
-  bn->neg = 0;
-}
-
 const BIGNUM *BN_value_one(void) {
   static const BN_ULONG kOneLimbs[1] = { 1 };
   STATIC_BIGNUM_DIAGNOSTIC_PUSH

crypto/bn/ctx.c

@@ -244,7 +244,7 @@ static void BN_POOL_finish(BN_POOL *p) {
     BIGNUM *bn = p->head->vals;
     while (loop++ < BN_CTX_POOL_SIZE) {
       if (bn->d) {
-        BN_clear_free(bn);
+        BN_free(bn);
       }
       bn++;
     }

crypto/bn/exponentiation.c

@@ -738,10 +738,7 @@ int BN_mod_exp_mont_consttime(BIGNUM *rr, const BIGNUM *a, const BIGNUM *p,
 
 err:
   BN_MONT_CTX_free(new_mont);
-  if (powerbuf != NULL) {
-    OPENSSL_cleanse(powerbuf, powerbufLen);
-    OPENSSL_free(powerbufFree);
-  }
+  OPENSSL_free(powerbufFree);
   BN_CTX_end(ctx);
   return (ret);
 }

crypto/bn/random.c

@@ -172,10 +172,7 @@ int BN_rand(BIGNUM *rnd, int bits, int top, int bottom) {
   ret = 1;
 
 err:
-  if (buf != NULL) {
-    OPENSSL_cleanse(buf, bytes);
-    OPENSSL_free(buf);
-  }
+  OPENSSL_free(buf);
   return (ret);
 }
 

crypto/bn/rsaz_exp.c

@@ -247,8 +247,6 @@ void RSAZ_1024_mod_exp_avx2(BN_ULONG result_norm[16],
 	rsaz_1024_mul_avx2(result, result, one, m, k0);
 
 	rsaz_1024_red2norm_avx2(result_norm, result);
-
-	OPENSSL_cleanse(storage,sizeof(storage));
 }
 
 #endif  /* OPENSSL_X86_64 */

crypto/ec/ec_key.c

@@ -104,9 +104,8 @@ void EC_KEY_free(EC_KEY *r) {
   }
 
   EC_POINT_free(r->pub_key);
-  BN_clear_free(r->priv_key);
+  BN_free(r->priv_key);
 
-  OPENSSL_cleanse((void *)r, sizeof(EC_KEY));
   OPENSSL_free(r);
 }
 

crypto/ec/internal.h

@@ -167,7 +167,6 @@ int ec_wNAF_mul_public(const EC_GROUP *group, EC_POINT *r,
 unsigned ec_GFp_simple_group_get_degree(const EC_GROUP *);
 int ec_GFp_simple_point_init(EC_POINT *);
 void ec_GFp_simple_point_finish(EC_POINT *);
-void ec_GFp_simple_point_clear_finish(EC_POINT *);
 int ec_GFp_simple_point_copy(EC_POINT *, const EC_POINT *);
 int ec_GFp_simple_point_set_to_infinity(const EC_GROUP *, EC_POINT *);
 int ec_GFp_simple_set_Jprojective_coordinates_GFp(const EC_GROUP *, EC_POINT *,

crypto/ec/simple.c

@@ -107,12 +107,6 @@ void ec_GFp_simple_point_finish(EC_POINT *point) {
   BN_free(&point->Z);
 }
 
-void ec_GFp_simple_point_clear_finish(EC_POINT *point) {
-  BN_clear_free(&point->X);
-  BN_clear_free(&point->Y);
-  BN_clear_free(&point->Z);
-}
-
 int ec_GFp_simple_point_copy(EC_POINT *dest, const EC_POINT *src) {
   if (!BN_copy(&dest->X, &src->X) ||
       !BN_copy(&dest->Y, &src->Y) ||
@@ -988,7 +982,7 @@ int ec_GFp_simple_points_make_affine(const EC_GROUP *group, size_t num,
       if (prod_Z[i] == NULL) {
         break;
       }
-      BN_clear_free(prod_Z[i]);
+      BN_free(prod_Z[i]);
     }
     OPENSSL_free(prod_Z);
   }

crypto/ec/wnaf.c

@@ -496,8 +496,7 @@ int ec_wNAF_mul_public(const EC_GROUP *group, EC_POINT *r,
   }
   if (val != NULL) {
     for (v = val; *v != NULL; v++) {
-      ec_GFp_simple_point_clear_finish(*v);
-      OPENSSL_cleanse(*v, sizeof **v);
+      ec_GFp_simple_point_finish(*v);
       OPENSSL_free(*v);
     }
 

crypto/mem.c

@@ -54,36 +54,8 @@
  * copied and put under another distribution licence
  * [including the GNU Public Licence.] */
 
-#if !defined(_POSIX_C_SOURCE)
-#define _POSIX_C_SOURCE 201410L  /* needed for strdup, snprintf, vprintf etc */
-#endif
-
 #include <openssl/mem.h>
 
-#if defined(OPENSSL_WINDOWS)
-#pragma warning(push, 3)
-#include <windows.h>
-#pragma warning(pop)
-#else
-#include <string.h>
-#endif
-
-
-void OPENSSL_cleanse(void *ptr, size_t len) {
-#if defined(OPENSSL_WINDOWS)
-  SecureZeroMemory(ptr, len);
-#else
-  memset(ptr, 0, len);
-
-#if !defined(OPENSSL_NO_ASM)
-  /* As best as we can tell, this is sufficient to break any optimisations that
-     might try to eliminate "superfluous" memsets. If there's an easy way to
-     detect memset_s, it would be better to use that. */
-  __asm__ __volatile__("" : : "r"(ptr) : "memory");
-#endif
-#endif  /* !OPENSSL_NO_ASM */
-}
-
 int CRYPTO_memcmp(const void *in_a, const void *in_b, size_t len) {
   size_t i;
   const uint8_t *a = in_a;

crypto/modes/gcm.c

@@ -1116,10 +1116,7 @@ void CRYPTO_gcm128_tag(GCM128_CONTEXT *ctx, unsigned char *tag,
 }
 
 void CRYPTO_gcm128_release(GCM128_CONTEXT *ctx) {
-  if (ctx) {
-    OPENSSL_cleanse(ctx, sizeof(*ctx));
-    OPENSSL_free(ctx);
-  }
+  OPENSSL_free(ctx);
 }
 
 #if defined(OPENSSL_X86) || defined(OPENSSL_X86_64)

crypto/rand/rand.c

@@ -64,11 +64,6 @@ static const uint64_t kMaxBytesPerRefresh = 1024 * 1024;
 /* rand_thread_state_free frees a |rand_thread_state|. This is called when a
  * thread exits. */
 static void rand_thread_state_free(void *state) {
-  if (state == NULL) {
-    return;
-  }
-
-  OPENSSL_cleanse(state, sizeof(struct rand_thread_state));
   OPENSSL_free(state);
 }
 

crypto/rsa/rsa.c

@@ -131,14 +131,14 @@ void RSA_free(RSA *rsa) {
     return;
   }
 
-  BN_clear_free(rsa->n);
-  BN_clear_free(rsa->e);
-  BN_clear_free(rsa->d);
-  BN_clear_free(rsa->p);
-  BN_clear_free(rsa->q);
-  BN_clear_free(rsa->dmp1);
-  BN_clear_free(rsa->dmq1);
-  BN_clear_free(rsa->iqmp);
+  BN_free(rsa->n);
+  BN_free(rsa->e);
+  BN_free(rsa->d);
+  BN_free(rsa->p);
+  BN_free(rsa->q);
+  BN_free(rsa->dmp1);
+  BN_free(rsa->dmq1);
+  BN_free(rsa->iqmp);
   BN_MONT_CTX_free(rsa->mont_n);
   BN_MONT_CTX_free(rsa->mont_p);
   BN_MONT_CTX_free(rsa->mont_q);

crypto/rsa/rsa_impl.c

@@ -186,11 +186,7 @@ int RSA_encrypt(RSA *rsa, size_t *out_len, uint8_t *out, size_t max_out,
     BN_CTX_end(ctx);
     BN_CTX_free(ctx);
   }
-  if (buf != NULL) {
-    OPENSSL_cleanse(buf, rsa_size);
-    OPENSSL_free(buf);
-  }
-
+  OPENSSL_free(buf);
   return ret;
 }
 
@@ -342,11 +338,7 @@ int RSA_sign_raw(RSA *rsa, size_t *out_len, uint8_t *out, size_t max_out,
   ret = 1;
 
 err:
-  if (buf != NULL) {
-    OPENSSL_cleanse(buf, rsa_size);
-    OPENSSL_free(buf);
-  }
-
+  OPENSSL_free(buf);
   return ret;
 }
 
@@ -410,8 +402,7 @@ int RSA_decrypt(RSA *rsa, size_t *out_len, uint8_t *out, size_t max_out,
   }
 
 err:
-  if (padding != RSA_NO_PADDING && buf != NULL) {
-    OPENSSL_cleanse(buf, rsa_size);
+  if (padding != RSA_NO_PADDING) {
     OPENSSL_free(buf);
   }
 
@@ -521,8 +512,7 @@ int rsa_verify_raw(RSA *rsa, size_t *out_len, uint8_t *out, size_t max_out,
     BN_CTX_end(ctx);
     BN_CTX_free(ctx);
   }
-  if (padding != RSA_NO_PADDING && buf != NULL) {
-    OPENSSL_cleanse(buf, rsa_size);
+  if (padding != RSA_NO_PADDING) {
     OPENSSL_free(buf);
   }
   return ret;

include/openssl/bn.h

@@ -174,10 +174,6 @@ OPENSSL_EXPORT void BN_init(BIGNUM *bn);
  * allocated on the heap, frees |bn| also. */
 OPENSSL_EXPORT void BN_free(BIGNUM *bn);
 
-/* BN_clear_free erases and frees the data referenced by |bn| and, if |bn| was
- * originally allocated on the heap, frees |bn| also. */
-OPENSSL_EXPORT void BN_clear_free(BIGNUM *bn);
-
 /* BN_dup allocates a new BIGNUM and sets it equal to |src|. It returns the
  * allocated BIGNUM on success or NULL otherwise. */
 OPENSSL_EXPORT BIGNUM *BN_dup(const BIGNUM *src);
@@ -186,9 +182,6 @@ OPENSSL_EXPORT BIGNUM *BN_dup(const BIGNUM *src);
  * failure. */
 OPENSSL_EXPORT BIGNUM *BN_copy(BIGNUM *dest, const BIGNUM *src);
 
-/* BN_clear sets |bn| to zero and erases the old data. */
-OPENSSL_EXPORT void BN_clear(BIGNUM *bn);
-
 /* BN_value_one returns a static BIGNUM with value 1. */
 OPENSSL_EXPORT const BIGNUM *BN_value_one(void);
 

include/openssl/mem.h

@@ -78,10 +78,6 @@ extern "C" {
 #define OPENSSL_realloc realloc
 #define OPENSSL_free free
 
-/* OPENSSL_cleanse zeros out |len| bytes of memory at |ptr|. This is similar to
- * |memset_s| from C11. */
-OPENSSL_EXPORT void OPENSSL_cleanse(void *ptr, size_t len);
-
 /* CRYPTO_memcmp returns zero iff the |len| bytes at |a| and |b| are equal. It
  * takes an amount of time dependent on |len|, but independent of the contents
  * of |a| and |b|. Unlike memcmp, it cannot be used to put elements into a