Attention is currently required from: plaisthos.
Hello plaisthos,
I'd like you to do a code review.
Please visit
http://gerrit.openvpn.net/c/openvpn/+/1916?usp=email
to review the following change.
Change subject: ssl: do not trust the peer's request to resend the wrapped
client key
......................................................................
ssl: do not trust the peer's request to resend the wrapped client key
parse_early_negotiation_tlvs() sets CO_RESEND_WKC just because the peer
set EARLY_NEG_FLAG_RESEND_WKC in its reset packet, without checking that
this client has a wrapped client key at all. control_packet_needs_wkc()
then reports that our first control packet needs the key appended and
write_outgoing_tls_ciphertext() sizes it with
maxlen -= buf_len(session->tls_wrap.tls_crypt_v2_wkc);
tls_crypt_v2_wkc is only set with --tls-crypt-v2 and buf_len() is not
NULL-safe, so a client without --tls-crypt-v2 dies with SIGSEGV while
processing the server's first reset packet, before the peer has been
authenticated.
Only set the flag if we have a key to resend, check the same in
control_packet_needs_wkc(), and let tls_wrap_control() refuse to append a
key it does not have.
Github: OpenVPN/openvpn-private-issues#181
Change-Id: Iac6d49d064215510bde9a4cd4600ab3a50a45cb4
Signed-off-by: Lev Stipakov <[email protected]>
---
M src/openvpn/ssl.c
M src/openvpn/ssl_pkt.c
2 files changed, 23 insertions(+), 9 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/16/1916/1
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 76150d4..5e2fc23 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -2591,7 +2591,8 @@
* Parses the TLVs (type, length, value) in the early negotiation
*/
static bool
-parse_early_negotiation_tlvs(struct buffer *buf, struct key_state *ks)
+parse_early_negotiation_tlvs(struct buffer *buf, const struct tls_session
*session,
+ struct key_state *ks)
{
while (buf->len > 0)
{
@@ -2618,7 +2619,19 @@
if (flags & EARLY_NEG_FLAG_RESEND_WKC)
{
- ks->crypto_options.flags |= CO_RESEND_WKC;
+ /* Only a tls-crypt-v2 client has a wrapped client key to
+ * resend. The flag comes from the peer, so a peer that
asks
+ * anyone else is broken or hostile; ignore it rather than
+ * promising a key we do not have. */
+ if (session->tls_wrap.tls_crypt_v2_wkc)
+ {
+ ks->crypto_options.flags |= CO_RESEND_WKC;
+ }
+ else
+ {
+ msg(D_TLS_ERRORS, "TLS Error: peer asked us to resend
the wrapped "
+ "client key, but this is not a
tls-crypt-v2 client");
+ }
}
break;
@@ -2666,9 +2679,10 @@
}
static bool
-control_packet_needs_wkc(const struct key_state *ks)
+control_packet_needs_wkc(const struct tls_session *session, const struct
key_state *ks)
{
- return (ks->crypto_options.flags & CO_RESEND_WKC) &&
(ks->send_reliable->packet_id == 1);
+ return (ks->crypto_options.flags & CO_RESEND_WKC) &&
(ks->send_reliable->packet_id == 1)
+ && session->tls_wrap.tls_crypt_v2_wkc;
}
@@ -2719,7 +2733,7 @@
int maxlen = max_pkt_len * rel_avail;
/* Is first packet one that will have a WKC appended? */
- if (control_packet_needs_wkc(ks))
+ if (control_packet_needs_wkc(session, ks))
{
maxlen -= buf_len(session->tls_wrap.tls_crypt_v2_wkc);
}
@@ -2759,7 +2773,7 @@
{
int len = max_pkt_len;
int opcode = P_CONTROL_V1;
- if (control_packet_needs_wkc(ks))
+ if (control_packet_needs_wkc(session, ks))
{
opcode = P_CONTROL_WKC_V1;
len = max_int(0, len -
buf_len(session->tls_wrap.tls_crypt_v2_wkc));
@@ -2901,7 +2915,7 @@
* contains early protocol negotiation */
if (entry->packet_id == 0 && is_hard_reset_method2(entry->opcode))
{
- if (!parse_early_negotiation_tlvs(&entry->buf, ks))
+ if (!parse_early_negotiation_tlvs(&entry->buf, session, ks))
{
goto error;
}
@@ -3157,7 +3171,7 @@
/* Send 1 or more ACKs (each received control packet gets one ACK) */
if (!to_link->len && !reliable_ack_empty(ks->rec_ack))
{
- if (control_packet_needs_wkc(ks))
+ if (control_packet_needs_wkc(session, ks))
{
struct buffer *buf =
reliable_get_buf_output_sequenced(ks->send_reliable);
if (!buf)
diff --git a/src/openvpn/ssl_pkt.c b/src/openvpn/ssl_pkt.c
index 90b2aec..940f089 100644
--- a/src/openvpn/ssl_pkt.c
+++ b/src/openvpn/ssl_pkt.c
@@ -147,7 +147,7 @@
if ((header >> P_OPCODE_SHIFT) == P_CONTROL_HARD_RESET_CLIENT_V3
|| (header >> P_OPCODE_SHIFT) == P_CONTROL_WKC_V1)
{
- if (!buf_copy(&ctx->work, ctx->tls_crypt_v2_wkc))
+ if (!ctx->tls_crypt_v2_wkc || !buf_copy(&ctx->work,
ctx->tls_crypt_v2_wkc))
{
msg(D_TLS_ERRORS, "Could not append tls-crypt-v2 client key");
buf->len = 0;
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1916?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: Iac6d49d064215510bde9a4cd4600ab3a50a45cb4
Gerrit-Change-Number: 1916
Gerrit-PatchSet: 1
Gerrit-Owner: stipa <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
