Introduce and use secure_memzero() to erase secrets

As described in trac #751, and shortly after reported by Zhaomo Yang, of the University of California, San Diego, we use memset() (often through the CLEAR() macro) to erase secrets after use. In some cases however, the compiler might optimize these calls away.

This patch replaces these memset() calls on secrets by calls to a new secure_memzero() function, that will not be optimized away.

Since we use CLEAR() a LOT of times, I'm not changing that to use secure_memzero() to prevent performance impact. I did annotate the macro to point people at secure_memzero().

This patch also replaces some CLEAR() or memset() calls with a zero- initialization using "= { 0 }" if that has the same effect, because that better captures the intend of that code.

Signed-off-by Steffan Karger <[email protected]...>

On Mon, Nov 28, 2016 at 7:13 AM, Steffan Karger <[email protected]... <mailto:[email protected]...>> wrote:

As described in trac #751, and shortly after reported by Zhaomo Yang, of the University of California, San Diego, we use memset() (often through the CLEAR() macro) to erase secrets after use. In some cases however, the
compiler might optimize these calls away.

This patch replaces these memset() calls on secrets by calls to a new secure_memzero() function, that will not be optimized away.

Since we use CLEAR() a LOT of times, I'm not changing that to use secure_memzero() to prevent performance impact. I did annotate the macro
to point people at secure_memzero().

This patch also replaces some CLEAR() or memset() calls with a zero- initialization using "= { 0 }" if that has the same effect, because that better captures the intend of that code.

Signed-off-by Steffan Karger <[email protected]...

<mailto:[email protected]...>>

All look good except that it might have missed some memset calls that could benefit from a SecureZeroMemory() or equivalent:

With no pretense of completeness, here is a list with some context:
calls marked with <-- SecureZero

src/openvpn/console_builtin.c +1 -1

@@ -218,7 +218,7 @@ static bool get_console_input (const char *prompt,
 const bool echo, char *input,
         if (gp)
         {
             strncpynt (input, gp, capacity);
-            memset (gp, 0, strlen (gp));
+            memset (gp, 0, strlen (gp)); <-- SecureZero
             ret = true;
         }

     }

src/openvpn/misc.c +1 -1

@@ -501,7 +501,7 @@ remove_env_item (const char *str, const bool
 do_free, struct env_item **list)
            *list = current->next;
          if (do_free)
            {
-             memset (current->string, 0, strlen (current->string));
+             memset (current->string, 0, strlen (current->string)); <--

SecureZero

              free (current->string);
              free (current);
            }

src/openvpn/ssl.c +1 -1

@@ -1212,7 +1212,7 @@ tls_multi_free (struct tls_multi *multi, bool clear)
 
   if (multi->auth_token)
     {
-      memset (multi->auth_token, 0, AUTH_TOKEN_SIZE);
+      memset (multi->auth_token, 0, AUTH_TOKEN_SIZE); <-- SecureZero
       free (multi->auth_token);
     }
 

src/openvpn/ssl_verify.c +2 -2

@@ -1176,7 +1176,7 @@ verify_user_pass(struct user_pass *up, struct
 tls_multi *multi,
       if (memcmp_constant_time(multi->auth_token, up->password,
                  strlen(multi->auth_token)) != 0)
         {
-          memset (multi->auth_token, 0, AUTH_TOKEN_SIZE);
+          memset (multi->auth_token, 0, AUTH_TOKEN_SIZE); <-- SecureZero
           free (multi->auth_token);
 

@@ -1262,7 +1262,7 @@ verify_user_pass(struct user_pass *up, struct
 tls_multi *multi,
                   "No auth-token will be activated now");
      if (multi->auth_token)
 {
- memset (multi->auth_token, 0, AUTH_TOKEN_SIZE);
+ memset (multi->auth_token, 0, AUTH_TOKEN_SIZE); <-- SecureZero
  free (multi->auth_token);
  multi->auth_token = NULL;

}

Just wanted to let you know that we have a new implementation of secure_memzero.h, which is available at
https://compsec.sysnet.ucsd.edu/secure_memzero.h
<https://compsec.sysnet.ucsd.edu/secure_memzero.h>.

The version I sent to you guys has a minor issue in dealing with memset_s. Also, this implementation synthesizes most of the existing working solutions for this problem. It allows you to specify, via macros, which implementation you prefer. This will let users override our defaults, if they want.

Hi,

Quoted 10 lines · Zhaomo Yang

Just wanted to let you know that we have a new implementation of secure_memzero.h, which is available at
https://compsec.sysnet.ucsd.edu/secure_memzero.h
<https://compsec.sysnet.ucsd.edu/secure_memzero.h>.

The version I sent to you guys has a minor issue in dealing with memset_s. Also, this implementation synthesizes most of the existing working solutions for this problem. It allows you to specify, via macros, which implementation you prefer. This will let users override our defaults, if they want.

Thanks, but I think I prefer the more light-weight approach you sent earlier (slightly molded to use static inline wrapper functions instead of macro's). It does what it needs to do for us, and is sufficiently small to easily integrate into existing code.

And thanks again for notifying us (on security@) on your findings.

I think we should prefer memset_s if available since it is a C11 standard.

Hi,

Quoted 1 line · Arne Schwabe

I think we should prefer memset_s if available since it is a C11 standard.

Since we're currently compiling against C99, we can't rely on it being there (in my OS Zoo, only MacOS seems to have it, at least "have a manpage for it" - not even FreeBSD 11 or "gentoo current") - but then, configure could check that for us...

Shall we merge the current patch anyway, as the memory barrier does the job on all current compilers, and add a memset_s() variant "whenever one of us finds time to add and test this"?

Am 29.11.16 um 11:18 schrieb Gert Doering:

Quoted 8 lines

Hi,

Quoted 1 line · Arne Schwabe

I think we should prefer memset_s if available since it is a C11 standard.

Since we're currently compiling against C99, we can't rely on it being there (in my OS Zoo, only MacOS seems to have it, at least "have a manpage for it" - not even FreeBSD 11 or "gentoo current") - but then, configure could check that for us...

I wouldn't add a configure test for that. According to
http://en.cppreference.com/w/c/string/byte/memset:

As all bounds-checked functions, |memset_s| is only guaranteed to be available if __STDC_LIB_EXT1__ is defined by the implementation and if the user defines __STDC_WANT_LIB_EXT1__ to the integer constant 1 before including |string.h|.

so preprocessor magic can check that.

As described in trac #751, and shortly after reported by Zhaomo Yang, of the University of California, San Diego, we use memset() (often through the CLEAR() macro) to erase secrets after use. In some cases however, the compiler might optimize these calls away.

This patch replaces these memset() calls on secrets by calls to a new secure_memzero() function, that will not be optimized away.

Since we use CLEAR() a LOT of times, I'm not changing that to use secure_memzero() to prevent performance impact. I did annotate the macro to point people at secure_memzero().

This patch also replaces some CLEAR() or memset() calls with a zero- initialization using "= { 0 }" if that has the same effect, because that better captures the intend of that code.

Signed-off-by Steffan Karger <[email protected]...>

---
v2: replace more memset()/CLEAR() calls with secure_memzero()

 src/openvpn/basic.h              |  2 +-
 src/openvpn/buffer.c             |  8 ++++----
 src/openvpn/buffer.h             | 43 ++++++++++++++++++++++++++++++++++++++++
 src/openvpn/console_builtin.c    |  2 +-
 src/openvpn/crypto.c             | 17 ++++++----------
 src/openvpn/manage.c             |  2 +-
 src/openvpn/misc.c               |  4 ++--
 src/openvpn/options.c            |  6 +++---
 src/openvpn/ssl.c                | 32 +++++++++++++++---------------
 src/openvpn/ssl_verify.c         |  4 ++--
 src/openvpn/ssl_verify_mbedtls.c |  4 +---
 11 files changed, 80 insertions(+), 44 deletions(-)

src/openvpn/basic.h +1 -1

index 298cf10..48d4d9b 100644
@@ -30,7 +30,7 @@
 /* size of an array */
 #define SIZE(x) (sizeof(x)/sizeof(x[0]))
 
-/* clear an object */
+/* clear an object (may be optimized away, use secure_memzero() to erase secrets) */
 #define CLEAR(x) memset(&(x), 0, sizeof(x))
 
 #define IPV4_NETMASK_HOST 0xffffffffU

src/openvpn/buffer.c +4 -4

index 75ccb7c..9a8cc17 100644
@@ -155,7 +155,9 @@ void
 buf_clear (struct buffer *buf)
 {
   if (buf->capacity > 0)
-    memset (buf->data, 0, buf->capacity);
+    {
+      secure_memzero (buf->data, buf->capacity);
+    }
   buf->len = 0;
   buf->offset = 0;
 }

@@ -624,9 +626,7 @@ string_clear (char *str)
 {
   if (str)
     {
-      const int len = strlen (str);
-      if (len > 0)
-	memset (str, 0, len);
+      secure_memzero (str, strlen (str));
     }
 }
 

src/openvpn/buffer.h +43 -0

index 3a07d53..e82811e 100644
@@ -333,6 +333,49 @@ has_digit (const unsigned char* src)
   return false;
 }
 
+/**
+ * Securely zeroise memory.
+ *
+ * This code and description are based on code supplied by Zhaomo Yang, of the
+ * University of California, San Diego (which was released into the public
+ * domain).
+ *
+ * The secure_memzero function attempts to ensure that an optimizing compiler
+ * does not remove the intended operation if cleared memory is not accessed
+ * again by the program. This code has been tested under Clang 3.9.0 and GCC
+ * 6.2 with optimization flags -O, -Os, -O0, -O1, -O2, and -O3 on
+ * Ubuntu 16.04.1 LTS; under Clang 3.9.0 with optimization flags -O, -Os,
+ * -O0, -O1, -O2, and -O3 on FreeBSD 10.2-RELEASE; under Microsoft Visual Studio
+ * 2015 with optimization flags /O1, /O2 and /Ox on Windows 10.
+ *
+ * Theory of operation:
+ *
+ * 1. On Windows, use the SecureZeroMemory which ensures that data is
+ *    overwritten.
+ * 2. Under GCC or Clang, use a memory barrier, which forces the preceding
+ *    memset to be carried out. The overhead of a memory barrier is usually
+ *    negligible.
+ * 3. If none of the above are available, use the volatile pointer
+ *    technique to zero memory one byte at a time.
+ *
+ * @param data	Pointer to data to zeroise.
+ * @param len	Length of data, in bytes.
+ */
+static inline void
+secure_memzero (void *data, size_t len)
+{
+#if defined(_WIN32)
+  SecureZeroMemory (data, len);
+#elif defined(__GNUC__) || defined(__clang__)
+  memset(data, 0, len);
+  __asm__ __volatile__("" : : "r"(data) : "memory");
+#else
+  volatile char *p = (volatile char *) data;
+  while (len--)
+    *p++ = 0;
+#endif
+}
+
 /*
  * printf append to a buffer with overflow check,
  * due to usage of vsnprintf, it will leave space for

src/openvpn/console_builtin.c +1 -1

index 6b0211d..06994fd 100644
@@ -218,7 +218,7 @@ static bool get_console_input (const char *prompt, const bool echo, char *input,
         if (gp)
         {
             strncpynt (input, gp, capacity);
-            memset (gp, 0, strlen (gp));
+            secure_memzero (gp, strlen (gp));
             ret = true;
         }
     }

src/openvpn/crypto.c +6 -11

index 05622ce..708cc92 100644
@@ -86,12 +86,11 @@ openvpn_encrypt_aead (struct buffer *buf, struct buffer work,
   {
     struct buffer iv_buffer;
     struct packet_id_net pin;
-    uint8_t iv[OPENVPN_MAX_IV_LENGTH];
+    uint8_t iv[OPENVPN_MAX_IV_LENGTH] = {0};
     const int iv_len = cipher_ctx_iv_length (ctx->cipher);
 
     ASSERT (iv_len >= OPENVPN_AEAD_MIN_IV_LEN && iv_len <= OPENVPN_MAX_IV_LENGTH);
 
-    memset(iv, 0, sizeof(iv));
     buf_set_write (&iv_buffer, iv, iv_len);
 
     /* IV starts with packet id to make the IV unique for packet */

@@ -175,7 +174,7 @@ openvpn_encrypt_v1 (struct buffer *buf, struct buffer work,
       /* Do Encrypt from buf -> work */
       if (ctx->cipher)
 	{
-	  uint8_t iv_buf[OPENVPN_MAX_IV_LENGTH];
+	  uint8_t iv_buf[OPENVPN_MAX_IV_LENGTH] = {0};
 	  const int iv_size = cipher_ctx_iv_length (ctx->cipher);
 	  const cipher_kt_t *cipher_kt = cipher_ctx_get_cipher_kt (ctx->cipher);
 	  int outlen;

@@ -190,8 +189,6 @@ openvpn_encrypt_v1 (struct buffer *buf, struct buffer work,
 
 	  if (cipher_kt_mode_cbc(cipher_kt))
 	    {
-	      CLEAR (iv_buf);
-
 	      /* generate pseudo-random IV */
 	      if (opt->flags & CO_USE_IV)
 		prng_bytes (iv_buf, iv_size);

@@ -214,7 +211,6 @@ openvpn_encrypt_v1 (struct buffer *buf, struct buffer work,
 	      ASSERT (packet_id_initialized(&opt->packet_id));
 
 	      packet_id_alloc_outgoing (&opt->packet_id.send, &pin, true);
-	      memset (iv_buf, 0, iv_size);
 	      buf_set_write (&b, iv_buf, iv_size);
 	      ASSERT (packet_id_write (&pin, &b, true, false));
 	    }

@@ -550,14 +546,13 @@ openvpn_decrypt_v1 (struct buffer *buf, struct buffer work,
 	{
 	  const int iv_size = cipher_ctx_iv_length (ctx->cipher);
 	  const cipher_kt_t *cipher_kt = cipher_ctx_get_cipher_kt (ctx->cipher);
-	  uint8_t iv_buf[OPENVPN_MAX_IV_LENGTH];
+	  uint8_t iv_buf[OPENVPN_MAX_IV_LENGTH] = { 0 };
 	  int outlen;
 
 	  /* initialize work buffer with FRAME_HEADROOM bytes of prepend capacity */
 	  ASSERT (buf_init (&work, FRAME_HEADROOM_ADJ (frame, FRAME_HEADROOM_MARKER_DECRYPT)));
 
 	  /* use IV if user requested it */
-	  CLEAR (iv_buf);
 	  if (opt->flags & CO_USE_IV)
 	    {
 	      if (buf->len < iv_size)

@@ -1128,7 +1123,7 @@ crypto_read_openvpn_key (const struct key_type *key_type,
   init_key_ctx (&ctx->decrypt, &key2.keys[kds.in_key], key_type,
 		OPENVPN_OP_DECRYPT, log_prefix);
 
-  CLEAR (key2);
+  secure_memzero (&key2, sizeof (key2));
 }
 
 /* header and footer for static key file */

@@ -1380,8 +1375,8 @@ write_key_file (const int nkeys, const char *filename)
       buf_printf (&out, "%s\n", fmt);
 
       /* zero memory which held key component (will be freed by GC) */
-      memset (fmt, 0, strlen(fmt));
-      CLEAR (key);
+      secure_memzero (fmt, strlen (fmt));
+      secure_memzero (&key, sizeof (key));
     }
 
   buf_printf (&out, "%s\n", static_key_foot);

src/openvpn/manage.c +1 -1

index 77a8006..4918ed2 100644
@@ -3154,7 +3154,7 @@ management_query_user_pass (struct management *man,
 	  man->connection.up_query.nocache = up->nocache; /* preserve caller's nocache setting */
 	  *up = man->connection.up_query;
 	}
-      CLEAR (man->connection.up_query);
+      secure_memzero (&man->connection.up_query, sizeof (man->connection.up_query));
     }
 
   gc_free (&gc);

src/openvpn/misc.c +2 -2

index 56d43e0..4e06c91 100644
@@ -501,7 +501,7 @@ remove_env_item (const char *str, const bool do_free, struct env_item **list)
 	    *list = current->next;
 	  if (do_free)
 	    {
-	      memset (current->string, 0, strlen (current->string));
+	      secure_memzero (current->string, strlen (current->string));
 	      free (current->string);
 	      free (current);
 	    }

@@ -1342,7 +1342,7 @@ purge_user_pass (struct user_pass *up, const bool force)
   static bool warn_shown = false;
   if (nocache || force)
     {
-      CLEAR (*up);
+      secure_memzero (up, sizeof(*up));
       up->nocache = nocache;
     }
   else if (!warn_shown)

src/openvpn/options.c +3 -3

index db0179d..38a8d2f 100644
@@ -3957,7 +3957,7 @@ read_inline_file (struct in_src *is, const char *close_tag, struct gc_arena *gc)
   ret = string_alloc (BSTR (&buf), gc);
   buf_clear (&buf);
   free_buf (&buf);
-  CLEAR (line);
+  secure_memzero (line, sizeof (line));
   return ret;
 }
 

@@ -4072,7 +4072,7 @@ read_config_file (struct options *options,
     {
       msg (msglevel, "In %s:%d: Maximum recursive include levels exceeded in include attempt of file %s -- probably you have a configuration file that tries to include itself.", top_file, top_line, file);
     }
-  CLEAR (line);
+  secure_memzero (line, sizeof (line));
   CLEAR (p);
 }
 

@@ -4104,7 +4104,7 @@ read_config_string (const char *prefix,
 	}
       CLEAR (p);
     }
-  CLEAR (line);
+  secure_memzero (line, sizeof (line));
 }
 
 void

src/openvpn/ssl.c +16 -16

index 4885031..14d1331 100644
@@ -894,7 +894,7 @@ key_state_free (struct key_state *ks, bool clear)
 #endif
 
   if (clear)
-    CLEAR (*ks);
+    secure_memzero (ks, sizeof (*ks));
 }
 
 /** @} name Functions for initialization and cleanup of key_state structures */

@@ -1024,7 +1024,7 @@ tls_session_free (struct tls_session *session, bool clear)
   cert_hash_free (session->cert_hash_set);
 
   if (clear)
-    CLEAR (*session);
+    secure_memzero (session, sizeof (*session));
 }
 
 /** @} name Functions for initialization and cleanup of tls_session structures */

@@ -1048,7 +1048,7 @@ move_session (struct tls_multi* multi, int dest, int src, bool reinit_src)
   if (reinit_src)
     tls_session_init (multi, &multi->session[src]);
   else
-    CLEAR (multi->session[src]);
+    secure_memzero (&multi->session[src], sizeof (multi->session[src]));
 
   dmsg (D_TLS_DEBUG, "TLS: move_session: exit");
 }

@@ -1212,7 +1212,7 @@ tls_multi_free (struct tls_multi *multi, bool clear)
 
   if (multi->auth_token)
     {
-      memset (multi->auth_token, 0, AUTH_TOKEN_SIZE);
+      secure_memzero (multi->auth_token, AUTH_TOKEN_SIZE);
       free (multi->auth_token);
     }
 

@@ -1222,7 +1222,7 @@ tls_multi_free (struct tls_multi *multi, bool clear)
     tls_session_free (&multi->session[i], false);
 
   if (clear)
-    CLEAR (*multi);
+    secure_memzero (multi, sizeof (*multi));
 
   free(multi);
 }

@@ -1512,7 +1512,7 @@ tls1_P_hash(const md_kt_t *md_kt,
     }
   hmac_ctx_cleanup(&ctx);
   hmac_ctx_cleanup(&ctx_tmp);
-  CLEAR (A1);
+  secure_memzero (A1, sizeof (A1));
 
   dmsg (D_SHOW_KEY_SOURCE, "tls1_P_hash out: %s", format_hex (out_orig, olen_orig, 0, &gc));
   gc_free (&gc);

@@ -1565,7 +1565,7 @@ tls1_PRF(const uint8_t *label,
   for (i=0; i<olen; i++)
     out1[i]^=out2[i];
 
-  memset (out2, 0, olen);
+  secure_memzero (out2, olen);
 
   dmsg (D_SHOW_KEY_SOURCE, "tls1_PRF out[%d]: %s", olen, format_hex (out1, olen, 0, &gc));
 

@@ -1702,8 +1702,8 @@ generate_key_expansion (struct key_ctx_bi *key,
   ret = true;
 
  exit:
-  CLEAR (master);
-  CLEAR (key2);
+  secure_memzero (&master, sizeof (master));
+  secure_memzero (&key2, sizeof (key2));
 
   return ret;
 }

@@ -1787,7 +1787,7 @@ tls_session_generate_data_channel_keys(struct tls_session *session)
 
   ret = true;
 cleanup:
-  CLEAR (*ks->key_src);
+  secure_memzero (ks->key_src, sizeof (*ks->key_src));
   return ret;
 }
 

@@ -2017,7 +2017,7 @@ key_method_1_write (struct buffer *buf, struct tls_session *session)
   init_key_ctx (&ks->crypto_options.key_ctx_bi.encrypt, &key,
 		&session->opt->key_type, OPENVPN_OP_ENCRYPT,
 		"Data Channel Encrypt");
-  CLEAR (key);
+  secure_memzero (&key, sizeof (key));
 
   /* send local options string */
   {

@@ -2202,7 +2202,7 @@ key_method_2_write (struct buffer *buf, struct tls_session *session)
 
  error:
   msg (D_TLS_ERRORS, "TLS Error: Key Method #2 write failed");
-  CLEAR (*ks->key_src);
+  secure_memzero (ks->key_src, sizeof (*ks->key_src));
   return false;
 }
 

@@ -2257,13 +2257,13 @@ key_method_1_read (struct buffer *buf, struct tls_session *session)
   init_key_ctx (&ks->crypto_options.key_ctx_bi.decrypt, &key,
 		&session->opt->key_type, OPENVPN_OP_DECRYPT,
 		"Data Channel Decrypt");
-  CLEAR (key);
+  secure_memzero (&key, sizeof (key));
   ks->authenticated = true;
   return true;
 
  error:
   buf_clear (buf);
-  CLEAR (key);
+  secure_memzero (&key, sizeof (key));
   return false;
 }
 

@@ -2377,7 +2377,7 @@ key_method_2_read (struct buffer *buf, struct tls_multi *multi, struct tls_sessi
     }
 
   /* clear username and password from memory */
-  CLEAR (*up);
+  secure_memzero (up, sizeof (*up));
 
   /* Perform final authentication checks */
   if (ks->authenticated)

@@ -2433,7 +2433,7 @@ key_method_2_read (struct buffer *buf, struct tls_multi *multi, struct tls_sessi
   return true;
 
  error:
-  CLEAR (*ks->key_src);
+  secure_memzero (ks->key_src, sizeof (*ks->key_src));
   buf_clear (buf);
   gc_free (&gc);
   return false;

src/openvpn/ssl_verify.c +2 -2

index a099776..4328828 100644
@@ -1176,7 +1176,7 @@ verify_user_pass(struct user_pass *up, struct tls_multi *multi,
       if (memcmp_constant_time(multi->auth_token, up->password,
                  strlen(multi->auth_token)) != 0)
         {
-          memset (multi->auth_token, 0, AUTH_TOKEN_SIZE);
+          secure_memzero (multi->auth_token, AUTH_TOKEN_SIZE);
           free (multi->auth_token);
           multi->auth_token = NULL;
           multi->auth_token_sent = false;

@@ -1262,7 +1262,7 @@ verify_user_pass(struct user_pass *up, struct tls_multi *multi,
                   "No auth-token will be activated now");
 	      if (multi->auth_token)
 		{
-		  memset (multi->auth_token, 0, AUTH_TOKEN_SIZE);
+		  secure_memzero (multi->auth_token, AUTH_TOKEN_SIZE);
 		  free (multi->auth_token);
 		  multi->auth_token = NULL;
 		}

src/openvpn/ssl_verify_mbedtls.c +1 -3

index 332f04b..4260823 100644
@@ -348,12 +348,10 @@ x509_setenv (struct env_set *es, int cert_depth, mbedtls_x509_crt *cert)
   int i;
   unsigned char c;
   const mbedtls_x509_name *name;
-  char s[128];
+  char s[128] = { 0 };
 
   name = &cert->subject;
 
-  memset( s, 0, sizeof( s ) );
-
   while( name != NULL )
     {
       char name_expand[64+8];

Quoted 3 lines · Steffan Karger

As described in trac #751, and shortly after reported by Zhaomo Yang, of the University of California, San Diego, we use memset() (often through the CLEAR() macro) to erase secrets after use. In some cases however,

the

Quoted 5 lines

compiler might optimize these calls away.

This patch replaces these memset() calls on secrets by calls to a new secure_memzero() function, that will not be optimized away.

Quoted 7 lines

Since we use CLEAR() a LOT of times, I'm not changing that to use secure_memzero() to prevent performance impact. I did annotate the macro to point people at secure_memzero().

This patch also replaces some CLEAR() or memset() calls with a zero- initialization using "= { 0 }" if that has the same effect, because that better captures the intend of that code.

First of all, wonderful that you've looked into an improved memset() approach. We need that! I am very happy to see these patches and this discussion.

But I am very reluctant to this approach here, to be honest. I do not like that we have a two semantics for a overlapping functions; one which always ensures memory is wiped (but might be slower) and the other one which might or might not wipe the memory (depending on the mood of the compiler, etc)

If we "do not care" if a memory region is wiped or not (just that it's nice if it happens), then I'd say these are code paths which *do* *not* require CLEAR() at all.

If we *know* a certain memory region *must* be cleared, then we do the proper call which does what it promises us.

I think we should not have "do not care" code paths, because that ambiguity gives us nothing but confusing complexity and such code paths bitrots faster than other type of code.

So I'd say that either we move over to secure_memzero() and yank out CLEAR() all together. Or we yank out CLEAR() calls where we "don't care" and let CLEAR() always do what it tries to promise through its name.

If we "do not care" if a memory region is wiped or not (just that it's nice if it happens), then I'd say these are code paths which *do* *not* require CLEAR() at all.

If we *know* a certain memory region *must* be cleared, then we do the proper call which does what it promises us.

As described in trac #751, and shortly after reported by Zhaomo Yang, of the University of California, San Diego, we use memset() (often through the CLEAR() macro) to erase secrets after use. In some cases however, the compiler might optimize these calls away.

This patch replaces these memset() calls on secrets by calls to a new secure_memzero() function, that will not be optimized away.

Since we use CLEAR() a LOT of times, I'm not changing that to use secure_memzero() to prevent performance impact. I did annotate the macro to point people at secure_memzero().

This patch also replaces some CLEAR() or memset() calls with a zero- initialization using "= { 0 }" if that has the same effect, because that better captures the intend of that code.

Signed-off-by Steffan Karger <[email protected]...>

---
v2: replace more memset()/CLEAR() calls with secure_memzero()

 src/openvpn/basic.h              |  2 +-
 src/openvpn/buffer.c             |  8 ++++----
 src/openvpn/buffer.h             | 43 ++++++++++++++++++++++++++++++
++++++++++
 src/openvpn/console_builtin.c    |  2 +-
 src/openvpn/crypto.c             | 17 ++++++----------
 src/openvpn/manage.c             |  2 +-
 src/openvpn/misc.c               |  4 ++--
 src/openvpn/options.c            |  6 +++---
 src/openvpn/ssl.c                | 32 +++++++++++++++---------------
 src/openvpn/ssl_verify.c         |  4 ++--
 src/openvpn/ssl_verify_mbedtls.c |  4 +---
 11 files changed, 80 insertions(+), 44 deletions(-)