Fix broken fragmentation logic when using NCP

This is the 2.4 backport of master patch (commit d22ba6b).

NCP negotiation replaces worst case crypto overhead
with actual one in data channel frame. That frame
params are used by mssfix. Fragment frame still contains
worst case overhead.

Without this patch, fragmentation logic incorrectly uses
max crypto overhead when calculating packet size. It exceeds
fragment size and openvpn peforms fragmentation:

> sudo tcpdump port 1194
13:59:06.956394 IP server.fi.openvpn > nat2.panoulu.net.openvpn: UDP,
length 652
13:59:06.956489 IP server.fi.openvpn > nat2.panoulu.net.openvpn: UDP,
length 648

This patch fixes fragmentation calculation by
setting actual crypto overhead, and no unnecessary
fragmentation is performed:

> sudo tcpdump port 1194
13:58:08.685915 IP server.fi.openvpn > nat2.panoulu.net.openvpn: UDP,
length 1272
13:58:08.686007 IP server.fi.openvpn > nat2.panoulu.net.openvpn: UDP,
length 1272

Trac #1140

Signed-off-by: Lev Stipakov <lev@openvpn.net>
Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <1572439499-16276-1-git-send-email-lstipakov@gmail.com>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg18975.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>

diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 65f790fdad1b1bbdec9a997ea445edc2d37be30d..84bb58447dfd23a5b26da0f8d91eeeaac100c015 100644 (file)
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -873,6 +873,9 @@ process_incoming_link_part1(struct context *c, struct link_socket_info *lsi, boo
                 if (is_hard_reset(opcode, c->options.key_method))
                 {
                     c->c2.frame = c->c2.frame_initial;
+#ifdef ENABLE_FRAGMENT
+                    c->c2.frame_fragment = c->c2.frame_fragment_initial;
+#endif
                 }
 
                 interval_action(&c->c2.tmp_int);

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index d3785cabdd9db5a1f5e66a8bc7bc5be01866c8f4..37b832ab055a0447fcd0db3a663c019898f83e08 100644 (file)
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2294,9 +2294,18 @@ do_deferred_options(struct context *c, const unsigned int found)
         {
             tls_poor_mans_ncp(&c->options, c->c2.tls_multi->remote_ciphername);
         }
+        struct frame *frame_fragment = NULL;
+#ifdef ENABLE_FRAGMENT
+        if (c->options.ce.fragment)
+        {
+            frame_fragment = &c->c2.frame_fragment;
+        }
+#endif
+
         /* Do not regenerate keys if server sends an extra push reply */
         if (!session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized
-            && !tls_session_update_crypto_params(session, &c->options, &c->c2.frame))
+            && !tls_session_update_crypto_params(session, &c->options, &c->c2.frame,
+                                                 frame_fragment))
         {
             msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to import crypto options");
             return false;
@@ -3035,6 +3044,7 @@ do_init_frame(struct context *c)
      */
     c->c2.frame_fragment = c->c2.frame;
     frame_subtract_extra(&c->c2.frame_fragment, &c->c2.frame_fragment_omit);
+    c->c2.frame_fragment_initial = c->c2.frame_fragment;
 #endif
 
 #if defined(ENABLE_FRAGMENT) && defined(ENABLE_OCC)

diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h
index 77361833d34b04b56f28fa598b03f0db9f84e484..ed7975c35ba71becd7abea085b5e383a65566287 100644 (file)
--- a/src/openvpn/openvpn.h
+++ b/src/openvpn/openvpn.h
@@ -269,6 +269,7 @@ struct context_2
     /* Object to handle advanced MTU negotiation and datagram fragmentation */
     struct fragment_master *fragment;
     struct frame frame_fragment;
+    struct frame frame_fragment_initial;
     struct frame frame_fragment_omit;
 #endif
 

diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index dd5bd4163d27ac5c718944f71d55c1a6e5fc9102..ba2fbe4045551d717ae5369fa1d963ac3cb1e0e1 100644 (file)
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -287,11 +287,18 @@ incoming_push_message(struct context *c, const struct buffer *buffer)
     {
         if (c->options.mode == MODE_SERVER)
         {
+            struct frame *frame_fragment = NULL;
+#ifdef ENABLE_FRAGMENT
+            if (c->options.ce.fragment)
+            {
+                frame_fragment = &c->c2.frame_fragment;
+            }
+#endif
             struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
             /* Do not regenerate keys if client send a second push request */
             if (!session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized
                 && !tls_session_update_crypto_params(session, &c->options,
-                                                     &c->c2.frame))
+                                                     &c->c2.frame, frame_fragment))
             {
                 msg(D_TLS_ERRORS, "TLS Error: initializing data channel failed");
                 goto error;

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 9696e9bab7e1b6660348cb54186b9947dfda7dc5..7dcd9622fafd8b73376c7191c26bc07990c98db9 100644 (file)
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1962,7 +1962,8 @@ cleanup:
 
 bool
 tls_session_update_crypto_params(struct tls_session *session,
-                                 struct options *options, struct frame *frame)
+                                 struct options *options, struct frame *frame,
+                                 struct frame *frame_fragment)
 {
     if (!session->opt->server
         && 0 != strcmp(options->ciphername, session->opt->config_ciphername)
@@ -2006,6 +2007,22 @@ tls_session_update_crypto_params(struct tls_session *session,
     frame_init_mssfix(frame, options);
     frame_print(frame, D_MTU_INFO, "Data Channel MTU parms");
 
+    /*
+     * mssfix uses data channel framing, which at this point contains
+     * actual overhead. Fragmentation logic uses frame_fragment, which
+     * still contains worst case overhead. Replace it with actual overhead
+     * to prevent unneeded fragmentation.
+     */
+
+    if (frame_fragment)
+    {
+        frame_remove_from_extra_frame(frame_fragment, crypto_max_overhead());
+        crypto_adjust_frame_parameters(frame_fragment, &session->opt->key_type,
+                                       options->use_iv, options->replay, packet_id_long_form);
+        frame_set_mtu_dynamic(frame_fragment, options->ce.fragment, SET_MTU_UPPER_BOUND);
+        frame_print(frame_fragment, D_MTU_INFO, "Fragmentation MTU parms");
+    }
+
     return tls_session_generate_data_channel_keys(session);
 }
 

diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
index 8066789b6995fd5acd22feb2361e2fb65919ea08..6672d43fb3907df57d06a0703e1cab5555f0dba3 100644 (file)
--- a/src/openvpn/ssl.h
+++ b/src/openvpn/ssl.h
@@ -475,15 +475,18 @@ void tls_update_remote_addr(struct tls_multi *multi,
  * Update TLS session crypto parameters (cipher and auth) and derive data
  * channel keys based on the supplied options.
  *
- * @param session       The TLS session to update.
- * @param options       The options to use when updating session.
- * @param frame         The frame options for this session (frame overhead is
- *                      adjusted based on the selected cipher/auth).
+ * @param session         The TLS session to update.
+ * @param options         The options to use when updating session.
+ * @param frame           The frame options for this session (frame overhead is
+ *                        adjusted based on the selected cipher/auth).
+ * @param frame_fragment  The fragment frame options.
  *
  * @return true if updating succeeded, false otherwise.
  */
 bool tls_session_update_crypto_params(struct tls_session *session,
-                                      struct options *options, struct frame *frame);
+                                      struct options *options,
+                                      struct frame *frame,
+                                      struct frame *frame_fragment);
 
 /**
  * "Poor man's NCP": Use peer cipher if it is an allowed (NCP) cipher.