Browse code

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.

Trac: #751

Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
Acked-by: Selva Nair <selva.nair@gmail.com>
Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <1480371252-3880-1-git-send-email-steffan@karger.me>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg13278.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>

Steffan Karger authored on 2016/11/29 07:14:12
Showing 11 changed files
... ...
@@ -30,7 +30,7 @@
30 30
 /* size of an array */
31 31
 #define SIZE(x) (sizeof(x)/sizeof(x[0]))
32 32
 
33
-/* clear an object */
33
+/* clear an object (may be optimized away, use secure_memzero() to erase secrets) */
34 34
 #define CLEAR(x) memset(&(x), 0, sizeof(x))
35 35
 
36 36
 #define IPV4_NETMASK_HOST 0xffffffffU
... ...
@@ -155,7 +155,9 @@ void
155 155
 buf_clear (struct buffer *buf)
156 156
 {
157 157
   if (buf->capacity > 0)
158
-    memset (buf->data, 0, buf->capacity);
158
+    {
159
+      secure_memzero (buf->data, buf->capacity);
160
+    }
159 161
   buf->len = 0;
160 162
   buf->offset = 0;
161 163
 }
... ...
@@ -619,9 +621,7 @@ string_clear (char *str)
619 619
 {
620 620
   if (str)
621 621
     {
622
-      const int len = strlen (str);
623
-      if (len > 0)
624
-	memset (str, 0, len);
622
+      secure_memzero (str, strlen (str));
625 623
     }
626 624
 }
627 625
 
... ...
@@ -328,6 +328,49 @@ has_digit (const unsigned char* src)
328 328
   return false;
329 329
 }
330 330
 
331
+/**
332
+ * Securely zeroise memory.
333
+ *
334
+ * This code and description are based on code supplied by Zhaomo Yang, of the
335
+ * University of California, San Diego (which was released into the public
336
+ * domain).
337
+ *
338
+ * The secure_memzero function attempts to ensure that an optimizing compiler
339
+ * does not remove the intended operation if cleared memory is not accessed
340
+ * again by the program. This code has been tested under Clang 3.9.0 and GCC
341
+ * 6.2 with optimization flags -O, -Os, -O0, -O1, -O2, and -O3 on
342
+ * Ubuntu 16.04.1 LTS; under Clang 3.9.0 with optimization flags -O, -Os,
343
+ * -O0, -O1, -O2, and -O3 on FreeBSD 10.2-RELEASE; under Microsoft Visual Studio
344
+ * 2015 with optimization flags /O1, /O2 and /Ox on Windows 10.
345
+ *
346
+ * Theory of operation:
347
+ *
348
+ * 1. On Windows, use the SecureZeroMemory which ensures that data is
349
+ *    overwritten.
350
+ * 2. Under GCC or Clang, use a memory barrier, which forces the preceding
351
+ *    memset to be carried out. The overhead of a memory barrier is usually
352
+ *    negligible.
353
+ * 3. If none of the above are available, use the volatile pointer
354
+ *    technique to zero memory one byte at a time.
355
+ *
356
+ * @param data	Pointer to data to zeroise.
357
+ * @param len	Length of data, in bytes.
358
+ */
359
+static inline void
360
+secure_memzero (void *data, size_t len)
361
+{
362
+#if defined(_WIN32)
363
+  SecureZeroMemory (data, len);
364
+#elif defined(__GNUC__) || defined(__clang__)
365
+  memset(data, 0, len);
366
+  __asm__ __volatile__("" : : "r"(data) : "memory");
367
+#else
368
+  volatile char *p = (volatile char *) data;
369
+  while (len--)
370
+    *p++ = 0;
371
+#endif
372
+}
373
+
331 374
 /*
332 375
  * printf append to a buffer with overflow check,
333 376
  * due to usage of vsnprintf, it will leave space for
... ...
@@ -218,7 +218,7 @@ static bool get_console_input (const char *prompt, const bool echo, char *input,
218 218
         if (gp)
219 219
         {
220 220
             strncpynt (input, gp, capacity);
221
-            memset (gp, 0, strlen (gp));
221
+            secure_memzero (gp, strlen (gp));
222 222
             ret = true;
223 223
         }
224 224
     }
... ...
@@ -86,12 +86,11 @@ openvpn_encrypt_aead (struct buffer *buf, struct buffer work,
86 86
   {
87 87
     struct buffer iv_buffer;
88 88
     struct packet_id_net pin;
89
-    uint8_t iv[OPENVPN_MAX_IV_LENGTH];
89
+    uint8_t iv[OPENVPN_MAX_IV_LENGTH] = {0};
90 90
     const int iv_len = cipher_ctx_iv_length (ctx->cipher);
91 91
 
92 92
     ASSERT (iv_len >= OPENVPN_AEAD_MIN_IV_LEN && iv_len <= OPENVPN_MAX_IV_LENGTH);
93 93
 
94
-    memset(iv, 0, sizeof(iv));
95 94
     buf_set_write (&iv_buffer, iv, iv_len);
96 95
 
97 96
     /* 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,
175 175
       /* Do Encrypt from buf -> work */
176 176
       if (ctx->cipher)
177 177
 	{
178
-	  uint8_t iv_buf[OPENVPN_MAX_IV_LENGTH];
178
+	  uint8_t iv_buf[OPENVPN_MAX_IV_LENGTH] = {0};
179 179
 	  const int iv_size = cipher_ctx_iv_length (ctx->cipher);
180 180
 	  const cipher_kt_t *cipher_kt = cipher_ctx_get_cipher_kt (ctx->cipher);
181 181
 	  int outlen;
... ...
@@ -190,8 +189,6 @@ openvpn_encrypt_v1 (struct buffer *buf, struct buffer work,
190 190
 
191 191
 	  if (cipher_kt_mode_cbc(cipher_kt))
192 192
 	    {
193
-	      CLEAR (iv_buf);
194
-
195 193
 	      /* generate pseudo-random IV */
196 194
 	      if (opt->flags & CO_USE_IV)
197 195
 		prng_bytes (iv_buf, iv_size);
... ...
@@ -214,7 +211,6 @@ openvpn_encrypt_v1 (struct buffer *buf, struct buffer work,
214 214
 	      ASSERT (packet_id_initialized(&opt->packet_id));
215 215
 
216 216
 	      packet_id_alloc_outgoing (&opt->packet_id.send, &pin, true);
217
-	      memset (iv_buf, 0, iv_size);
218 217
 	      buf_set_write (&b, iv_buf, iv_size);
219 218
 	      ASSERT (packet_id_write (&pin, &b, true, false));
220 219
 	    }
... ...
@@ -550,14 +546,13 @@ openvpn_decrypt_v1 (struct buffer *buf, struct buffer work,
550 550
 	{
551 551
 	  const int iv_size = cipher_ctx_iv_length (ctx->cipher);
552 552
 	  const cipher_kt_t *cipher_kt = cipher_ctx_get_cipher_kt (ctx->cipher);
553
-	  uint8_t iv_buf[OPENVPN_MAX_IV_LENGTH];
553
+	  uint8_t iv_buf[OPENVPN_MAX_IV_LENGTH] = { 0 };
554 554
 	  int outlen;
555 555
 
556 556
 	  /* initialize work buffer with FRAME_HEADROOM bytes of prepend capacity */
557 557
 	  ASSERT (buf_init (&work, FRAME_HEADROOM_ADJ (frame, FRAME_HEADROOM_MARKER_DECRYPT)));
558 558
 
559 559
 	  /* use IV if user requested it */
560
-	  CLEAR (iv_buf);
561 560
 	  if (opt->flags & CO_USE_IV)
562 561
 	    {
563 562
 	      if (buf->len < iv_size)
... ...
@@ -1128,7 +1123,7 @@ crypto_read_openvpn_key (const struct key_type *key_type,
1128 1128
   init_key_ctx (&ctx->decrypt, &key2.keys[kds.in_key], key_type,
1129 1129
 		OPENVPN_OP_DECRYPT, log_prefix);
1130 1130
 
1131
-  CLEAR (key2);
1131
+  secure_memzero (&key2, sizeof (key2));
1132 1132
 }
1133 1133
 
1134 1134
 /* header and footer for static key file */
... ...
@@ -1380,8 +1375,8 @@ write_key_file (const int nkeys, const char *filename)
1380 1380
       buf_printf (&out, "%s\n", fmt);
1381 1381
 
1382 1382
       /* zero memory which held key component (will be freed by GC) */
1383
-      memset (fmt, 0, strlen(fmt));
1384
-      CLEAR (key);
1383
+      secure_memzero (fmt, strlen (fmt));
1384
+      secure_memzero (&key, sizeof (key));
1385 1385
     }
1386 1386
 
1387 1387
   buf_printf (&out, "%s\n", static_key_foot);
... ...
@@ -3154,7 +3154,7 @@ management_query_user_pass (struct management *man,
3154 3154
 	  man->connection.up_query.nocache = up->nocache; /* preserve caller's nocache setting */
3155 3155
 	  *up = man->connection.up_query;
3156 3156
 	}
3157
-      CLEAR (man->connection.up_query);
3157
+      secure_memzero (&man->connection.up_query, sizeof (man->connection.up_query));
3158 3158
     }
3159 3159
 
3160 3160
   gc_free (&gc);
... ...
@@ -501,7 +501,7 @@ remove_env_item (const char *str, const bool do_free, struct env_item **list)
501 501
 	    *list = current->next;
502 502
 	  if (do_free)
503 503
 	    {
504
-	      memset (current->string, 0, strlen (current->string));
504
+	      secure_memzero (current->string, strlen (current->string));
505 505
 	      free (current->string);
506 506
 	      free (current);
507 507
 	    }
... ...
@@ -1342,7 +1342,7 @@ purge_user_pass (struct user_pass *up, const bool force)
1342 1342
   static bool warn_shown = false;
1343 1343
   if (nocache || force)
1344 1344
     {
1345
-      CLEAR (*up);
1345
+      secure_memzero (up, sizeof(*up));
1346 1346
       up->nocache = nocache;
1347 1347
     }
1348 1348
   else if (!warn_shown)
... ...
@@ -3968,7 +3968,7 @@ read_inline_file (struct in_src *is, const char *close_tag, struct gc_arena *gc)
3968 3968
   ret = string_alloc (BSTR (&buf), gc);
3969 3969
   buf_clear (&buf);
3970 3970
   free_buf (&buf);
3971
-  CLEAR (line);
3971
+  secure_memzero (line, sizeof (line));
3972 3972
   return ret;
3973 3973
 }
3974 3974
 
... ...
@@ -4083,7 +4083,7 @@ read_config_file (struct options *options,
4083 4083
     {
4084 4084
       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);
4085 4085
     }
4086
-  CLEAR (line);
4086
+  secure_memzero (line, sizeof (line));
4087 4087
   CLEAR (p);
4088 4088
 }
4089 4089
 
... ...
@@ -4115,7 +4115,7 @@ read_config_string (const char *prefix,
4115 4115
 	}
4116 4116
       CLEAR (p);
4117 4117
     }
4118
-  CLEAR (line);
4118
+  secure_memzero (line, sizeof (line));
4119 4119
 }
4120 4120
 
4121 4121
 void
... ...
@@ -894,7 +894,7 @@ key_state_free (struct key_state *ks, bool clear)
894 894
 #endif
895 895
 
896 896
   if (clear)
897
-    CLEAR (*ks);
897
+    secure_memzero (ks, sizeof (*ks));
898 898
 }
899 899
 
900 900
 /** @} name Functions for initialization and cleanup of key_state structures */
... ...
@@ -1024,7 +1024,7 @@ tls_session_free (struct tls_session *session, bool clear)
1024 1024
   cert_hash_free (session->cert_hash_set);
1025 1025
 
1026 1026
   if (clear)
1027
-    CLEAR (*session);
1027
+    secure_memzero (session, sizeof (*session));
1028 1028
 }
1029 1029
 
1030 1030
 /** @} 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)
1048 1048
   if (reinit_src)
1049 1049
     tls_session_init (multi, &multi->session[src]);
1050 1050
   else
1051
-    CLEAR (multi->session[src]);
1051
+    secure_memzero (&multi->session[src], sizeof (multi->session[src]));
1052 1052
 
1053 1053
   dmsg (D_TLS_DEBUG, "TLS: move_session: exit");
1054 1054
 }
... ...
@@ -1212,7 +1212,7 @@ tls_multi_free (struct tls_multi *multi, bool clear)
1212 1212
 
1213 1213
   if (multi->auth_token)
1214 1214
     {
1215
-      memset (multi->auth_token, 0, AUTH_TOKEN_SIZE);
1215
+      secure_memzero (multi->auth_token, AUTH_TOKEN_SIZE);
1216 1216
       free (multi->auth_token);
1217 1217
     }
1218 1218
 
... ...
@@ -1222,7 +1222,7 @@ tls_multi_free (struct tls_multi *multi, bool clear)
1222 1222
     tls_session_free (&multi->session[i], false);
1223 1223
 
1224 1224
   if (clear)
1225
-    CLEAR (*multi);
1225
+    secure_memzero (multi, sizeof (*multi));
1226 1226
 
1227 1227
   free(multi);
1228 1228
 }
... ...
@@ -1512,7 +1512,7 @@ tls1_P_hash(const md_kt_t *md_kt,
1512 1512
     }
1513 1513
   hmac_ctx_cleanup(&ctx);
1514 1514
   hmac_ctx_cleanup(&ctx_tmp);
1515
-  CLEAR (A1);
1515
+  secure_memzero (A1, sizeof (A1));
1516 1516
 
1517 1517
   dmsg (D_SHOW_KEY_SOURCE, "tls1_P_hash out: %s", format_hex (out_orig, olen_orig, 0, &gc));
1518 1518
   gc_free (&gc);
... ...
@@ -1565,7 +1565,7 @@ tls1_PRF(const uint8_t *label,
1565 1565
   for (i=0; i<olen; i++)
1566 1566
     out1[i]^=out2[i];
1567 1567
 
1568
-  memset (out2, 0, olen);
1568
+  secure_memzero (out2, olen);
1569 1569
 
1570 1570
   dmsg (D_SHOW_KEY_SOURCE, "tls1_PRF out[%d]: %s", olen, format_hex (out1, olen, 0, &gc));
1571 1571
 
... ...
@@ -1702,8 +1702,8 @@ generate_key_expansion (struct key_ctx_bi *key,
1702 1702
   ret = true;
1703 1703
 
1704 1704
  exit:
1705
-  CLEAR (master);
1706
-  CLEAR (key2);
1705
+  secure_memzero (&master, sizeof (master));
1706
+  secure_memzero (&key2, sizeof (key2));
1707 1707
 
1708 1708
   return ret;
1709 1709
 }
... ...
@@ -1787,7 +1787,7 @@ tls_session_generate_data_channel_keys(struct tls_session *session)
1787 1787
 
1788 1788
   ret = true;
1789 1789
 cleanup:
1790
-  CLEAR (*ks->key_src);
1790
+  secure_memzero (ks->key_src, sizeof (*ks->key_src));
1791 1791
   return ret;
1792 1792
 }
1793 1793
 
... ...
@@ -2017,7 +2017,7 @@ key_method_1_write (struct buffer *buf, struct tls_session *session)
2017 2017
   init_key_ctx (&ks->crypto_options.key_ctx_bi.encrypt, &key,
2018 2018
 		&session->opt->key_type, OPENVPN_OP_ENCRYPT,
2019 2019
 		"Data Channel Encrypt");
2020
-  CLEAR (key);
2020
+  secure_memzero (&key, sizeof (key));
2021 2021
 
2022 2022
   /* send local options string */
2023 2023
   {
... ...
@@ -2202,7 +2202,7 @@ key_method_2_write (struct buffer *buf, struct tls_session *session)
2202 2202
 
2203 2203
  error:
2204 2204
   msg (D_TLS_ERRORS, "TLS Error: Key Method #2 write failed");
2205
-  CLEAR (*ks->key_src);
2205
+  secure_memzero (ks->key_src, sizeof (*ks->key_src));
2206 2206
   return false;
2207 2207
 }
2208 2208
 
... ...
@@ -2257,13 +2257,13 @@ key_method_1_read (struct buffer *buf, struct tls_session *session)
2257 2257
   init_key_ctx (&ks->crypto_options.key_ctx_bi.decrypt, &key,
2258 2258
 		&session->opt->key_type, OPENVPN_OP_DECRYPT,
2259 2259
 		"Data Channel Decrypt");
2260
-  CLEAR (key);
2260
+  secure_memzero (&key, sizeof (key));
2261 2261
   ks->authenticated = true;
2262 2262
   return true;
2263 2263
 
2264 2264
  error:
2265 2265
   buf_clear (buf);
2266
-  CLEAR (key);
2266
+  secure_memzero (&key, sizeof (key));
2267 2267
   return false;
2268 2268
 }
2269 2269
 
... ...
@@ -2377,7 +2377,7 @@ key_method_2_read (struct buffer *buf, struct tls_multi *multi, struct tls_sessi
2377 2377
     }
2378 2378
 
2379 2379
   /* clear username and password from memory */
2380
-  CLEAR (*up);
2380
+  secure_memzero (up, sizeof (*up));
2381 2381
 
2382 2382
   /* Perform final authentication checks */
2383 2383
   if (ks->authenticated)
... ...
@@ -2433,7 +2433,7 @@ key_method_2_read (struct buffer *buf, struct tls_multi *multi, struct tls_sessi
2433 2433
   return true;
2434 2434
 
2435 2435
  error:
2436
-  CLEAR (*ks->key_src);
2436
+  secure_memzero (ks->key_src, sizeof (*ks->key_src));
2437 2437
   buf_clear (buf);
2438 2438
   gc_free (&gc);
2439 2439
   return false;
... ...
@@ -1176,7 +1176,7 @@ verify_user_pass(struct user_pass *up, struct tls_multi *multi,
1176 1176
       if (memcmp_constant_time(multi->auth_token, up->password,
1177 1177
                  strlen(multi->auth_token)) != 0)
1178 1178
         {
1179
-          memset (multi->auth_token, 0, AUTH_TOKEN_SIZE);
1179
+          secure_memzero (multi->auth_token, AUTH_TOKEN_SIZE);
1180 1180
           free (multi->auth_token);
1181 1181
           multi->auth_token = NULL;
1182 1182
           multi->auth_token_sent = false;
... ...
@@ -1262,7 +1262,7 @@ verify_user_pass(struct user_pass *up, struct tls_multi *multi,
1262 1262
                   "No auth-token will be activated now");
1263 1263
 	      if (multi->auth_token)
1264 1264
 		{
1265
-		  memset (multi->auth_token, 0, AUTH_TOKEN_SIZE);
1265
+		  secure_memzero (multi->auth_token, AUTH_TOKEN_SIZE);
1266 1266
 		  free (multi->auth_token);
1267 1267
 		  multi->auth_token = NULL;
1268 1268
 		}
... ...
@@ -348,12 +348,10 @@ x509_setenv (struct env_set *es, int cert_depth, mbedtls_x509_crt *cert)
348 348
   int i;
349 349
   unsigned char c;
350 350
   const mbedtls_x509_name *name;
351
-  char s[128];
351
+  char s[128] = { 0 };
352 352
 
353 353
   name = &cert->subject;
354 354
 
355
-  memset( s, 0, sizeof( s ) );
356
-
357 355
   while( name != NULL )
358 356
     {
359 357
       char name_expand[64+8];