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. Change-Id: Ieefb501038b06c7520ed105c660a1c79887476f3 Signed-off-by: Arne Schwabe <a...@rfc2549.org> --- Changes.rst | 6 ++ doc/man-sections/protocol-options.rst | 3 + src/openvpn/comp.c | 32 +++--- src/openvpn/comp.h | 44 ++++---- src/openvpn/dco.c | 4 +- src/openvpn/init.c | 12 ++- src/openvpn/options.c | 150 ++++++++++++++++---------- src/openvpn/options.h | 2 - 8 files changed, 156 insertions(+), 97 deletions(-) diff --git a/Changes.rst b/Changes.rst index dedb97ee4..17b4c4d04 100644 --- a/Changes.rst +++ b/Changes.rst @@ -232,6 +232,12 @@ User-visible Changes - The ``client-pending-auth`` management command now requires also the key id. The management version has been changed to 5 to indicate this change. +- (OpenVPN 2.6.2) ``--allow-compression no`` has been changed to not allow + compression or compression framing at all now and is the new default. + Use ``--allow-compression stub-only`` for the old ``no`` behaviour of OpenVPN + 2.5 and OpenVPN 2.6.0. + + Common errors with OpenSSL 3.0 and OpenVPN 2.6 ---------------------------------------------- Both OpenVPN 2.6 and OpenSSL 3.0 tighten the security considerable, so some diff --git a/doc/man-sections/protocol-options.rst b/doc/man-sections/protocol-options.rst index 248f65cfd..76c323413 100644 --- a/doc/man-sections/protocol-options.rst +++ b/doc/man-sections/protocol-options.rst @@ -25,6 +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 compression or compression framing (stub). + + :code:`stub-only` OpenVPN will refuse any non-stub compression. :code:`yes` diff --git a/src/openvpn/comp.c b/src/openvpn/comp.c index 3b8d78996..c7ec6c7f5 100644 --- a/src/openvpn/comp.c +++ b/src/openvpn/comp.c @@ -134,27 +134,29 @@ 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"); } #endif /* USE_COMP */ diff --git a/src/openvpn/comp.h b/src/openvpn/comp.h index 685f40391..49b715b10 100644 --- a/src/openvpn/comp.h +++ b/src/openvpn/comp.h @@ -28,6 +28,29 @@ #ifndef OPENVPN_COMP_H #define OPENVPN_COMP_H +/* Compression flags */ +#define COMP_F_ADAPTIVE (1<<0) /* COMP_ALG_LZO only */ +#define COMP_F_ALLOW_COMPRESS (1<<1) /* not only downlink is compressed but also uplink */ +#define COMP_F_SWAP (1<<2) /* initial command byte is swapped with last byte in buffer to preserve payload alignment */ +#define COMP_F_ADVERTISE_STUBS_ONLY (1<<3) /* tell server that we only support compression stubs */ +#define COMP_F_ALLOW_STUB_ONLY (1<<4) /* Only accept stub compression, even with COMP_F_ADVERTISE_STUBS_ONLY + * 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 like stub v2 or comp-lzo no. Breaks DCO */ +#define COMP_F_INVALID_SETTING (1<<8) /* The server pushed an invalid setting that cannot work */ + +/* + * Information that basically identifies a compression + * algorithm and related flags. Also used without compression to bail + * out if we + */ +struct compress_options +{ + int alg; + unsigned int flags; +}; + #ifdef USE_COMP #include "buffer.h" @@ -51,17 +74,6 @@ #define COMP_ALGV2_SNAPPY 13 */ -/* Compression flags */ -#define COMP_F_ADAPTIVE (1<<0) /* COMP_ALG_LZO only */ -#define COMP_F_ALLOW_COMPRESS (1<<1) /* not only downlink is compressed but also uplink */ -#define COMP_F_SWAP (1<<2) /* initial command byte is swapped with last byte in buffer to preserve payload alignment */ -#define COMP_F_ADVERTISE_STUBS_ONLY (1<<3) /* tell server that we only support compression stubs */ -#define COMP_F_ALLOW_STUB_ONLY (1<<4) /* Only accept stub compression, even with COMP_F_ADVERTISE_STUBS_ONLY - * 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 */ - - /* * Length of prepended prefix on compressed packets */ @@ -130,16 +142,6 @@ struct compress_alg #include "comp-lz4.h" #endif -/* - * Information that basically identifies a compression - * algorithm and related flags. - */ -struct compress_options -{ - int alg; - unsigned int flags; -}; - /* * Workspace union of all supported compression algorithms */ diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c index 308578b47..c514bb143 100644 --- a/src/openvpn/dco.c +++ b/src/openvpn/dco.c @@ -408,9 +408,7 @@ dco_check_option(int msglevel, const struct options *o) } #if defined(USE_COMP) - if (o->comp.alg != COMP_ALG_UNDEF - || o->comp.flags & COMP_F_ALLOW_ASYM - || o->comp.flags & COMP_F_ALLOW_COMPRESS) + if (o->comp.alg != COMP_ALG_UNDEF || !(o->comp.flags & COMP_F_ALLOW_NOCOMP_ONLY)) { msg(msglevel, "Note: '--allow-compression' is not set to 'no', disabling data channel offload."); diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 3a6f624fd..2108efa90 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2634,14 +2634,22 @@ do_deferred_options(struct context *c, const unsigned int found) } } -#ifdef USE_COMP if (found & OPT_P_COMP) { + if (c->options.comp.flags & COMP_F_INVALID_SETTING) + { + msg(D_PUSH_ERRORS, "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."); + return false; + } +#ifdef USE_COMP msg(D_PUSH_DEBUG, "OPTIONS IMPORT: compression parms modified"); comp_uninit(c->c2.comp_context); c->c2.comp_context = comp_init(&c->options.comp); - } #endif + } if (found & OPT_P_SHAPER) { diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 64a8250b2..b4cbdb8d6 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -3644,11 +3644,21 @@ 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 (o->comp.alg == COMP_ALG_UNDEF) + { + o->comp.flags = COMP_F_ALLOW_NOCOMP_ONLY; + } + else if (!comp_non_stub_enabled(&o->comp)) + { + o->comp.flags = COMP_F_ALLOW_STUB_ONLY | COMP_F_ADVERTISE_STUBS_ONLY; + } } +#else /* ifdef USE_COMP */ + /* If no compression is compiled in, we do not support compression in + * any way */ + o->comp.flags = COMP_F_ALLOW_NOCOMP_ONLY; #endif } @@ -8355,15 +8365,23 @@ add_option(struct options *options, options->passtos = true; } #endif -#if defined(USE_COMP) +#ifdef USE_COMP else if (streq(p[0], "allow-compression") && p[1] && !p[2]) { VERIFY_PERMISSION(OPT_P_GENERAL); if (streq(p[1], "no")) { - options->comp.flags = - COMP_F_ALLOW_STUB_ONLY|COMP_F_ADVERTISE_STUBS_ONLY; + options->comp.flags = COMP_F_ALLOW_NOCOMP_ONLY; + if (comp_non_stub_enabled(&options->comp)) + { + msg(msglevel, "'--allow-compression no' conflicts with " + " enabling compression"); + } + } + else if (streq(p[1], "stub-only")) + { + options->comp.flags = COMP_F_ADVERTISE_STUBS_ONLY|COMP_F_ALLOW_STUB_ONLY; if (comp_non_stub_enabled(&options->comp)) { msg(msglevel, "'--allow-compression no' conflicts with " @@ -8374,7 +8392,7 @@ add_option(struct options *options, { /* Also printed on a push to hint at configuration problems */ msg(msglevel, "Cannot set allow-compression to '%s' " - "after set to 'no'", p[1]); + "after set to 'stub-only'", p[1]); goto err; } else if (streq(p[1], "asym")) @@ -8395,18 +8413,29 @@ add_option(struct options *options, else { msg(msglevel, "bad allow-compression option: %s -- " - "must be 'yes', 'no', or 'asym'", p[1]); + "must be 'yes', 'no', 'stub-only', or 'asym'", p[1]); goto err; } } +#endif /* ifdef USE_COMP */ else if (streq(p[0], "comp-lzo") && !p[2]) { VERIFY_PERMISSION(OPT_P_COMP); /* All lzo variants do not use swap */ options->comp.flags &= ~COMP_F_SWAP; + + if (options->comp.flags & COMP_F_ALLOW_NOCOMP_ONLY) + { + /* Also printed on a push to hint at configuration problems */ + msg(msglevel, "Cannot set comp-lzo to '%s', " + "allow-compression is set to 'no'", p[1]); + options->comp.flags |= COMP_F_INVALID_SETTING; + goto err; + } +#if defined(USE_COMP) #if defined(ENABLE_LZO) - if (p[1] && streq(p[1], "no")) + else if (p[1] && streq(p[1], "no")) #endif { options->comp.alg = COMP_ALG_STUB; @@ -8417,7 +8446,8 @@ add_option(struct options *options, { /* Also printed on a push to hint at configuration problems */ msg(msglevel, "Cannot set comp-lzo to '%s', " - "allow-compression is set to 'no'", p[1]); + "allow-compression is set to 'stub-only'", p[1]); + options->comp.flags |= COMP_F_INVALID_SETTING; goto err; } else if (p[1]) @@ -8445,6 +8475,7 @@ add_option(struct options *options, } show_compression_warning(&options->comp); #endif /* if defined(ENABLE_LZO) */ +#endif /* if defined(USE_COMP) */ } else if (streq(p[0], "comp-noadapt") && !p[1]) { @@ -8458,63 +8489,74 @@ add_option(struct options *options, else if (streq(p[0], "compress") && !p[2]) { VERIFY_PERMISSION(OPT_P_COMP); + const char *alg = "stub"; if (p[1]) { - if (streq(p[1], "stub")) - { - options->comp.alg = COMP_ALG_STUB; - options->comp.flags |= (COMP_F_SWAP|COMP_F_ADVERTISE_STUBS_ONLY); - } - else if (streq(p[1], "stub-v2")) - { - options->comp.alg = COMP_ALGV2_UNCOMPRESSED; - options->comp.flags |= COMP_F_ADVERTISE_STUBS_ONLY; - } - else if (streq(p[1], "migrate")) - { - options->comp.alg = COMP_ALG_UNDEF; - options->comp.flags = COMP_F_MIGRATE; - - } - else if (options->comp.flags & COMP_F_ALLOW_STUB_ONLY) + alg = p[1]; + } + if (options->comp.flags & COMP_F_ALLOW_NOCOMP_ONLY) + { + /* Also printed on a push to hint at configuration problems */ + msg(msglevel, "Cannot set compress to '%s', " + "allow-compression is set to 'no'", p[1]); + if (!streq(alg, "stub-v2")) { - /* Also printed on a push to hint at configuration problems */ - msg(msglevel, "Cannot set compress to '%s', " - "allow-compression is set to 'no'", p[1]); - goto err; + /* We allow stub-v2 to ignored on push as it often blindly + * pushed to disable compression on the client side */ + options->comp.flags |= COMP_F_INVALID_SETTING; } + goto err; + } +#if defined(USE_COMP) + else if (streq(p[1], "stub")) + { + options->comp.alg = COMP_ALG_STUB; + options->comp.flags |= (COMP_F_SWAP|COMP_F_ADVERTISE_STUBS_ONLY); + } + else if (streq(p[1], "stub-v2")) + { + options->comp.alg = COMP_ALGV2_UNCOMPRESSED; + options->comp.flags |= COMP_F_ADVERTISE_STUBS_ONLY; + } + else if (streq(p[1], "migrate")) + { + options->comp.alg = COMP_ALG_UNDEF; + options->comp.flags = COMP_F_MIGRATE; + } + else if (options->comp.flags & COMP_F_ALLOW_STUB_ONLY) + { + /* Also printed on a push to hint at configuration problems */ + msg(msglevel, "Cannot set compress to '%s', " + "allow-compression is set to 'stub-only'", p[1]); + options->comp.flags |= COMP_F_INVALID_SETTING; + goto err; + } #if defined(ENABLE_LZO) - else if (streq(p[1], "lzo")) - { - options->comp.alg = COMP_ALG_LZO; - options->comp.flags &= ~(COMP_F_ADAPTIVE | COMP_F_SWAP); - } + else if (streq(p[1], "lzo")) + { + options->comp.alg = COMP_ALG_LZO; + options->comp.flags &= ~(COMP_F_ADAPTIVE | COMP_F_SWAP); + } #endif #if defined(ENABLE_LZ4) - else if (streq(p[1], "lz4")) - { - options->comp.alg = COMP_ALG_LZ4; - options->comp.flags |= COMP_F_SWAP; - } - else if (streq(p[1], "lz4-v2")) - { - options->comp.alg = COMP_ALGV2_LZ4; - } -#endif - else - { - msg(msglevel, "bad comp option: %s", p[1]); - goto err; - } + else if (streq(p[1], "lz4")) + { + options->comp.alg = COMP_ALG_LZ4; + options->comp.flags |= COMP_F_SWAP; } + else if (streq(p[1], "lz4-v2")) + { + options->comp.alg = COMP_ALGV2_LZ4; + } +#endif else { - options->comp.alg = COMP_ALG_STUB; - options->comp.flags |= COMP_F_SWAP; + msg(msglevel, "bad comp option: %s", p[1]); + goto err; } show_compression_warning(&options->comp); +#endif /* if defined(USE_COMP) */ } -#endif /* USE_COMP */ else if (streq(p[0], "show-ciphers") && !p[1]) { VERIFY_PERMISSION(OPT_P_GENERAL); diff --git a/src/openvpn/options.h b/src/openvpn/options.h index 2f25f5d07..c56ef5e5e 100644 --- a/src/openvpn/options.h +++ b/src/openvpn/options.h @@ -395,9 +395,7 @@ struct options /* optimize TUN/TAP/UDP writes */ bool fast_io; -#ifdef USE_COMP struct compress_options comp; -#endif /* buffer sizes */ int rcvbuf; -- 2.37.1 (Apple Git-137.1) _______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel