Reduce fragmentation when using ncp-ciphers

URL:
https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg18975.html

2 files changed, 196 insertions, 1 deletions

diff --git a/security/openvpn/Makefile b/security/openvpn/Makefile
index a7e9e7578e87..a26f56e5cac7 100644
--- a/security/openvpn/Makefile
+++ b/security/openvpn/Makefile
@@ -3,7 +3,7 @@
 
 PORTNAME=		openvpn
 DISTVERSION=		2.4.8
-PORTREVISION?=		0
+PORTREVISION?=		1
 CATEGORIES=		security net net-vpn
 MASTER_SITES=		https://swupdate.openvpn.org/community/releases/ \
 			https://build.openvpn.net/downloads/releases/
diff --git a/security/openvpn/files/patch-g3bd91cd-Fix-broken-fragmentation-logic-when-using-NCP b/security/openvpn/files/patch-g3bd91cd-Fix-broken-fragmentation-logic-when-using-NCP
new file mode 100644
index 000000000000..6dd93beff69f
--- /dev/null
+++ b/security/openvpn/files/patch-g3bd91cd-Fix-broken-fragmentation-logic-when-using-NCP
@@ -0,0 +1,195 @@
+From 3bd91cd0e68762b861c57cf37f144d8a11704e9d Mon Sep 17 00:00:00 2001
+From: Lev Stipakov <lev@openvpn.net>
+Date: Wed, 30 Oct 2019 14:44:59 +0200
+Subject: [PATCH] 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>
+---
+ 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(-)
+
+diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
+index 65f790fda..84bb58447 100644
+--- ./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 d3785cabd..37b832ab0 100644
+--- ./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 77361833d..ed7975c35 100644
+--- ./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 dd5bd4163..ba2fbe404 100644
+--- ./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 9696e9bab..7dcd9622f 100644
+--- ./src/openvpn/ssl.c
++++ b/src/openvpn/ssl.c
+@@ -1962,7 +1962,8 @@ tls_session_generate_data_channel_keys(struct tls_session *session)
+ 
+ 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 8066789b6..6672d43fb 100644
+--- ./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.