[Openvpn-devel] [PATCH v9] Make required action returned from pre_decrypt_verdict more explicit

From: Arne Schwabe <[email protected]>

The current code relies on the condition if state.server_session_id
is defined to decide if session_skip_to_pre_start should be used.

Instead explicitly return the intent and use that to decide if
session_skip_to_pre_start should be called.

Change-Id: Iccdd4cfad090c565aac27e16cdaa9871106c2f89
Signed-off-by: Arne Schwabe <[email protected]>
Acked-by: Razvan Cojocaru <[email protected]>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1826
---

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/+/1826
This mail reflects revision 9 of this Change.

Signed-off-by line for the author was added as per our policy.

Acked-by according to Gerrit (reflected above):
Razvan Cojocaru <[email protected]>

        
diff --git a/src/openvpn/mudp.c b/src/openvpn/mudp.c
index 632b064..b370b6b 100644
--- a/src/openvpn/mudp.c
+++ b/src/openvpn/mudp.c
@@ -87,9 +87,24 @@
                           "Reset packet from client, sending HMAC based reset challenge", sock);
 }
 
+/**
+ * Verdict if this packet should create a new session. If a packet is invalid
+ * or we send out an HMAC based challenge, we do not want to create a new
+ * session.
+ */
+enum pre_decrypt_verdict
+{
+    /** This packet should not create a new session */
+    PRE_DECRYPT_NO_ACTION,
+    /** This packet creates a new session normally */
+    PRE_DECRYPT_CREATE_SESSION,
+    /** Create a new session but skip the first two packets of
+     * the three way handshake */
+    PRE_DECRYPT_CREATE_SESSION_SKIP
+};
 
-/* Returns true if this packet should create a new session */
-static bool
+/** Returns a verdict if this packet should create a new session */
+static enum pre_decrypt_verdict
 do_pre_decrypt_check(struct multi_context *m, struct tls_pre_decrypt_state *state,
                      struct mroute_addr addr, struct link_socket *sock)
 {
@@ -111,7 +126,7 @@
          * responses */
         if (!reflect_filter_rate_limit_check(m->initial_rate_limiter))
         {
-            return false;
+            return PRE_DECRYPT_NO_ACTION;
         }
     }
 
@@ -136,7 +151,7 @@
                 calculate_session_id_hmac(state->peer_session_id, from, hmac_key, handwindow, 0);
             send_hmac_reset_packet(m, state, tas, &sid, true, sock);
 
-            return false;
+            return PRE_DECRYPT_NO_ACTION;
         }
         else
         {
@@ -153,11 +168,11 @@
                     "ignoring connection attempt from old client (%s)",
                     peer);
                 gc_free(&gc);
-                return false;
+                return PRE_DECRYPT_NO_ACTION;
             }
             else
             {
-                return true;
+                return PRE_DECRYPT_CREATE_SESSION;
             }
         }
     }
@@ -170,7 +185,7 @@
         send_hmac_reset_packet(m, state, tas, &sid, false, sock);
 
         /* We have a reply do not create a new session */
-        return false;
+        return PRE_DECRYPT_NO_ACTION;
     }
     else if (verdict == VERDICT_VALID_CONTROL_V1 || verdict == VERDICT_VALID_ACK_V1
              || verdict == VERDICT_VALID_WKC_V1)
@@ -181,6 +196,7 @@
 
         bool pkt_is_ack = (verdict == VERDICT_VALID_ACK_V1);
         bool ret = check_session_hmac_and_pkt_id(state, from, hmac_key, handwindow, pkt_is_ack);
+        enum pre_decrypt_verdict action = PRE_DECRYPT_NO_ACTION;
 
         const char *peer = print_link_socket_actual(&m->top.c2.from, &gc);
         uint8_t pkt_firstbyte = *BPTR(&m->top.c2.buf);
@@ -198,14 +214,15 @@
                 "Valid packet (%s) with HMAC challenge from peer (%s), "
                 "accepting new connection.",
                 packet_opcode_name(op), peer);
+            action = PRE_DECRYPT_CREATE_SESSION_SKIP;
         }
         gc_free(&gc);
 
-        return ret;
+        return action;
     }
 
     /* VERDICT_INVALID */
-    return false;
+    return PRE_DECRYPT_NO_ACTION;
 }
 
 /**
@@ -220,9 +237,6 @@
                           struct link_socket *sock,
                           struct mroute_addr *real)
 {
-    struct hash *hash = m->hash;
-    struct tls_pre_decrypt_state state = { 0 };
-    struct multi_instance *mi = NULL;
     struct gc_arena gc = gc_new();
 
     if (m->deferred_shutdown_signal.signal_received)
@@ -231,8 +245,17 @@
             "MULTI: Connection attempt from %s ignored while server is "
             "shutting down",
             mroute_addr_print(real, &gc));
+        gc_free(&gc);
+        return NULL;
     }
-    else if (do_pre_decrypt_check(m, &state, *real, sock))
+
+    struct hash *hash = m->hash;
+    struct tls_pre_decrypt_state state = { 0 };
+    struct multi_instance *mi = NULL;
+
+    enum pre_decrypt_verdict verdict = do_pre_decrypt_check(m, &state, *real, sock);
+
+    if (verdict != PRE_DECRYPT_NO_ACTION)
     {
         /* 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
@@ -255,14 +278,14 @@
                 mi->did_real_hash = true;
                 multi_assign_peer_id(m, mi);
 
-                /* If we have a session id already, ensure that the
-                 * state is using the same */
-                if (session_id_defined(&state.server_session_id)
-                    && session_id_defined((&state.peer_session_id)))
+                struct tls_session *session =
+                    &mi->context.c2.tls_multi->session[TM_INITIAL];
+
+                if (verdict == PRE_DECRYPT_CREATE_SESSION_SKIP)
                 {
+                    /* This verdict is only possible if we have a peer session ID */
+                    ASSERT(session_id_defined(&state.peer_session_id));
                     mi->context.c2.tls_multi->n_sessions++;
-                    struct tls_session *session =
-                        &mi->context.c2.tls_multi->session[TM_INITIAL];
                     session_skip_to_pre_start(session, &state, &m->top.c2.from);
                 }
             }


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