Introduce and use secure_memzero() to erase secrets
PATCH 1/1 Introduce and use secure_memzero() to erase secrets
This is a cherry-pick of commit 009521ac (master).
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.
--- src/openvpn/basic.h | 2 +- src/openvpn/buffer.c | 8 ++++---- src/openvpn/buffer.h | 43 +++++++++++++++++++++++++++++++++++++++++++ src/openvpn/crypto.c | 14 +++++--------- src/openvpn/init.c | 2 +- src/openvpn/manage.c | 2 +- src/openvpn/misc.c | 2 +- src/openvpn/options.c | 6 +++--- src/openvpn/ssl.c | 31 ++++++++++++++----------------- 9 files changed, 73 insertions(+), 37 deletions(-)
src/openvpn/basic.h +1 -1
@@ -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
@@ -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 ; }
@@ -579,9 +581,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
49 lines not on this page · raw message
src/openvpn/crypto.c +5 -9
@@ -98,15 +98,13 @@ openvpn_encrypt (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;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);
@@ -124,7 +122,6 @@ openvpn_encrypt (struct buffer *buf, struct buffer work,ASSERT (opt->flags &CO_USE_IV );/* IV and packet-ID required */ ASSERT (opt->packet_id);/* for this mode. */ -memset (iv_buf,0 , iv_size);buf_set_write (&b, iv_buf, iv_size);ASSERT (packet_id_write (&opt->packet_id->send, &b, true, false)); }
@@ -271,14 +268,13 @@ openvpn_decrypt (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)
@@ -828,7 +824,7 @@ get_tls_handshake_key (const struct key_type *key_type,init_key_ctx (&ctx->decrypt, &key2.keys[kds.in_key], &kt,OPENVPN_OP_DECRYPT ,"Incoming Control Channel Authentication" ); -CLEAR (key2); +secure_memzero (&key2,sizeof (key2)); }else {
@@ -1138,8 +1134,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/init.c +1 -1
@@ -2115,7 +2115,7 @@ do_init_crypto_static (struct context *c, const unsigned int flags) &c->c1.ks.key_type, OPENVPN_OP_DECRYPT, "Static Decrypt");/* Erase the temporary copy of key */ -CLEAR (key2); +secure_memzero (&key2,sizeof (key2)); }else {
src/openvpn/manage.c +1 -1
@@ -2975,7 +2975,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 +1 -1
@@ -1340,7 +1340,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
@@ -3819,7 +3819,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; }
@@ -3934,7 +3934,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); }
@@ -3966,7 +3966,7 @@ read_config_string (const char *prefix, }CLEAR (p); } -CLEAR (line); +secure_memzero (line,sizeof (line)); }void
src/openvpn/ssl.c +14 -17
@@ -1427,7 +1427,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);
@@ -1537,14 +1537,11 @@ generate_key_expansion (struct key_ctx_bi *key,const struct session_id *server_sid, bool server) { - uint8_t master[48]; -struct key2 key2; + uint8_t master[48] = { 0 }; +struct key2 key2 = {0 };bool ret = false;int i; -CLEAR (master); -CLEAR (key2); -/* debugging print of source key material */ key_source2_print (key_src);
@@ -1608,8 +1605,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; }
@@ -1806,7 +1803,7 @@ key_method_1_write (struct buffer *buf, struct tls_session *session)init_key_ctx (&ks->key.encrypt, &key, &session->opt->key_type,OPENVPN_OP_ENCRYPT ,"Data Channel Encrypt" ); -CLEAR (key); +secure_memzero (&key,sizeof (key));/* send local options string */ {
@@ -1976,7 +1973,7 @@ key_method_2_write (struct buffer *buf, struct tls_session *session) } } -CLEAR (*ks->key_src); +secure_memzero (ks->key_src,sizeof (*ks->key_src));tls_limit_reneg_bytes (session->opt->key_type.cipher, &session->opt->renegotiate_bytes); }
@@ -1985,7 +1982,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; }
@@ -2040,13 +2037,13 @@ key_method_1_read (struct buffer *buf, struct tls_session *session)init_key_ctx (&ks->key.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; }
@@ -2111,7 +2108,7 @@ key_method_2_read (struct buffer *buf, struct tls_multi *multi, struct tls_sessiif (!username_status || !password_status) { -CLEAR (*up); +secure_memzero (up,sizeof (*up));if (!(session->opt->ssl_flags &SSLF_AUTH_USER_PASS_OPTIONAL )) {msg (D_TLS_ERRORS ,"TLS Error: Auth Username/Password was not provided by peer" );
@@ -2126,7 +2123,7 @@ key_method_2_read (struct buffer *buf, struct tls_multi *multi, struct tls_sessi#endif verify_user_pass (up, multi, session); -CLEAR (*up); +secure_memzero (up,sizeof (*up)); }else {
@@ -2188,14 +2185,14 @@ key_method_2_read (struct buffer *buf, struct tls_multi *multi, struct tls_sessi goto error; } -CLEAR (*ks->key_src); +secure_memzero (ks->key_src,sizeof (*ks->key_src)); }gc_free (&gc);return true; error: -CLEAR (*ks->key_src); +secure_memzero (ks->key_src,sizeof (*ks->key_src));buf_clear (buf);gc_free (&gc);return false;
Signature · 1 line
2.7.4
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK. Verified that the code itself is the same as in master and release/2.4. There are a few difference where secure_memzero() are used, which is natural due to the differences between the 2.3 and newer releases. Overall, it looks good.
Your patch has been applied to the release/2.3 branch
commit e80c659d1c65db2047a245a50287906fda03d305
Author: Steffan Karger Date: Wed May 10 22:56:15 2017 +0200
Introduce and use secure_memzero() to erase secrets
Signed-off-by: Steffan Karger <[email protected]...>
Acked-by: David Sommerseth <[email protected]...>
Message-Id: <[email protected]...>
URL: https://www.mail-archive.com/[email protected]/msg14628.html
Signed-off-by: David Sommerseth <[email protected]...>- --
kind regards,
David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
iQIcBAEBCAAGBQJZFJX5AAoJEIbPlEyWcf3yAZEP/ArqOAsQIFOmcElbobjQdnJ8 y4tjMsA0SlqPmMcnzEE1qr/A69zg1fshqRxHrnmdxcLCd7ge/A66dBrmBBdb3QMP qN1kXBK12jrTXfrBBjOvg2kwx5Vpa+7oUjm0GG2V4WiAT+hpzHclGF3PE5YeUXIP X3SY9eFMsAWofX9XP2kJkEV4uTnKHyGeZaEHhkwJFqm9+jFqurF5xa9SJ0VzztPI oCK0IFlEMSwZwSHpHyCPA5NxUyv7GdrbKzDqZKZ6+5gROJQ7nX/SqoUGobnujFxq lYffAJqGUJe5/Oif3AI1AIVsT1H3Nik58e4HWOxo2L7gAHLuOAlxfKH4qg/i0qJs FuomcAMdnsuid7Pqxu1qMC6+gVuxel6Z9vb14Na6jy8qHK8fP+CHqZcwmO8ziYVw CN6f0Ecs6OUvDN4X5zMw++CA2JIOBBI6TxBoEgzrDP7xbY20PeNWsDQlKckAX0Ai 0RRRlNZ2rVEZFWoxjx1l+2t/T2fk7HXjnkKwamoHWrWueJkKfIvM77os3GeF5jBr 4ymc0NsmnVZBaGCUE+3QTdvIni+gAUdmAIGJDbgs9Hu4lqEWo0k/naNSc7vExm4Z ORoQhc/c5v6x2X/LxU+nKy2gVZS9lTmVUBzl/Ey7xTymWHlpY69iH+m/yNxdevJh mvnN0x9D6ahDSU4R6VPf
=GqO4
-----END PGP SIGNATURE-----