From: Antonio Quartulli <[email protected]> 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]> Acked-by: Arne Schwabe <[email protected]> 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 <[email protected]> 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 */ _______________________________________________ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
