[Openvpn-devel,v1] mudp: send HMAC reset reply synchronously
Commit Message
From: Antonio Quartulli <antonio@mandelbit.com>
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 <antonio@mandelbit.com>
Acked-by: Arne Schwabe <arne-openvpn@rfc2549.org>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1702
---
This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1702
This mail reflects revision 1 of this Change.
Acked-by according to Gerrit (reflected above):
Arne Schwabe <arne-openvpn@rfc2549.org>
Comments
So, the need for this is "not directly obvious", because, "everything
works fine", no? Discussed this on IRC with Antonio and Gianmarco, and
the finding they made is that basically this is just working by accident,
given that the event handler does not work really well for setups with
multiple UDP sockets having activity at the same time. Gerrit#1635 sets
out to fix this - and uncovers that our "initial packet reply" code
is misbehaving, so if you have multiple sockets receiving RESET packets
at the same time, replies get sent to the wrong address. Very bad.
Now, to trigger this, you need to actually have multiple sockets at
the same time, *and* lots of concurrent connection activities - which
is not very typical at steady state, but after restarting a busy server,
this might get hit.
I have stared at it "and it seems to make sense", and also my t_server
testbed passes all server side tests just fine - so it's not breaking
anything ;-) - also, Arne had a close look and since he understands the
initial handshake bits best, if he's fine, so am I.
Your patch has been applied to the master and release/2.7 branch (bugfix).
commit 9a80d609674b70324abf23ded4d2133f967f99d3
Author: Antonio Quartulli
Date: Mon Jun 8 08:02:20 2026 +0200
mudp: send HMAC reset reply synchronously
Signed-off-by: Antonio Quartulli <antonio@mandelbit.com>
Acked-by: Arne Schwabe <arne-openvpn@rfc2549.org>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1702
Message-Id: <20260608060227.9892-1-gert@greenie.muc.de>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg37081.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
@@ -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;
@@ -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
*/