Attention is currently required from: plaisthos.
Hello plaisthos,
I'd like you to reexamine a change. Please visit
http://gerrit.openvpn.net/c/openvpn/+/1916?usp=email
to look at the new patch set (#2).
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.
tls_reset_standalone() drives tls_wrap_control() directly, so the test
uses it to build the two control packets that carry a wrapped client key
with tls_crypt_v2_wkc unset. Both segfault without the fix.
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
M tests/unit_tests/openvpn/test_pkt.c
3 files changed, 70 insertions(+), 9 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/16/1916/2
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;
diff --git a/tests/unit_tests/openvpn/test_pkt.c
b/tests/unit_tests/openvpn/test_pkt.c
index a732c2b..51c73d8 100644
--- a/tests/unit_tests/openvpn/test_pkt.c
+++ b/tests/unit_tests/openvpn/test_pkt.c
@@ -695,6 +695,52 @@
free_tas(&tas_server);
}
+/* A peer can ask us to append the wrapped client key (WKc) to our next
+ * control packet. Only a tls-crypt-v2 client has one, so check that we
+ * refuse to build such a packet when we have no key instead of walking
+ * into the NULL pointer. */
+static void
+test_wkc_not_appended_without_key(void **ut_state)
+{
+ struct tls_auth_standalone tas = init_tas_crypt(false);
+ struct session_id own_id = { { 1, 2, 3, 4, 5, 6, 7, 8 } };
+ struct session_id remote_id = { { 8, 7, 6, 5, 4, 3, 2, 1 } };
+ struct frame frame = { .buf = { .headroom = 200, .payload_size = 1400 }, 0
};
+ tas.frame = frame;
+
+ packet_id_init(&tas.tls_wrap.opt.packet_id, 5, 5, "UNITTEST", 0);
+ reset_packet_id_send(&tas.tls_wrap.opt.packet_id.send);
+ now = 0x22446688;
+
+ /* no WKc configured, so both opcodes that would append one must fail */
+ assert_null(tas.tls_wrap.tls_crypt_v2_wkc);
+
+ uint8_t header = 0 | (P_CONTROL_WKC_V1 << P_OPCODE_SHIFT);
+ struct buffer buf =
+ tls_reset_standalone(&tas.tls_wrap, &tas, &own_id, &remote_id, header,
false);
+ assert_int_equal(BLEN(&buf), 0);
+
+ header = 0 | (P_CONTROL_HARD_RESET_CLIENT_V3 << P_OPCODE_SHIFT);
+ buf = tls_reset_standalone(&tas.tls_wrap, &tas, &own_id, &remote_id,
header, false);
+ assert_int_equal(BLEN(&buf), 0);
+
+ /* with a WKc the same packet is built and the key ends up at its end */
+ uint8_t wkc_data[32];
+ memset(wkc_data, 0x5a, sizeof(wkc_data));
+ struct buffer wkc = alloc_buf(sizeof(wkc_data));
+ assert_true(buf_write(&wkc, wkc_data, sizeof(wkc_data)));
+ tas.tls_wrap.tls_crypt_v2_wkc = &wkc;
+
+ header = 0 | (P_CONTROL_WKC_V1 << P_OPCODE_SHIFT);
+ buf = tls_reset_standalone(&tas.tls_wrap, &tas, &own_id, &remote_id,
header, false);
+ assert_true(BLEN(&buf) > (int)sizeof(wkc_data));
+ assert_memory_equal(BPTR(&buf) + BLEN(&buf) - sizeof(wkc_data), wkc_data,
sizeof(wkc_data));
+
+ free_buf(&wkc);
+ packet_id_free(&tas.tls_wrap.opt.packet_id);
+ free_tas(&tas);
+}
+
static void
test_extract_control_message(void **ut_state)
{
@@ -745,6 +791,7 @@
cmocka_unit_test(test_verify_hmac_none_out_of_range_ack),
cmocka_unit_test(test_generate_reset_packet_plain),
cmocka_unit_test(test_generate_reset_packet_tls_auth),
+ cmocka_unit_test(test_wkc_not_appended_without_key),
cmocka_unit_test(test_extract_control_message)
};
--
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: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Iac6d49d064215510bde9a4cd4600ab3a50a45cb4
Gerrit-Change-Number: 1916
Gerrit-PatchSet: 2
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
