diff mbox series

[Openvpn-devel,v4] Add 'allow-compression stub-only' internally for DCO

Message ID 20230324100640.1340535-1-arne@rfc2549.org
State Accepted
Headers
Series [Openvpn-devel,v4] Add 'allow-compression stub-only' internally for DCO |

Commit Message

Arne Schwabe March 24, 2023, 10:06 a.m. UTC 
  This changes the "no" setting of allow-compression to also refuse framing.
This is important for our DCO implementation as these do not implement framing.

This behaviour surfaced when a commercial VPN provider was pushing
"comp-lzo no" to a client with DCO. While we are technically at fault here
for announcing comp-lzo no support by announcing IV_LZO_STUB=1, the
VPN provider continues to push "comp-lzo no" even in absense of that
flag.

As the new default we default to allow-compression stub-only if a stub option
is found in the config and to allow-compression no otherwise. This ensures
that we only enable DCO when no compression framing is used.

This will now also bail out if the server pushes a compression setting that
we do not support as mismatching compression is almost never a working
connection. In my lz4-v2 and lzo-v2 you might have a connection that works
mostly but some packets will be dropped since they compressed which is not
desirable either since it becomes very hard to debug.

Patch v2: bail out if server pushes an unsupported method. Also include this
          bail out logic when OpenVPN is compiled without compression support.

Patch v3: always parse all compression option and move logic to check method
Patch v4: fix for not setting correct default for non-dco

Change-Id: Ibd0c77af24e2214b3055d585dc23a4b06dccd414
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 doc/man-sections/protocol-options.rst |  4 ++-
 src/openvpn/comp.c                    | 47 ++++++++++++++++++---------
 src/openvpn/comp.h                    |  2 +-
 src/openvpn/options.c                 | 14 ++++++--
 4 files changed, 46 insertions(+), 21 deletions(-)

Comments

Gert Doering March 24, 2023, 11:14 a.m. UTC | #1 
Acked-by: Gert Doering <gert@greenie.muc.de>

This is the actual thing we want to fix: if a server pushes 'comp-lzo no',
a non-DCO client will enable compression framing, while a DCO client can
not do this, and silently stays on "no framing" - and then both sides
will drop all data packets because "incorrect format".   We can not "make
it work", but we *can* abort the connection with a clear message so
the VPN provider / server operator can fix their setup.

This change also removes sending of all IV_COMP* variables to the server
if DCO is active - so a "server that cares" knows that it must not send
any compression settings.


I have run this through the t_client/t_server tests on DCO and non DCO
hosts, with and without compression, and all the existing setups still
work fine, including compatibility to older versions.

I have also tested pushed options and ccd/ options on "no compression
enabled" setups

 - pushing 'comp-lzo no' with no DCO --> accepted, do "stub" framing

 - pushing 'comp-lzo no' with DCO active --> refused, SIGUSR1 restart

   2023-03-24 09:09:48 Compression or compression stub framing is not 
     allowed since data-channel offloading is enabled.
   2023-03-24 09:09:48 OPTIONS ERROR: server pushed compression settings 
     that are not allowed and will result in a non-working connection.
     See also allow-compression in the manual.

 - pushing 'compress lz4' is refused in both cases, unless
   "allow-compression asym/yes" is set

 - ccd file producing 'comp-lzo no'
 - ccd file producing 'compress stub-v2'
 - ccd file producing 'compress lz4'
   --> this all works as expected (refusing the client with AUTH_FAILED), 
       though we have started to be "just a bit" chatty about this...

    tun-udp-p2mp[564755]: peer-id=1 OPTIONS IMPORT: reading client specific options from: ccd/freebsd-14-amd64
    tun-udp-p2mp[564755]: peer-id=1 Note: '--allow-compression' is not set to 'no', disabling data channel offload.
    tun-udp-p2mp[564755]: peer-id=1 MULTI: client has been rejected due to incompatible DCO options
    tun-udp-p2mp[564755]: peer-id=1 Compression or compression stub framing is not allowed since data-channel offloading is enabled.
    tun-udp-p2mp[564755]: peer-id=1 MULTI: client has been rejected due to invalid compression options


Compilation with --disable-lzo --disable-lz4 is still broken with this
commit - this was overlooked in part 2/4, and will be fixed in 4/4.

Your patch has been applied to the master and release/2.6 branch.

commit 4117d950788eebfaf6c9b5dde278e3a81b9e805d (master)
commit 2ac91ea73b76dd17d5cdf78740790ed928e08bff (release/2.6)
Author: Arne Schwabe
Date:   Fri Mar 24 11:06:40 2023 +0100

     Add 'allow-compression stub-only' internally for DCO

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20230324100640.1340535-1-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26509.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/doc/man-sections/protocol-options.rst b/doc/man-sections/protocol-options.rst
index 248f65cfd..055d08f95 100644
--- a/doc/man-sections/protocol-options.rst
+++ b/doc/man-sections/protocol-options.rst
@@ -25,7 +25,9 @@  configured in a compatible way between both the local and remote side.
       compression at the same time is not a feasible option.
 
   :code:`no`  (default)
-      OpenVPN will refuse any non-stub compression.
+      OpenVPN will refuse any compression.  If data-channel offloading
+      is enabled. OpenVPN will additionally also refuse compression
+      framing (stub).
 
   :code:`yes`
       OpenVPN will send and receive compressed packets.
diff --git a/src/openvpn/comp.c b/src/openvpn/comp.c
index d6d8029da..c7a562f5a 100644
--- a/src/openvpn/comp.c
+++ b/src/openvpn/comp.c
@@ -134,36 +134,51 @@  comp_print_stats(const struct compress_context *compctx, struct status_output *s
 void
 comp_generate_peer_info_string(const struct compress_options *opt, struct buffer *out)
 {
-    if (opt)
+    if (!opt || opt->flags & COMP_F_ALLOW_NOCOMP_ONLY)
+    {
+        return;
+    }
+
+    bool lzo_avail = false;
+    if (!(opt->flags & COMP_F_ADVERTISE_STUBS_ONLY))
     {
-        bool lzo_avail = false;
-        if (!(opt->flags & COMP_F_ADVERTISE_STUBS_ONLY))
-        {
 #if defined(ENABLE_LZ4)
-            buf_printf(out, "IV_LZ4=1\n");
-            buf_printf(out, "IV_LZ4v2=1\n");
+        buf_printf(out, "IV_LZ4=1\n");
+        buf_printf(out, "IV_LZ4v2=1\n");
 #endif
 #if defined(ENABLE_LZO)
-            buf_printf(out, "IV_LZO=1\n");
-            lzo_avail = true;
+        buf_printf(out, "IV_LZO=1\n");
+        lzo_avail = true;
 #endif
-        }
-        if (!lzo_avail)
-        {
-            buf_printf(out, "IV_LZO_STUB=1\n");
-        }
-        buf_printf(out, "IV_COMP_STUB=1\n");
-        buf_printf(out, "IV_COMP_STUBv2=1\n");
     }
+    if (!lzo_avail)
+    {
+        buf_printf(out, "IV_LZO_STUB=1\n");
+    }
+    buf_printf(out, "IV_COMP_STUB=1\n");
+    buf_printf(out, "IV_COMP_STUBv2=1\n");
 }
 
 bool
 check_compression_settings_valid(struct compress_options *info, int msglevel)
 {
+    /*
+     * We also allow comp-stub-v2 here as it technically allows escaping of
+     * weird mac address and IPv5 protocol but practically always is used
+     * as an way to disable all framing.
+     */
+    if (info->alg != COMP_ALGV2_UNCOMPRESSED && info->alg != COMP_ALG_UNDEF
+        && (info->flags & COMP_F_ALLOW_NOCOMP_ONLY))
+    {
+        msg(msglevel, "Compression or compression stub framing is not allowed "
+            "since data-channel offloading is enabled.");
+        return false;
+    }
+
     if ((info->flags & COMP_F_ALLOW_STUB_ONLY) && comp_non_stub_enabled(info))
     {
         msg(msglevel, "Compression is not allowed since allow-compression is "
-            "set to 'no'");
+            "set to 'stub-only'");
         return false;
     }
 #ifndef ENABLE_LZ4
diff --git a/src/openvpn/comp.h b/src/openvpn/comp.h
index 8636727ab..71b350d09 100644
--- a/src/openvpn/comp.h
+++ b/src/openvpn/comp.h
@@ -60,7 +60,7 @@ 
                                             * we still accept other compressions to be pushed */
 #define COMP_F_MIGRATE              (1<<5) /* push stub-v2 or comp-lzo no when we see a client with comp-lzo in occ */
 #define COMP_F_ALLOW_ASYM           (1<<6) /* Compression was explicitly set to allow asymetric compression */
-
+#define COMP_F_ALLOW_NOCOMP_ONLY    (1<<7) /* Do not allow compression framing (breaks DCO) */
 
 /*
  * Length of prepended prefix on compressed packets
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 435e1ca9e..0ccff7213 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -3644,10 +3644,12 @@  options_set_backwards_compatible_options(struct options *o)
      *
      * Disable compression by default starting with 2.6.0 if no other
      * compression related option has been explicitly set */
-    if (!comp_non_stub_enabled(&o->comp) && !need_compatibility_before(o, 20600)
-        && (o->comp.flags == 0))
+    if (!need_compatibility_before(o, 20600) && (o->comp.flags == 0))
     {
-        o->comp.flags = COMP_F_ALLOW_STUB_ONLY|COMP_F_ADVERTISE_STUBS_ONLY;
+        if (!comp_non_stub_enabled(&o->comp))
+        {
+            o->comp.flags = COMP_F_ALLOW_STUB_ONLY | COMP_F_ADVERTISE_STUBS_ONLY;
+        }
     }
 #endif
 }
@@ -3749,6 +3751,12 @@  options_postprocess_mutate(struct options *o, struct env_set *es)
         o->tuntap_options.disable_dco = !dco_check_option(D_DCO, o)
                                         || !dco_check_startup_option(D_DCO, o);
     }
+#ifdef USE_COMP
+    if (dco_enabled(o))
+    {
+        o->comp.flags |= COMP_F_ALLOW_NOCOMP_ONLY;
+    }
+#endif
 
 #ifdef _WIN32
     if (dco_enabled(o))