Attention is currently required from: flichtenheld, plaisthos.
Hello plaisthos, flichtenheld,
I'd like you to do a code review.
Please visit
http://gerrit.openvpn.net/c/openvpn/+/1315?usp=email
to review the following change.
Change subject: Zeroize tls-crypt-v2 client keys
......................................................................
Zeroize tls-crypt-v2 client keys
Joshua Rogers sent in a bug report generated with ZeroPath that the
tls-crypt-v2 client key is loaded before running the verify script. If
the verify script fails, the key is not zeroized.
While investigating this report, I found that free_tls_pre_decrypt_state
never zeroizes tls_wrap_tmp.original_wrap_keydata. So also when the
check is successful, key data will remain in memory when it is no longer
needed.
This commit moves the tls-crypt-v2-verify check before loading the key.
If it fails, original_wrap_keydata is zeroized. Also, in
free_tls_pre_decrypt_state, if a key has been loaded,
original_wrap_keydata is zeroized.
Change-Id: Icfcbf8ee20c1c0016eb98b570f24b9325b157c5c
Signed-off-by: Max Fillinger <[email protected]>
---
M src/openvpn/ssl_pkt.c
M src/openvpn/tls_crypt.c
2 files changed, 7 insertions(+), 5 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/15/1315/1
diff --git a/src/openvpn/ssl_pkt.c b/src/openvpn/ssl_pkt.c
index 825719c..d7f7ac3 100644
--- a/src/openvpn/ssl_pkt.c
+++ b/src/openvpn/ssl_pkt.c
@@ -280,6 +280,7 @@
if (state->tls_wrap_tmp.cleanup_key_ctx)
{
free_key_ctx_bi(&state->tls_wrap_tmp.opt.key_ctx_bi);
+ secure_memzero(&state->tls_wrap_tmp.original_wrap_keydata,
sizeof(state->tls_wrap_tmp.original_wrap_keydata));
}
}
diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c
index 51b4eb3..a808de3 100644
--- a/src/openvpn/tls_crypt.c
+++ b/src/openvpn/tls_crypt.c
@@ -642,6 +642,12 @@
return false;
}
+ if (opt && opt->tls_crypt_v2_verify_script &&
!tls_crypt_v2_verify_metadata(ctx, opt))
+ {
+ secure_memzero(&ctx->original_wrap_keydata,
sizeof(ctx->original_wrap_keydata));
+ return false;
+ }
+
/* Load the decrypted key */
ctx->mode = TLS_WRAP_CRYPT;
ctx->cleanup_key_ctx = true;
@@ -652,11 +658,6 @@
/* Remove client key from buffer so tls-crypt code can unwrap message */
ASSERT(buf_inc_len(buf, -(BLEN(&wrapped_client_key))));
- if (opt && opt->tls_crypt_v2_verify_script)
- {
- return tls_crypt_v2_verify_metadata(ctx, opt);
- }
-
return true;
}
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1315?usp=email
To unsubscribe, or for help writing mail filters, visit
http://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Icfcbf8ee20c1c0016eb98b570f24b9325b157c5c
Gerrit-Change-Number: 1315
Gerrit-PatchSet: 1
Gerrit-Owner: MaxF <[email protected]>
Gerrit-Reviewer: flichtenheld <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>
Gerrit-Attention: flichtenheld <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
