[Openvpn-devel,v3] Fix broken fragmentation logic when using NCP

Message ID 1572439499-16276-1-git-send-email-lstipakov@gmail.com
State Accepted
Headers show
Series [Openvpn-devel,v3] Fix broken fragmentation logic when using NCP | expand

Commit Message

Lev Stipakov Oct. 30, 2019, 1:44 a.m. UTC
From: Lev Stipakov <lev@openvpn.net>

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>
---

 v3:

    Refine a bit misleading commit message. Before one might think that
 problem is in incorrect MSS value being set - it is not. The problem is in
 fragment calculation.

 v2:

    Fix --disable-fragment build

 src/openvpn/forward.c |  3 +++
 src/openvpn/init.c    | 12 +++++++++++-
 src/openvpn/openvpn.h |  1 +
 src/openvpn/push.c    |  9 ++++++++-
 src/openvpn/ssl.c     | 19 ++++++++++++++++++-
 src/openvpn/ssl.h     | 13 ++++++++-----
 6 files changed, 49 insertions(+), 8 deletions(-)

Comments

Gert Doering Nov. 6, 2019, 8:42 a.m. UTC | #1
Acked-by: Gert Doering <gert@greenie.muc.de>

Thanks for clarifying what the actual problem is, so we have it nicely
in the logs.  I have diff'ed this patch vs. d22ba6b2c551f, and the only
difference is related to "use_iv" which got kicked out from master.

So I'm mostly relying on the ACks from Arne and Steffan and not doing
any review myself :-) - I have done a full client test, of course.

Your patch has been applied to the release/2.4 branch.

commit 3bd91cd0e68762b861c57cf37f144d8a11704e9d
Author: Lev Stipakov
Date:   Wed Oct 30 14:44:59 2019 +0200

     Fix broken fragmentation logic when using NCP

     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>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 65f790f..84bb584 100644
--- 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 d3785ca..37b832a 100644
--- 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 7736183..ed7975c 100644
--- 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 dd5bd41..ba2fbe4 100644
--- 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 9696e9b..7dcd962 100644
--- 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 8066789..6672d43 100644
--- 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.