| Message ID | 20210319153129.8734-2-arne@rfc2549.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel,1/2] Move extract_iv_proto to ssl_util.c/h | expand |
On 19/03/2021 16:31, Arne Schwabe wrote: > This option allow migration to a non compression server config while > still retraining compatibility with client that have a compression > setting in their config. > > For existing setups that used to have comp-lzo no or another > compression setting in their configs it is a difficult to migrate to > a setup without compression without replacing all client configs at > once especially if OpenVPN 2.3 or earlier clients are in the mix that > do not support pushing stub-v2. Even with OpenVPN 2.4 and later clients > that support pushing this is not a satisfying solution as the clients > log occ mismatches and the "push stub-v2" needs to be in the server > config "forever". > > Signed-off-by: Arne Schwabe <arne@rfc2549.org> > --- > doc/man-sections/protocol-options.rst | 12 +++- > src/openvpn/comp.h | 1 + > src/openvpn/multi.c | 41 +++++++++++++ > src/openvpn/options.c | 6 ++ > src/openvpn/ssl.c | 34 ++++++++++- > src/openvpn/ssl_common.h | 1 + > src/openvpn/ssl_util.c | 43 ++++++++++++++ > src/openvpn/ssl_util.h | 15 +++++ > tests/unit_tests/openvpn/Makefile.am | 14 ++++- > tests/unit_tests/openvpn/test_misc.c | 83 +++++++++++++++++++++++++++ > 10 files changed, 245 insertions(+), 5 deletions(-) > create mode 100644 tests/unit_tests/openvpn/test_misc.c > This fails compiling: ../../../src/openvpn/ssl.c: In function ‘key_method_2_write’: ../../../src/openvpn/ssl.c:2280:13: error: ‘multi’ undeclared (first use in this function) if (multi->remote_usescomp && session->opt->mode == MODE_SERVER ^~~~~ ../../../src/openvpn/ssl.c:2280:13: note: each undeclared identifier is reported only once for each function it appears in make[3]: *** [Makefile:742: ssl.o] Error 1
Am 20.03.21 um 14:20 schrieb David Sommerseth: > On 19/03/2021 16:31, Arne Schwabe wrote: >> This option allow migration to a non compression server config while >> still retraining compatibility with client that have a compression >> setting in their config. >> >> For existing setups that used to have comp-lzo no or another >> compression setting in their configs it is a difficult to migrate to >> a setup without compression without replacing all client configs at >> once especially if OpenVPN 2.3 or earlier clients are in the mix that >> do not support pushing stub-v2. Even with OpenVPN 2.4 and later clients >> that support pushing this is not a satisfying solution as the clients >> log occ mismatches and the "push stub-v2" needs to be in the server >> config "forever". >> >> Signed-off-by: Arne Schwabe <arne@rfc2549.org> >> --- >> doc/man-sections/protocol-options.rst | 12 +++- >> src/openvpn/comp.h | 1 + >> src/openvpn/multi.c | 41 +++++++++++++ >> src/openvpn/options.c | 6 ++ >> src/openvpn/ssl.c | 34 ++++++++++- >> src/openvpn/ssl_common.h | 1 + >> src/openvpn/ssl_util.c | 43 ++++++++++++++ >> src/openvpn/ssl_util.h | 15 +++++ >> tests/unit_tests/openvpn/Makefile.am | 14 ++++- >> tests/unit_tests/openvpn/test_misc.c | 83 +++++++++++++++++++++++++++ >> 10 files changed, 245 insertions(+), 5 deletions(-) >> create mode 100644 tests/unit_tests/openvpn/test_misc.c >> > > This fails compiling: > > ../../../src/openvpn/ssl.c: In function ‘key_method_2_write’: > ../../../src/openvpn/ssl.c:2280:13: error: ‘multi’ undeclared (first use > in this function) > if (multi->remote_usescomp && session->opt->mode == MODE_SERVER > ^~~~~ > ../../../src/openvpn/ssl.c:2280:13: note: each undeclared identifier is > reported only once for each function it appears in > make[3]: *** [Makefile:742: ssl.o] Error 1 Oh the fun of rebasing. The multi argument was added by another patch. Can you just the small patch to review the rest of the patch?: static bool -key_method_2_write(struct buffer *buf, struct tls_session *session) +key_method_2_write(struct buffer *buf, struct tls_multi *multi, struct tls_session *session) { struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */ @@ -2856,7 +2856,7 @@ tls_process(struct tls_multi *multi, if (!buf->len && ((ks->state == S_START && !session->opt->server) || (ks->state == S_GOT_KEY && session->opt->server))) { - if (!key_method_2_write(buf, session)) + if (!key_method_2_write(buf, multi, session)) { goto error; }
On 21/03/2021 13:56, Arne Schwabe wrote: > Am 20.03.21 um 14:20 schrieb David Sommerseth: >> On 19/03/2021 16:31, Arne Schwabe wrote: >>> This option allow migration to a non compression server config while >>> still retraining compatibility with client that have a compression >>> setting in their config. >>> >>> For existing setups that used to have comp-lzo no or another >>> compression setting in their configs it is a difficult to migrate to >>> a setup without compression without replacing all client configs at >>> once especially if OpenVPN 2.3 or earlier clients are in the mix that >>> do not support pushing stub-v2. Even with OpenVPN 2.4 and later clients >>> that support pushing this is not a satisfying solution as the clients >>> log occ mismatches and the "push stub-v2" needs to be in the server >>> config "forever". >>> >>> Signed-off-by: Arne Schwabe <arne@rfc2549.org> >>> --- >>> doc/man-sections/protocol-options.rst | 12 +++- >>> src/openvpn/comp.h | 1 + >>> src/openvpn/multi.c | 41 +++++++++++++ >>> src/openvpn/options.c | 6 ++ >>> src/openvpn/ssl.c | 34 ++++++++++- >>> src/openvpn/ssl_common.h | 1 + >>> src/openvpn/ssl_util.c | 43 ++++++++++++++ >>> src/openvpn/ssl_util.h | 15 +++++ >>> tests/unit_tests/openvpn/Makefile.am | 14 ++++- >>> tests/unit_tests/openvpn/test_misc.c | 83 +++++++++++++++++++++++++++ >>> 10 files changed, 245 insertions(+), 5 deletions(-) >>> create mode 100644 tests/unit_tests/openvpn/test_misc.c >>> >> >> This fails compiling: >> >> ../../../src/openvpn/ssl.c: In function ‘key_method_2_write’: >> ../../../src/openvpn/ssl.c:2280:13: error: ‘multi’ undeclared (first use >> in this function) >> if (multi->remote_usescomp && session->opt->mode == MODE_SERVER >> ^~~~~ >> ../../../src/openvpn/ssl.c:2280:13: note: each undeclared identifier is >> reported only once for each function it appears in >> make[3]: *** [Makefile:742: ssl.o] Error 1 > > Oh the fun of rebasing. The multi argument was added by another patch. > Can you just the small patch to review the rest of the patch?: > > static bool > -key_method_2_write(struct buffer *buf, struct tls_session *session) > +key_method_2_write(struct buffer *buf, struct tls_multi *multi, struct > tls_session *session) > { > struct key_state *ks = &session->key[KS_PRIMARY]; /* primary > key */ > > @@ -2856,7 +2856,7 @@ tls_process(struct tls_multi *multi, > if (!buf->len && ((ks->state == S_START && !session->opt->server) > || (ks->state == S_GOT_KEY && > session->opt->server))) > { > - if (!key_method_2_write(buf, session)) > + if (!key_method_2_write(buf, multi, session)) > { > goto error; > } > I'll give this a spin and continue the review. Thx!
Hi, On 19/03/2021 16:31, Arne Schwabe wrote: > This option allow migration to a non compression server config while > still retraining compatibility with client that have a compression > setting in their config. > > For existing setups that used to have comp-lzo no or another > compression setting in their configs it is a difficult to migrate to > a setup without compression without replacing all client configs at > once especially if OpenVPN 2.3 or earlier clients are in the mix that > do not support pushing stub-v2. Even with OpenVPN 2.4 and later clients > that support pushing this is not a satisfying solution as the clients > log occ mismatches and the "push stub-v2" needs to be in the server > config "forever". As discussed on other channels, I think it would be beneficial to report in the commit message also the "what is this patch doing to achieve this goal?". Basically a shorter version of what you are already putting in the manpage. > > Signed-off-by: Arne Schwabe <arne@rfc2549.org> > --- > doc/man-sections/protocol-options.rst | 12 +++- > src/openvpn/comp.h | 1 + > src/openvpn/multi.c | 41 +++++++++++++ > src/openvpn/options.c | 6 ++ > src/openvpn/ssl.c | 34 ++++++++++- > src/openvpn/ssl_common.h | 1 + > src/openvpn/ssl_util.c | 43 ++++++++++++++ > src/openvpn/ssl_util.h | 15 +++++ > tests/unit_tests/openvpn/Makefile.am | 14 ++++- > tests/unit_tests/openvpn/test_misc.c | 83 +++++++++++++++++++++++++++ > 10 files changed, 245 insertions(+), 5 deletions(-) > create mode 100644 tests/unit_tests/openvpn/test_misc.c > > diff --git a/doc/man-sections/protocol-options.rst b/doc/man-sections/protocol-options.rst > index e9d5d63d..c5cd76dd 100644 > --- a/doc/man-sections/protocol-options.rst > +++ b/doc/man-sections/protocol-options.rst > @@ -84,10 +84,10 @@ configured in a compatible way between both the local and remote side. > --compress algorithm > **DEPRECATED** Enable a compression algorithm. Compression is generally > not recommended. VPN tunnels which use compression are susceptible to > - the VORALCE attack vector. > + the VORALCE attack vector. See also the :code:`migrate` parameter below. > > The ``algorithm`` parameter may be :code:`lzo`, :code:`lz4`, > - :code:`lz4-v2`, :code:`stub`, :code:`stub-v2` or empty. > + :code:`lz4-v2`, :code:`stub`, :code:`stub-v2`, :code:`migrate` or empty. > LZO and LZ4 are different compression algorithms, with LZ4 generally > offering the best performance with least CPU usage. > > @@ -106,6 +106,14 @@ configured in a compatible way between both the local and remote side. > Note: the :code:`stub` (or empty) option is NOT compatible with the older > option ``--comp-lzo no``. > > + The :code:`migrate` algorithm is a special and is intended to allow > + migration away from ``--compress``/``--comp-lzo`` options to no compression. > + This options set the server to no compression mode. If a client > + is detected that indicates that compression is used the will automatically typ0 - missing $something after 'the' > + add ``--push compress stub-v2`` to the client specific configuration > + if supported by the client and otherwise switch to ``comp-lzo no`` > + and also ``--push comp-lzo`` to the client specific configuration. > + > ***Security Considerations*** > > Compression and encryption is a tricky combination. If an attacker knows > diff --git a/src/openvpn/comp.h b/src/openvpn/comp.h > index 5c0322ca..a880e198 100644 > --- a/src/openvpn/comp.h > +++ b/src/openvpn/comp.h > @@ -58,6 +58,7 @@ > #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 */ > > > /* > diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c > index f7e0f680..3060410d 100644 > --- a/src/openvpn/multi.c > +++ b/src/openvpn/multi.c > @@ -2469,6 +2469,46 @@ multi_client_connect_early_setup(struct multi_context *m, > multi_client_connect_setenv(m, mi); > } > > +/** > + * Do the necessary modification for doing the compress mitigrate. This is typ0: mitigrate > + * implemented as a connect handler as it fits the modify config for a client > + * paradigm and also is early enough in the chain to be overwritten by another > + * ccd/script to do compression on a special client. > + */ > +static enum client_connect_return > +multi_client_connect_compress_migrate(struct multi_context *m, > + struct multi_instance *mi, > + bool deferred, > + unsigned int *option_types_found) > +{ > + struct options *o = &mi->context.options; > + const char *const peer_info =mi->context.c2.tls_multi->peer_info; space after = > + > + if (!peer_info) > + { > + return CC_RET_SUCCEEDED; > + } > + > + > + if (o->comp.flags & COMP_F_MIGRATE && mi->context.c2.tls_multi->remote_usescomp) > + { > + if(strstr(peer_info, "IV_COMP_STUBv2=1")) > + { > + push_option(o, "compress stub-v2", M_USAGE); > + } > + else > + { > + /* Client is old and does not support STUBv2 but since it > + * announced comp-lzo via OCC we assume it uses comp-lzo, so > + * switch to that and pushed the uncompressed variant. */ pushed -> push ? > + push_option(o, "comp-lzo no", M_USAGE); > + o->comp.alg = COMP_ALG_STUB; > + *option_types_found |= OPT_P_COMP; > + } > + } > + return CC_RET_SUCCEEDED; > +} > + > /** > * Try to source a dynamic config file from the > * --client-config-dir directory. > @@ -2537,6 +2577,7 @@ typedef enum client_connect_return (*multi_client_connect_handler) > bool from_deferred, unsigned int *option_types_found); > > static const multi_client_connect_handler client_connect_handlers[] = { > + multi_client_connect_compress_migrate, > multi_client_connect_source_ccd, > multi_client_connect_call_plugin_v1, > multi_client_connect_call_plugin_v2, > diff --git a/src/openvpn/options.c b/src/openvpn/options.c > index 86e78b05..f0d1e128 100644 > --- a/src/openvpn/options.c > +++ b/src/openvpn/options.c > @@ -7692,6 +7692,12 @@ add_option(struct options *options, > 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 */ > diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c > index 893e5753..7ab8d585 100644 > --- a/src/openvpn/ssl.c > +++ b/src/openvpn/ssl.c > @@ -60,6 +60,7 @@ > #include "ssl_verify.h" > #include "ssl_backend.h" > #include "ssl_ncp.h" > +#include "ssl_util.h" > #include "auth_token.h" > > #include "memdbg.h" > @@ -2235,6 +2236,16 @@ error: > return ret; > } > > +static bool > +write_compat_local_options(struct buffer *buf, const char *options) > +{ > + struct gc_arena gc = gc_new(); > + const char* local_options = options_string_compat_lzo(options, &gc); space should go before * and not after. > + bool ret = write_string(buf, local_options, TLS_OPTIONS_LEN); > + gc_free(&gc); > + return ret; > +} > + > /** > * Handle the writing of key data, peer-info, username/password, OCC > * to the TLS control channel (cleartext). > @@ -2266,7 +2277,15 @@ key_method_2_write(struct buffer *buf, struct tls_session *session) > > /* write options string */ > { > - if (!write_string(buf, session->opt->local_options, TLS_OPTIONS_LEN)) > + if (multi->remote_usescomp && session->opt->mode == MODE_SERVER > + && multi->opt.comp_options.flags & COMP_F_MIGRATE) > + { > + if (!write_compat_local_options(buf, session->opt->local_options)) > + { > + goto error; > + } > + } > + else if (!write_string(buf, session->opt->local_options, TLS_OPTIONS_LEN)) > { > goto error; > } > @@ -2460,6 +2479,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio > free(multi->remote_ciphername); > multi->remote_ciphername = > options_string_extract_option(options, "cipher", NULL); > + multi->remote_usescomp = strstr(options, ",comp-lzo,") != NULL; remote_usescomp is bool - we never do "strstr() != NULL" when we need a boolean result. > > /* In OCC we send '[null-cipher]' instead 'none' */ > if (multi->remote_ciphername > @@ -2509,7 +2529,17 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio > if (!session->opt->disable_occ > && !options_cmp_equal(options, session->opt->remote_options)) > { > - options_warning(options, session->opt->remote_options); > + const char *remote_options = session->opt->remote_options; > + if (multi->opt.comp_options.flags & COMP_F_MIGRATE && multi->remote_usescomp) > + { > + msg(D_SHOW_OCC, "Note: --comp-lzo migrate is enabled and remote " > + "announces comp-lzo, consider removing " > + "compress/comp-lzo options from remote config."); should we explain a bit more what is happening? i.e. compression is being forcefully disabled? > + remote_options = options_string_compat_lzo(remote_options, &gc); > + } > + > + options_warning(options, remote_options); > + > if (session->opt->ssl_flags & SSLF_OPT_VERIFY) > { > msg(D_TLS_ERRORS, "Option inconsistency warnings triggering disconnect due to --opt-verify"); > diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h > index 2e3c98db..5791b014 100644 > --- a/src/openvpn/ssl_common.h > +++ b/src/openvpn/ssl_common.h > @@ -574,6 +574,7 @@ struct tls_multi > bool use_peer_id; > > char *remote_ciphername; /**< cipher specified in peer's config file */ > + bool remote_usescomp; /**< remote announced comp-lzo in OCC string */ > > /* > * Our session objects. > diff --git a/src/openvpn/ssl_util.c b/src/openvpn/ssl_util.c > index f6e66be4..38b371f1 100644 > --- a/src/openvpn/ssl_util.c > +++ b/src/openvpn/ssl_util.c > @@ -75,3 +75,46 @@ extract_iv_proto(const char *peer_info) > } > return 0; > } > + > +const char * > +options_string_compat_lzo(const char *options, struct gc_arena *gc> +{ > + no need for this empty line? > + /* comp-lzo: 'V4,dev-type tun,link-mtu 1458,tun-mtu 1400,proto UDPv4,comp-lzo,auth SHA1,keysize 128,key-method 2,tls-server' */ > + /* w/o comp: 'V4,dev-type tun,link-mtu 1457,tun-mtu 1400,proto UDPv4,auth SHA1,keysize 128,key-method 2,tls-server' */ > + maybe this comment could be articulated a bit more? just to explain that this is also a sample input/output for this function. > + > + /* Note: since this function is used only in a very limited scope it makes > + * assumptions how the string looks. Since we locally generated the string > + * we can make these assumptions */ > + > + > + /* Check that the link-mtu string is in options */ > + const char* tmp = strstr(options, ",link-mtu"); space before the * and not after. > + if (!tmp) > + { > + return options; > + } > + > + /* Get old link_mtu size */ > + int link_mtu; > + if(sscanf(tmp, ",link-mtu %d,", &link_mtu) !=1 || link_mtu < 100 || link_mtu > 9000) - 9000 is sometimes used in some setups. How about having 9900 as upper bound? - space after if - space after != > + { > + return options; > + } > + > + struct buffer buf = alloc_buf_gc(strlen(options) + strlen(",comp-lzo") + 2, gc); as suggested, please add a comment to explain why +2 is needed > + > + no need for double empty line? > + buf_write(&buf, options, (int)(tmp - options)); > + > + /* Increase link-mtu by one for the comp-lzo opcode */ > + buf_printf(&buf, ",link-mtu %d", link_mtu + 1); > + > + tmp += strlen(",link-mtu ") + (link_mtu < 1000 ? 3 : 4); > + > + buf_printf(&buf, "%s,comp-lzo", tmp); > + > + return BSTR(&buf); > + no need for this empty line > +} > diff --git a/src/openvpn/ssl_util.h b/src/openvpn/ssl_util.h > index 741a7782..472aa591 100644 > --- a/src/openvpn/ssl_util.h > +++ b/src/openvpn/ssl_util.h > @@ -54,4 +54,19 @@ extract_var_peer_info(const char *peer_info, > */ > unsigned int > extract_iv_proto(const char *peer_info); > + > +/** > + * Takes a locally produced OCC string for TLS server mode and modifies as > + * if the option comp-lzo was enabled. This is to send a client in > + * comp-lzo migrate mode the expected OCC string. > + * > + * Note: This function excpets the string to be in the locally generated > + * format and does not accept arbitrary strings. > + * > + * @param options the locally generated OCC string > + * @param gc gc_arena to allocate the returned string in > + * @return the modified string or options on error > + */ > +const char * > +options_string_compat_lzo(const char *options, struct gc_arena *gc); > #endif > diff --git a/tests/unit_tests/openvpn/Makefile.am b/tests/unit_tests/openvpn/Makefile.am > index 50f3a02e..524ef50d 100644 > --- a/tests/unit_tests/openvpn/Makefile.am > +++ b/tests/unit_tests/openvpn/Makefile.am > @@ -6,7 +6,7 @@ if HAVE_LD_WRAP_SUPPORT > test_binaries += argv_testdriver buffer_testdriver > endif > > -test_binaries += crypto_testdriver packet_id_testdriver auth_token_testdriver ncp_testdriver > +test_binaries += crypto_testdriver packet_id_testdriver auth_token_testdriver ncp_testdriver misc_testdriver > if HAVE_LD_WRAP_SUPPORT > test_binaries += tls_crypt_testdriver > endif > @@ -127,3 +127,15 @@ ncp_testdriver_SOURCES = test_ncp.c mock_msg.c \ > $(openvpn_srcdir)/packet_id.c \ > $(openvpn_srcdir)/platform.c \ > $(openvpn_srcdir)/ssl_util.c > + > + no need for double empty line > +misc_testdriver_CFLAGS = @TEST_CFLAGS@ \ > + -I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) > + > +misc_testdriver_LDFLAGS = @TEST_LDFLAGS@ > + > +misc_testdriver_SOURCES = test_misc.c mock_msg.c \ > + mock_get_random.c \ > + $(openvpn_srcdir)/buffer.c \ > + $(openvpn_srcdir)/ssl_util.c \ > + $(openvpn_srcdir)/platform.c > \ No newline at end of file > diff --git a/tests/unit_tests/openvpn/test_misc.c b/tests/unit_tests/openvpn/test_misc.c > new file mode 100644 > index 00000000..4fcf82c1 > --- /dev/null > +++ b/tests/unit_tests/openvpn/test_misc.c > @@ -0,0 +1,83 @@ > +/* > + * OpenVPN -- An application to securely tunnel IP networks > + * over a single UDP port, with support for SSL/TLS-based > + * session authentication and key exchange, > + * packet encryption, packet authentication, and > + * packet compression. > + * > + * Copyright (C) 2019 Arne Schwabe <arne@rfc2549.org> since the file is being introduced now, maybe it should carry a more recent copyright year? > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 > + * as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License along > + * with this program; if not, write to the Free Software Foundation, Inc., > + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. > + */ > + > +#ifdef HAVE_CONFIG_H > +#include "config.h" > +#elif defined(_MSC_VER) > +#include "config-msvc.h" > +#endif > + > +#include "syshead.h" > + > +#include <stdio.h> > +#include <unistd.h> > +#include <stdlib.h> > +#include <stdarg.h> > +#include <string.h> > +#include <setjmp.h> > +#include <cmocka.h> > + > +#include "ssl_util.h" > + > +static void > +test_compat_lzo_string(void **state) > +{ > + struct gc_arena gc = gc_new(); > + > + const char* input = "V4,dev-type tun,link-mtu 1457,tun-mtu 1400,proto UDPv4,auth SHA1,keysize 128,key-method 2,tls-server"; > + > + const char* output = options_string_compat_lzo(input, &gc); > + > + assert_string_equal(output, "V4,dev-type tun,link-mtu 1458,tun-mtu 1400,proto UDPv4,auth SHA1,keysize 128,key-method 2,tls-server,comp-lzo"); > + > + /* This string is has a much too small link-mtu so we should fail on it" */ > + input = "V4,dev-type tun,link-mtu 2,tun-mtu 1400,proto UDPv4,auth SHA1,keysize 128,key-method 2,tls-server"; > + > + output = options_string_compat_lzo(input, &gc); > + > + assert_string_equal(input, output); > + > + /* not matching at all */ > + input = "V4,dev-type tun"; > + output = options_string_compat_lzo(input, &gc); > + > + assert_string_equal(input, output); > + > + > + input = "V4,dev-type tun,link-mtu 999,tun-mtu 1400,proto UDPv4,auth SHA1,keysize 128,key-method 2,tls-server"; > + output = options_string_compat_lzo(input, &gc); > + > + /* 999 -> 1000, 3 to 4 chars */ > + assert_string_equal(output, "V4,dev-type tun,link-mtu 1000,tun-mtu 1400,proto UDPv4,auth SHA1,keysize 128,key-method 2,tls-server,comp-lzo"); > + > +}; > + > +const struct CMUnitTest misc_tests[] = { > + cmocka_unit_test(test_compat_lzo_string), > +}; > + > +int > +main(void) > +{ > + return cmocka_run_group_tests(misc_tests, NULL, NULL); > +} > My comments are all basically about style only, because there is nothing else to report. The patch was discussed with Arne in a meeting and it represent a an important milestone that will allow people to migrate away from having compression enabled. This is very important, especially after it was realized that compression on the outer side of a VPN link can be dangerous. I tested this patch (after manually applying the fix as per Arne's suggestion) in the following scenarios: * server with --compress migrate 1) client without any compression enabled -> data flows; 2) client with --compress lzo/lz4 -> server pushes compress stub-v2 -> data flows; 3) client with --comp-lzo -> server pushes compress stub-v2 -> data flows; 4) client does not announce IV_COMP_STUBv2=1 and with --comp-lzo -> server pushes comp-lzo no -> data flows; 5) client does not announce IV_COMP_STUBv2=1 and with --compress lzo/lz4 (I don't think any client can do this) -> server pushes comp-lzo no -> data flows; Cheers,
diff --git a/doc/man-sections/protocol-options.rst b/doc/man-sections/protocol-options.rst index e9d5d63d..c5cd76dd 100644 --- a/doc/man-sections/protocol-options.rst +++ b/doc/man-sections/protocol-options.rst @@ -84,10 +84,10 @@ configured in a compatible way between both the local and remote side. --compress algorithm **DEPRECATED** Enable a compression algorithm. Compression is generally not recommended. VPN tunnels which use compression are susceptible to - the VORALCE attack vector. + the VORALCE attack vector. See also the :code:`migrate` parameter below. The ``algorithm`` parameter may be :code:`lzo`, :code:`lz4`, - :code:`lz4-v2`, :code:`stub`, :code:`stub-v2` or empty. + :code:`lz4-v2`, :code:`stub`, :code:`stub-v2`, :code:`migrate` or empty. LZO and LZ4 are different compression algorithms, with LZ4 generally offering the best performance with least CPU usage. @@ -106,6 +106,14 @@ configured in a compatible way between both the local and remote side. Note: the :code:`stub` (or empty) option is NOT compatible with the older option ``--comp-lzo no``. + The :code:`migrate` algorithm is a special and is intended to allow + migration away from ``--compress``/``--comp-lzo`` options to no compression. + This options set the server to no compression mode. If a client + is detected that indicates that compression is used the will automatically + add ``--push compress stub-v2`` to the client specific configuration + if supported by the client and otherwise switch to ``comp-lzo no`` + and also ``--push comp-lzo`` to the client specific configuration. + ***Security Considerations*** Compression and encryption is a tricky combination. If an attacker knows diff --git a/src/openvpn/comp.h b/src/openvpn/comp.h index 5c0322ca..a880e198 100644 --- a/src/openvpn/comp.h +++ b/src/openvpn/comp.h @@ -58,6 +58,7 @@ #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 */ /* diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index f7e0f680..3060410d 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -2469,6 +2469,46 @@ multi_client_connect_early_setup(struct multi_context *m, multi_client_connect_setenv(m, mi); } +/** + * Do the necessary modification for doing the compress mitigrate. This is + * implemented as a connect handler as it fits the modify config for a client + * paradigm and also is early enough in the chain to be overwritten by another + * ccd/script to do compression on a special client. + */ +static enum client_connect_return +multi_client_connect_compress_migrate(struct multi_context *m, + struct multi_instance *mi, + bool deferred, + unsigned int *option_types_found) +{ + struct options *o = &mi->context.options; + const char *const peer_info =mi->context.c2.tls_multi->peer_info; + + if (!peer_info) + { + return CC_RET_SUCCEEDED; + } + + + if (o->comp.flags & COMP_F_MIGRATE && mi->context.c2.tls_multi->remote_usescomp) + { + if(strstr(peer_info, "IV_COMP_STUBv2=1")) + { + push_option(o, "compress stub-v2", M_USAGE); + } + else + { + /* Client is old and does not support STUBv2 but since it + * announced comp-lzo via OCC we assume it uses comp-lzo, so + * switch to that and pushed the uncompressed variant. */ + push_option(o, "comp-lzo no", M_USAGE); + o->comp.alg = COMP_ALG_STUB; + *option_types_found |= OPT_P_COMP; + } + } + return CC_RET_SUCCEEDED; +} + /** * Try to source a dynamic config file from the * --client-config-dir directory. @@ -2537,6 +2577,7 @@ typedef enum client_connect_return (*multi_client_connect_handler) bool from_deferred, unsigned int *option_types_found); static const multi_client_connect_handler client_connect_handlers[] = { + multi_client_connect_compress_migrate, multi_client_connect_source_ccd, multi_client_connect_call_plugin_v1, multi_client_connect_call_plugin_v2, diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 86e78b05..f0d1e128 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -7692,6 +7692,12 @@ add_option(struct options *options, 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 */ diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 893e5753..7ab8d585 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -60,6 +60,7 @@ #include "ssl_verify.h" #include "ssl_backend.h" #include "ssl_ncp.h" +#include "ssl_util.h" #include "auth_token.h" #include "memdbg.h" @@ -2235,6 +2236,16 @@ error: return ret; } +static bool +write_compat_local_options(struct buffer *buf, const char *options) +{ + struct gc_arena gc = gc_new(); + const char* local_options = options_string_compat_lzo(options, &gc); + bool ret = write_string(buf, local_options, TLS_OPTIONS_LEN); + gc_free(&gc); + return ret; +} + /** * Handle the writing of key data, peer-info, username/password, OCC * to the TLS control channel (cleartext). @@ -2266,7 +2277,15 @@ key_method_2_write(struct buffer *buf, struct tls_session *session) /* write options string */ { - if (!write_string(buf, session->opt->local_options, TLS_OPTIONS_LEN)) + if (multi->remote_usescomp && session->opt->mode == MODE_SERVER + && multi->opt.comp_options.flags & COMP_F_MIGRATE) + { + if (!write_compat_local_options(buf, session->opt->local_options)) + { + goto error; + } + } + else if (!write_string(buf, session->opt->local_options, TLS_OPTIONS_LEN)) { goto error; } @@ -2460,6 +2479,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio free(multi->remote_ciphername); multi->remote_ciphername = options_string_extract_option(options, "cipher", NULL); + multi->remote_usescomp = strstr(options, ",comp-lzo,") != NULL; /* In OCC we send '[null-cipher]' instead 'none' */ if (multi->remote_ciphername @@ -2509,7 +2529,17 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio if (!session->opt->disable_occ && !options_cmp_equal(options, session->opt->remote_options)) { - options_warning(options, session->opt->remote_options); + const char *remote_options = session->opt->remote_options; + if (multi->opt.comp_options.flags & COMP_F_MIGRATE && multi->remote_usescomp) + { + msg(D_SHOW_OCC, "Note: --comp-lzo migrate is enabled and remote " + "announces comp-lzo, consider removing " + "compress/comp-lzo options from remote config."); + remote_options = options_string_compat_lzo(remote_options, &gc); + } + + options_warning(options, remote_options); + if (session->opt->ssl_flags & SSLF_OPT_VERIFY) { msg(D_TLS_ERRORS, "Option inconsistency warnings triggering disconnect due to --opt-verify"); diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h index 2e3c98db..5791b014 100644 --- a/src/openvpn/ssl_common.h +++ b/src/openvpn/ssl_common.h @@ -574,6 +574,7 @@ struct tls_multi bool use_peer_id; char *remote_ciphername; /**< cipher specified in peer's config file */ + bool remote_usescomp; /**< remote announced comp-lzo in OCC string */ /* * Our session objects. diff --git a/src/openvpn/ssl_util.c b/src/openvpn/ssl_util.c index f6e66be4..38b371f1 100644 --- a/src/openvpn/ssl_util.c +++ b/src/openvpn/ssl_util.c @@ -75,3 +75,46 @@ extract_iv_proto(const char *peer_info) } return 0; } + +const char * +options_string_compat_lzo(const char *options, struct gc_arena *gc) +{ + + /* comp-lzo: 'V4,dev-type tun,link-mtu 1458,tun-mtu 1400,proto UDPv4,comp-lzo,auth SHA1,keysize 128,key-method 2,tls-server' */ + /* w/o comp: 'V4,dev-type tun,link-mtu 1457,tun-mtu 1400,proto UDPv4,auth SHA1,keysize 128,key-method 2,tls-server' */ + + + /* Note: since this function is used only in a very limited scope it makes + * assumptions how the string looks. Since we locally generated the string + * we can make these assumptions */ + + + /* Check that the link-mtu string is in options */ + const char* tmp = strstr(options, ",link-mtu"); + if (!tmp) + { + return options; + } + + /* Get old link_mtu size */ + int link_mtu; + if(sscanf(tmp, ",link-mtu %d,", &link_mtu) !=1 || link_mtu < 100 || link_mtu > 9000) + { + return options; + } + + struct buffer buf = alloc_buf_gc(strlen(options) + strlen(",comp-lzo") + 2, gc); + + + buf_write(&buf, options, (int)(tmp - options)); + + /* Increase link-mtu by one for the comp-lzo opcode */ + buf_printf(&buf, ",link-mtu %d", link_mtu + 1); + + tmp += strlen(",link-mtu ") + (link_mtu < 1000 ? 3 : 4); + + buf_printf(&buf, "%s,comp-lzo", tmp); + + return BSTR(&buf); + +} diff --git a/src/openvpn/ssl_util.h b/src/openvpn/ssl_util.h index 741a7782..472aa591 100644 --- a/src/openvpn/ssl_util.h +++ b/src/openvpn/ssl_util.h @@ -54,4 +54,19 @@ extract_var_peer_info(const char *peer_info, */ unsigned int extract_iv_proto(const char *peer_info); + +/** + * Takes a locally produced OCC string for TLS server mode and modifies as + * if the option comp-lzo was enabled. This is to send a client in + * comp-lzo migrate mode the expected OCC string. + * + * Note: This function excpets the string to be in the locally generated + * format and does not accept arbitrary strings. + * + * @param options the locally generated OCC string + * @param gc gc_arena to allocate the returned string in + * @return the modified string or options on error + */ +const char * +options_string_compat_lzo(const char *options, struct gc_arena *gc); #endif diff --git a/tests/unit_tests/openvpn/Makefile.am b/tests/unit_tests/openvpn/Makefile.am index 50f3a02e..524ef50d 100644 --- a/tests/unit_tests/openvpn/Makefile.am +++ b/tests/unit_tests/openvpn/Makefile.am @@ -6,7 +6,7 @@ if HAVE_LD_WRAP_SUPPORT test_binaries += argv_testdriver buffer_testdriver endif -test_binaries += crypto_testdriver packet_id_testdriver auth_token_testdriver ncp_testdriver +test_binaries += crypto_testdriver packet_id_testdriver auth_token_testdriver ncp_testdriver misc_testdriver if HAVE_LD_WRAP_SUPPORT test_binaries += tls_crypt_testdriver endif @@ -127,3 +127,15 @@ ncp_testdriver_SOURCES = test_ncp.c mock_msg.c \ $(openvpn_srcdir)/packet_id.c \ $(openvpn_srcdir)/platform.c \ $(openvpn_srcdir)/ssl_util.c + + +misc_testdriver_CFLAGS = @TEST_CFLAGS@ \ + -I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) + +misc_testdriver_LDFLAGS = @TEST_LDFLAGS@ + +misc_testdriver_SOURCES = test_misc.c mock_msg.c \ + mock_get_random.c \ + $(openvpn_srcdir)/buffer.c \ + $(openvpn_srcdir)/ssl_util.c \ + $(openvpn_srcdir)/platform.c \ No newline at end of file diff --git a/tests/unit_tests/openvpn/test_misc.c b/tests/unit_tests/openvpn/test_misc.c new file mode 100644 index 00000000..4fcf82c1 --- /dev/null +++ b/tests/unit_tests/openvpn/test_misc.c @@ -0,0 +1,83 @@ +/* + * OpenVPN -- An application to securely tunnel IP networks + * over a single UDP port, with support for SSL/TLS-based + * session authentication and key exchange, + * packet encryption, packet authentication, and + * packet compression. + * + * Copyright (C) 2019 Arne Schwabe <arne@rfc2549.org> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#ifdef HAVE_CONFIG_H +#include "config.h" +#elif defined(_MSC_VER) +#include "config-msvc.h" +#endif + +#include "syshead.h" + +#include <stdio.h> +#include <unistd.h> +#include <stdlib.h> +#include <stdarg.h> +#include <string.h> +#include <setjmp.h> +#include <cmocka.h> + +#include "ssl_util.h" + +static void +test_compat_lzo_string(void **state) +{ + struct gc_arena gc = gc_new(); + + const char* input = "V4,dev-type tun,link-mtu 1457,tun-mtu 1400,proto UDPv4,auth SHA1,keysize 128,key-method 2,tls-server"; + + const char* output = options_string_compat_lzo(input, &gc); + + assert_string_equal(output, "V4,dev-type tun,link-mtu 1458,tun-mtu 1400,proto UDPv4,auth SHA1,keysize 128,key-method 2,tls-server,comp-lzo"); + + /* This string is has a much too small link-mtu so we should fail on it" */ + input = "V4,dev-type tun,link-mtu 2,tun-mtu 1400,proto UDPv4,auth SHA1,keysize 128,key-method 2,tls-server"; + + output = options_string_compat_lzo(input, &gc); + + assert_string_equal(input, output); + + /* not matching at all */ + input = "V4,dev-type tun"; + output = options_string_compat_lzo(input, &gc); + + assert_string_equal(input, output); + + + input = "V4,dev-type tun,link-mtu 999,tun-mtu 1400,proto UDPv4,auth SHA1,keysize 128,key-method 2,tls-server"; + output = options_string_compat_lzo(input, &gc); + + /* 999 -> 1000, 3 to 4 chars */ + assert_string_equal(output, "V4,dev-type tun,link-mtu 1000,tun-mtu 1400,proto UDPv4,auth SHA1,keysize 128,key-method 2,tls-server,comp-lzo"); + +}; + +const struct CMUnitTest misc_tests[] = { + cmocka_unit_test(test_compat_lzo_string), +}; + +int +main(void) +{ + return cmocka_run_group_tests(misc_tests, NULL, NULL); +}
This option allow migration to a non compression server config while still retraining compatibility with client that have a compression setting in their config. For existing setups that used to have comp-lzo no or another compression setting in their configs it is a difficult to migrate to a setup without compression without replacing all client configs at once especially if OpenVPN 2.3 or earlier clients are in the mix that do not support pushing stub-v2. Even with OpenVPN 2.4 and later clients that support pushing this is not a satisfying solution as the clients log occ mismatches and the "push stub-v2" needs to be in the server config "forever". Signed-off-by: Arne Schwabe <arne@rfc2549.org> --- doc/man-sections/protocol-options.rst | 12 +++- src/openvpn/comp.h | 1 + src/openvpn/multi.c | 41 +++++++++++++ src/openvpn/options.c | 6 ++ src/openvpn/ssl.c | 34 ++++++++++- src/openvpn/ssl_common.h | 1 + src/openvpn/ssl_util.c | 43 ++++++++++++++ src/openvpn/ssl_util.h | 15 +++++ tests/unit_tests/openvpn/Makefile.am | 14 ++++- tests/unit_tests/openvpn/test_misc.c | 83 +++++++++++++++++++++++++++ 10 files changed, 245 insertions(+), 5 deletions(-) create mode 100644 tests/unit_tests/openvpn/test_misc.c