Thread: [Openvpn-devel] [M] Change in openvpn[master]: Check message-id too when doing sessionid cookie

SourceForge Headquarters 225 Broadway Suite 1600 San Diego, CA 92101 +1 (858) 422-6466

Attention is currently required from: flichtenheld.

Hello flichtenheld,

I'd like you to do a code review.
Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1067?usp=email

to review the following change.


Change subject: Check message-id too when doing sessionid cookie
......................................................................

Check message-id too when doing sessionid cookie

This fixes that control packets on a floating client can trigger
creating a new session in rare instances.

Change-Id: I6752dcd5aff3e5cea2b439366479e86751a1c403
---
M src/openvpn/mudp.c
M src/openvpn/ssl_pkt.c
M src/openvpn/ssl_pkt.h
M tests/unit_tests/openvpn/test_pkt.c
4 files changed, 96 insertions(+), 6 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/67/1067/1

diff --git a/src/openvpn/mudp.c b/src/openvpn/mudp.c
index 93e65e0..9cd667c 100644
--- a/src/openvpn/mudp.c
+++ b/src/openvpn/mudp.c
@@ -63,7 +63,6 @@
     msg(D_MULTI_DEBUG, "Reset packet from client, sending HMAC based reset challenge");
 }
 
-
 /* Returns true if this packet should create a new session */
 static bool
 do_pre_decrypt_check(struct multi_context *m,
@@ -155,7 +154,8 @@
          * need to contain the peer id */
         struct gc_arena gc = gc_new();
 
-        bool ret = check_session_id_hmac(state, from, hmac, handwindow);
+        bool pkt_is_ack = (verdict == VERDICT_VALID_ACK_V1);
+        bool ret = check_session_id_hmac(state, from, hmac, handwindow, pkt_is_ack);
 
         const char *peer = print_link_socket_actual(&m->top.c2.from, &gc);
         uint8_t pkt_firstbyte = *BPTR( &m->top.c2.buf);
@@ -171,6 +171,7 @@
             msg(D_MULTI_DEBUG, "Valid packet (%s) with HMAC challenge from peer (%s), "
                 "accepting new connection.", packet_opcode_name(op), peer);
         }
+
         gc_free(&gc);
 
         return ret;
diff --git a/src/openvpn/ssl_pkt.c b/src/openvpn/ssl_pkt.c
index bfd405f..4b28798 100644
--- a/src/openvpn/ssl_pkt.c
+++ b/src/openvpn/ssl_pkt.c
@@ -293,6 +293,7 @@
     }
 }
 
+
 /*
  * This function is similar to tls_pre_decrypt, except it is called
  * when we are in server mode and receive an initial incoming
@@ -530,7 +531,8 @@
 check_session_id_hmac(struct tls_pre_decrypt_state *state,
                       const struct openvpn_sockaddr *from,
                       hmac_ctx_t *hmac,
-                      int handwindow)
+                      int handwindow,
+                      bool pkt_is_ack)
 {
     if (!from)
     {
@@ -545,6 +547,36 @@
         return false;
     }
 
+    /* Check if the packet ID of the packet or ACKED packet  is <= 1 */
+    for (int i = 0; i < ack.len; i++)
+    {
+        /* This packet ACKs a packet that has a higher packet id than the
+         * ones expected in the three-way handshake, consider it as invalid
+         * for the session */
+        if (ack.packet_id[i] > 1)
+        {
+            return false;
+        }
+    }
+
+    if (!pkt_is_ack)
+    {
+        packet_id_type message_id;
+        /* Extract the packet ID from the packet */
+        if (!reliable_ack_read_packet_id(&buf, &message_id))
+        {
+            return false;
+        }
+
+        /* same check. Anything larger than 1 is not considered part of the
+         * three-way handshake */
+        if (message_id > 1)
+        {
+            return false;
+        }
+    }
+
+
     /* check adjacent timestamps too */
     for (int offset = -2; offset <= 1; offset++)
     {
diff --git a/src/openvpn/ssl_pkt.h b/src/openvpn/ssl_pkt.h
index 98a39d3..53c1a5c 100644
--- a/src/openvpn/ssl_pkt.h
+++ b/src/openvpn/ssl_pkt.h
@@ -94,6 +94,9 @@
     VERDICT_VALID_ACK_V1,
     /** The packet is a valid control packet with appended wrapped client key */
     VERDICT_VALID_WKC_V1,
+    /** the packet is valid but failed the check of being belong to the packets
+     * that could be in the three-handshake */
+    VERDICT_INVALID_PKTID,
     /** the packet failed on of the various checks */
     VERDICT_INVALID
 };
@@ -180,17 +183,24 @@
 /**
  * Checks if a control packet has a correct HMAC server session id
  *
+ * This will also consider packets that have a packet id higher
+ * than 1 to be invalid as they are not part of the initial
+ * three way handshake of OpenVPN and should not create a new
+ * connection.
+ *
  * @param state         session information
  * @param from          link_socket from the client
  * @param hmac          the hmac context to use for the calculation
  * @param handwindow    the quantisation of the current time
+ * @param pkt_is_ack    the packet being checked is a P_ACK_V1
  * @return              the expected server session id
  */
 bool
 check_session_id_hmac(struct tls_pre_decrypt_state *state,
                       const struct openvpn_sockaddr *from,
                       hmac_ctx_t *hmac,
-                      int handwindow);
+                      int handwindow,
+                      bool check_session_id_hmac);
 
 /*
  * Write a control channel authentication record.
diff --git a/tests/unit_tests/openvpn/test_pkt.c b/tests/unit_tests/openvpn/test_pkt.c
index ebffabe..09fba8c 100644
--- a/tests/unit_tests/openvpn/test_pkt.c
+++ b/tests/unit_tests/openvpn/test_pkt.c
@@ -170,6 +170,17 @@
     0x85, 0xdb, 0x53, 0x56, 0x23, 0xb0, 0x2e
 };
 
+/* no tls-auth, P_ACK_V1, acks 0,1, and 2 */
+const uint8_t client_ack_123_none_random_id[] = {
+    0x28,
+    0xae, 0xb9, 0xaf, 0xe1, 0xf0, 0x1d, 0x79, 0xc8,
+    0x03,
+    0x00, 0x00, 0x00, 0x00,
+    0x00, 0x00, 0x00, 0x01,
+    0x00, 0x00, 0x00, 0x02,
+    0xdd, 0x85, 0xdb, 0x53, 0x56, 0x23, 0xb0, 0x2e
+};
+
 struct tls_auth_standalone
 init_tas_auth(int key_direction)
 {
@@ -439,7 +450,7 @@
     assert_int_equal(verdict, VERDICT_VALID_CONTROL_V1);
 
     /* This is a valid packet but containing a random id instead of an HMAC id*/
-    bool valid = check_session_id_hmac(&state, &from.dest, hmac, 30);
+    bool valid = check_session_id_hmac(&state, &from.dest, hmac, 30, false);
     assert_false(valid);
 
     free_tls_pre_decrypt_state(&state);
@@ -470,7 +481,7 @@
     verdict = tls_pre_decrypt_lite(&tas, &state, &from, &buf);
     assert_int_equal(verdict, VERDICT_VALID_ACK_V1);
 
-    bool valid = check_session_id_hmac(&state, &from.dest, hmac, 30);
+    bool valid = check_session_id_hmac(&state, &from.dest, hmac, 30, true);
     assert_true(valid);
 
     free_tls_pre_decrypt_state(&state);
@@ -479,6 +490,41 @@
     hmac_ctx_free(hmac);
 }
 
+static void
+test_verify_hmac_none_out_of_range_ack(void **ut_state)
+{
+    hmac_ctx_t *hmac = session_id_hmac_init();
+
+    struct link_socket_actual from = { 0 };
+    from.dest.addr.sa.sa_family = AF_INET;
+
+    struct tls_auth_standalone tas = { 0 };
+    struct tls_pre_decrypt_state state = { 0 };
+
+    struct buffer buf = alloc_buf(1024);
+    enum first_packet_verdict verdict;
+
+    tas.tls_wrap.mode = TLS_WRAP_NONE;
+
+    buf_reset_len(&buf);
+    buf_write(&buf, client_ack_123_none_random_id, sizeof(client_ack_123_none_random_id));
+
+
+    verdict = tls_pre_decrypt_lite(&tas, &state, &from, &buf);
+    assert_int_equal(verdict, VERDICT_VALID_ACK_V1);
+
+    /* should fail because it acks 2 */
+    bool valid = check_session_id_hmac(&state, &from.dest, hmac, 30, true);
+    assert_false(valid);
+
+    free_tls_pre_decrypt_state(&state);
+    free_buf(&buf);
+    hmac_ctx_cleanup(hmac);
+    hmac_ctx_free(hmac);
+}
+
+
+
 static hmac_ctx_t *
 init_static_hmac(void)
 {
@@ -667,6 +713,7 @@
         cmocka_unit_test(test_calc_session_id_hmac_static),
         cmocka_unit_test(test_verify_hmac_none),
         cmocka_unit_test(test_verify_hmac_tls_auth),
+        cmocka_unit_test(test_verify_hmac_none_out_of_range_ack),
         cmocka_unit_test(test_generate_reset_packet_plain),
         cmocka_unit_test(test_generate_reset_packet_tls_auth),
         cmocka_unit_test(test_extract_control_message)

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1067?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings

Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I6752dcd5aff3e5cea2b439366479e86751a1c403
Gerrit-Change-Number: 1067
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos <arn...@rf...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-MessageType: newchange


Thread: [Openvpn-devel] [M] Change in openvpn[master]: Check message-id too when doing sessionid cookie

Robust and flexible VPN network tunnelling

openvpn-devel