[S] Change in openvpn[master]: mudp: send HMAC reset reply synchronously

cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/1702?usp=email )

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]>
Acked-by: Gert Doering <[email protected]>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1702
Message-Id: <[email protected]>
URL: https://www.mail-archive.com/[email protected]/msg37117.html
Signed-off-by: Gert Doering <[email protected]>
---
M src/openvpn/mudp.c
M src/openvpn/multi.h
2 files changed, 21 insertions(+), 25 deletions(-)




diff --git a/src/openvpn/mudp.c b/src/openvpn/mudp.c
index 432c79a..dded1ee 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,16 +54,30 @@
     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;
+
+    /*
+     * We do not want to keep any state here, so we send the reply to the
+     * initial packet synchronously without queueing anything.
+     *
+     * If we hit EAGAIN on a busy socket, the packet will be lost and the
+     * client will have to retransmit its HARD_RESET. This is considered an
+     * acceptable compromise to avoid consuming server resources under attack.
+     */
+    msg_set_prefix("Connection Attempt");
+    c->c2.to_link = c->c2.buffers->aux_buf;
+    c->c2.to_link_addr = &c->c2.from;
     msg(D_MULTI_DEBUG, "Reset packet from client, sending HMAC based reset challenge");
+    process_outgoing_link(c, sock);
+    c->c2.to_link.len = 0;
+    c->c2.to_link_addr = NULL;
+    msg_set_prefix(NULL);
 }
 
 
 /* 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 +120,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 +153,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 +208,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 +266,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 +334,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 +386,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: merged
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I2df0fec786184b9fcf9b7c56c74816325cdb6942
Gerrit-Change-Number: 1702
Gerrit-PatchSet: 3
Gerrit-Owner: its_Giaan <[email protected]>
Gerrit-Reviewer: cron2 <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-CC: ordex <[email protected]>

_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel