Attention is currently required from: ordex, plaisthos.
Hello plaisthos,
I'd like you to do a code review.
Please visit
http://gerrit.openvpn.net/c/openvpn/+/1702?usp=email
to review the following change.
Change subject: mudp: send HMAC reset reply synchronously
......................................................................
mudp: send HMAC reset reply synchronously
Building a HARD_RESET reply was queueing the result into three
multi_context fields and deferring the send to the next event-loop
iteration, where multi_process_outgoing_link() flushed it:
struct buffer hmac_reply;
struct link_socket_actual *hmac_reply_dest; /* aliased &m->top.c2.from */
struct link_socket *hmac_reply_ls;
The mechanism had three latent issues:
1. hmac_reply_dest = &m->top.c2.from stored a pointer alias into
shared mutable state. Any subsequent read into m->top.c2.from
silently retargeted the pending reply to a different peer.
2. m->hmac_reply_ls = sock; at the top of multi_get_create_instance_udp()
was executed unconditionally for every UDP packet, including packets
that did not queue a reply. A stale queued reply could thus be sent
on the wrong listening socket.
3. hmac_reply.data pointed into m->top.c2.buffers->aux_buf (the only
slot). A second send_hmac_reset_packet() before the first flush
would overwrite the first reply's bytes.
These were latent on master because m->multi_io->udp_flags was consumed
and zeroed by the first event in each multi_io_process_io() loop, so at
most one UDP read ran per outer iteration.
Send the reply synchronously from within send_hmac_reset_packet() using
the sock that the read fired on (threaded through do_pre_decrypt_check).
The reply is small, stateless, and rate-limited by the existing
reflect_filter_rate_limit_check(); dropping on EAGAIN is acceptable
because the client retransmits its HARD_RESET. The three multi_context
fields and the deferred-flush block in multi_process_outgoing_link() are
gone; p2mp_iow_flags() no longer needs an IOW_TO_LINK branch for hmac
state.
Change-Id: I2df0fec786184b9fcf9b7c56c74816325cdb6942
Signed-off-by: Antonio Quartulli <[email protected]>
---
M src/openvpn/mudp.c
M src/openvpn/multi.h
2 files changed, 18 insertions(+), 25 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/02/1702/1
diff --git a/src/openvpn/mudp.c b/src/openvpn/mudp.c
index 432c79a..874736e 100644
--- a/src/openvpn/mudp.c
+++ b/src/openvpn/mudp.c
@@ -40,7 +40,7 @@
static void
send_hmac_reset_packet(struct multi_context *m, struct tls_pre_decrypt_state
*state,
struct tls_auth_standalone *tas, struct session_id *sid,
- bool request_resend_wkc)
+ bool request_resend_wkc, struct link_socket *sock)
{
reset_packet_id_send(&state->tls_wrap_tmp.opt.packet_id.send);
state->tls_wrap_tmp.opt.packet_id.rec.initialized = true;
@@ -54,8 +54,19 @@
ASSERT(buf_init(&c->c2.buffers->aux_buf, buf.offset));
buf_copy(&c->c2.buffers->aux_buf, &buf);
- m->hmac_reply = c->c2.buffers->aux_buf;
- m->hmac_reply_dest = &m->top.c2.from;
+
+ /* Send synchronously: queueing across event-loop iterations would alias
+ * m->top.c2.from and aux_buf into cross-socket race territory. The reply
+ * is small, stateless, and rate-limited by
reflect_filter_rate_limit_check();
+ * dropping on EAGAIN is acceptable as the client retransmits its
HARD_RESET. */
+ msg_set_prefix("Connection Attempt");
+ c->c2.to_link = c->c2.buffers->aux_buf;
+ c->c2.to_link_addr = &c->c2.from;
+ process_outgoing_link(c, sock);
+ c->c2.to_link.len = 0;
+ c->c2.to_link_addr = NULL;
+ msg_set_prefix(NULL);
+
msg(D_MULTI_DEBUG, "Reset packet from client, sending HMAC based reset
challenge");
}
@@ -63,7 +74,7 @@
/* Returns true if this packet should create a new session */
static bool
do_pre_decrypt_check(struct multi_context *m, struct tls_pre_decrypt_state
*state,
- struct mroute_addr addr)
+ struct mroute_addr addr, struct link_socket *sock)
{
ASSERT(m->top.c2.tls_auth_standalone);
@@ -106,7 +117,7 @@
/* Calculate the session ID HMAC for our reply and create reset
packet */
struct session_id sid =
calculate_session_id_hmac(state->peer_session_id, from, hmac,
handwindow, 0);
- send_hmac_reset_packet(m, state, tas, &sid, true);
+ send_hmac_reset_packet(m, state, tas, &sid, true, sock);
return false;
}
@@ -139,7 +150,7 @@
struct session_id sid =
calculate_session_id_hmac(state->peer_session_id, from, hmac,
handwindow, 0);
- send_hmac_reset_packet(m, state, tas, &sid, false);
+ send_hmac_reset_packet(m, state, tas, &sid, false, sock);
/* We have a reply do not create a new session */
return false;
@@ -194,7 +205,6 @@
struct multi_instance *mi = NULL;
struct hash *hash = m->hash;
real.proto = sock->info.proto;
- m->hmac_reply_ls = sock;
if (mroute_extract_openvpn_sockaddr(&real, &m->top.c2.from.dest, true) &&
m->top.c2.buf.len > 0)
{
@@ -253,7 +263,7 @@
"shutting down",
mroute_addr_print(&real, &gc));
}
- else if (do_pre_decrypt_check(m, &state, real))
+ else if (do_pre_decrypt_check(m, &state, real, sock))
{
/* This is an unknown session but with valid tls-auth/tls-crypt
* (or no auth at all). If this is the initial packet of a
@@ -321,15 +331,6 @@
{
multi_process_outgoing_link_dowork(m, mi, mpp_flags);
}
- if (m->hmac_reply_dest && m->hmac_reply.len > 0)
- {
- msg_set_prefix("Connection Attempt");
- m->top.c2.to_link = m->hmac_reply;
- m->top.c2.to_link_addr = m->hmac_reply_dest;
- process_outgoing_link(&m->top, m->hmac_reply_ls);
- m->hmac_reply_ls = NULL;
- m->hmac_reply_dest = NULL;
- }
}
/*
@@ -382,10 +383,6 @@
{
flags |= IOW_MBUF;
}
- else if (m->hmac_reply_dest)
- {
- flags |= IOW_TO_LINK;
- }
else
{
flags |= IOW_READ;
diff --git a/src/openvpn/multi.h b/src/openvpn/multi.h
index 3ed08d4..8b837fa 100644
--- a/src/openvpn/multi.h
+++ b/src/openvpn/multi.h
@@ -201,10 +201,6 @@
struct context top; /**< Storage structure for process-wide
* configuration. */
- struct buffer hmac_reply;
- struct link_socket_actual *hmac_reply_dest;
- struct link_socket *hmac_reply_ls;
-
/*
* Timer object for stale route check
*/
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1702?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: I2df0fec786184b9fcf9b7c56c74816325cdb6942
Gerrit-Change-Number: 1702
Gerrit-PatchSet: 1
Gerrit-Owner: its_Giaan <[email protected]>
Gerrit-Reviewer: ordex <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>
Gerrit-Attention: ordex <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
