From patchwork Sun Jun 3 10:11:56 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [Openvpn-devel,1/3] man: add security considerations to --compress section X-Patchwork-Submitter: Steffan Karger X-Patchwork-Id: 340 Message-Id: <1528020718-12721-1-git-send-email-steffan@karger.me> To: openvpn-devel@lists.sourceforge.net Date: Sun, 3 Jun 2018 12:11:56 +0200 From: Steffan Karger List-Id: As Ahamed Nafeez reported to the OpenVPN security team, we did not sufficiently inform our users about the risks of combining encryption and compression. This patch adds a "Security Considerations" paragraph to the --compress section of the manpage to point the risks out to our users. Signed-off-by: Steffan Karger Acked-By: Gert Doering --- doc/openvpn.8 | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/doc/openvpn.8 b/doc/openvpn.8 index 4114f40..0e5d467 100644 --- a/doc/openvpn.8 +++ b/doc/openvpn.8 @@ -2516,6 +2516,16 @@ If the parameter is empty, compression will be turned off, but the packet framing for compression will still be enabled, allowing a different setting to be pushed later. + +.B Security Considerations + +Compression and encryption is a tricky combination. If an attacker knows or is +able to control (parts of) the plaintext of packets that contain secrets, the +attacker might be able to extract the secret if compression is enabled. See +e.g. the CRIME and BREACH attacks on TLS which also leverage compression to +break encryption. If you are not entirely sure that the above does not apply +to your traffic, you are advised to *not* enable compression. + .\"********************************************************* .TP .B \-\-comp\-lzo [mode] From patchwork Sun Jun 3 10:11:57 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [Openvpn-devel,2/3] Reject unadvertised compression algorithms X-Patchwork-Submitter: Steffan Karger X-Patchwork-Id: 339 Message-Id: <1528020718-12721-2-git-send-email-steffan@karger.me> To: openvpn-devel@lists.sourceforge.net Date: Sun, 3 Jun 2018 12:11:57 +0200 From: Steffan Karger List-Id: A server should not push us compression algorithms we didn't specify. If the server does so anyway, reject the compression algorithm. This will result in a warning being printed, and a non-working connection to be set up. This is currently our way to "handle push/pull errors", which should probably be improved. But I didn't want refactor that in this patch. Signed-off-by: Steffan Karger --- doc/openvpn.8 | 16 +++++++--- src/openvpn/options.c | 85 ++++++++++++++++++++++++++++++++------------------- 2 files changed, 65 insertions(+), 36 deletions(-) diff --git a/doc/openvpn.8 b/doc/openvpn.8 index 0e5d467..9e988b3 100644 --- a/doc/openvpn.8 +++ b/doc/openvpn.8 @@ -2505,11 +2505,12 @@ Enable a compression algorithm. The .B algorithm -parameter may be "lzo", "lz4", or empty. LZO and LZ4 -are different compression algorithms, with LZ4 generally -offering the best performance with least CPU usage. -For backwards compatibility with OpenVPN versions before v2.4, use "lzo" -(which is identical to the older option "\-\-comp\-lzo yes"). +parameter may be empty, "stub", "stub-v2", "lzo", "lz4", or "lz4-v2". + +LZO and LZ4 are different compression algorithms, with LZ4 generally offering +the best performance with least CPU usage. For backwards compatibility with +OpenVPN versions before v2.4, use "lzo" (which is identical to the older option +"\-\-comp\-lzo yes"). If the .B algorithm @@ -2517,6 +2518,11 @@ parameter is empty, compression will be turned off, but the packet framing for compression will still be enabled, allowing a different setting to be pushed later. +If the +.B algorithm +parameter is "stub" or "stub-v2", compression framing is enabled, but no +compression will be used (even if pushed by the server). + .B Security Considerations Compression and encryption is a tricky combination. If an attacker knows or is diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 426057a..ad44f8e 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -7354,50 +7354,73 @@ add_option(struct options *options, VERIFY_PERMISSION(OPT_P_COMP); options->comp.flags &= ~COMP_F_ADAPTIVE; } - else if (streq(p[0], "compress") && !p[2]) + else if (streq(p[0], "compress") && !p[3]) { VERIFY_PERMISSION(OPT_P_COMP); - if (p[1]) + + /* Reset all compression flags, except "stubs only" and "no warn" if + * this option was pushed. */ + if (streq(file, "[PUSH-OPTIONS]")) + { + options->comp.flags = options->comp.flags + & (COMP_F_ADVERTISE_STUBS_ONLY|COMP_F_NOWARN); + } + + /* Parse supplied compression options */ + if (!p[1]) { - if (streq(p[1], "stub")) + options->comp.alg = COMP_ALG_STUB; + options->comp.flags |= COMP_F_SWAP; + } + else if (streq(p[1], "stub")) + { + options->comp.alg = COMP_ALG_STUB; + options->comp.flags |= COMP_F_SWAP; + if (!streq(file, "[PUSH-OPTIONS]")) { - options->comp.alg = COMP_ALG_STUB; - options->comp.flags = (COMP_F_SWAP|COMP_F_ADVERTISE_STUBS_ONLY); + options->comp.flags |= COMP_F_ADVERTISE_STUBS_ONLY; } - else if (streq(p[1], "stub-v2")) + } + else if (streq(p[1], "stub-v2")) + { + options->comp.alg = COMP_ALGV2_UNCOMPRESSED; + if (!streq(file, "[PUSH-OPTIONS]")) { - options->comp.alg = COMP_ALGV2_UNCOMPRESSED; - options->comp.flags = COMP_F_ADVERTISE_STUBS_ONLY; + options->comp.flags |= COMP_F_ADVERTISE_STUBS_ONLY; } + } + else if (options->comp.flags & COMP_F_ADVERTISE_STUBS_ONLY) + { + /* Reject pushed compression algorithms if explicitly disabled */ + msg(msglevel, "Enabling compression not allowed!"); + goto err; + } #if defined(ENABLE_LZO) - else if (streq(p[1], "lzo")) - { - options->comp.alg = COMP_ALG_LZO; - options->comp.flags = 0; - } + else if (streq(p[1], "lzo")) + { + options->comp.alg = COMP_ALG_LZO; + } #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; - options->comp.flags = 0; - } -#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; + } + + if (p[2] && streq(p[2], "nowarn")) + { + options->comp.flags |= COMP_F_NOWARN; } } #endif /* USE_COMP */ From patchwork Sun Jun 3 10:11:58 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [Openvpn-devel,3/3] Print a --verb 1 warning when a connection uses compression X-Patchwork-Submitter: Steffan Karger X-Patchwork-Id: 341 Message-Id: <1528020718-12721-3-git-send-email-steffan@karger.me> To: openvpn-devel@lists.sourceforge.net Date: Sun, 3 Jun 2018 12:11:58 +0200 From: Steffan Karger List-Id: Can be suppressed by adding a "nowarn" flag to the compress options, for those that are really sure that compression is fine for their use case. Signed-off-by: Steffan Karger --- This patch is also meant to discuss how far we want to go in warning users about using compression. I think this approach is reasonable, but I'm not sure everyone agrees. doc/openvpn.8 | 11 +++++++++-- src/openvpn/comp.c | 14 ++++++++++++++ src/openvpn/comp.h | 1 + 3 files changed, 24 insertions(+), 2 deletions(-) diff --git a/doc/openvpn.8 b/doc/openvpn.8 index 9e988b3..21a3c42 100644 --- a/doc/openvpn.8 +++ b/doc/openvpn.8 @@ -2500,12 +2500,13 @@ consecutive messages in the same category. This is useful to limit repetitive logging of similar message types. .\"********************************************************* .TP -.B \-\-compress [algorithm] +.B \-\-compress [algorithm] ["nowarn"] Enable a compression algorithm. The .B algorithm -parameter may be empty, "stub", "stub-v2", "lzo", "lz4", or "lz4-v2". +parameter may be empty, "any", "stub", "stub-v2", "lzo", "lz4", or "lz4-v2". +If left empty, OpenVPN defaults to "any". LZO and LZ4 are different compression algorithms, with LZ4 generally offering the best performance with least CPU usage. For backwards compatibility with @@ -2532,6 +2533,12 @@ e.g. the CRIME and BREACH attacks on TLS which also leverage compression to break encryption. If you are not entirely sure that the above does not apply to your traffic, you are advised to *not* enable compression. +If you have carefully considered the above, and are sure that using compression +is safe for your use case, you can add +.B "nowarn" +as the second parameter to suppress warnings about the risk of enabling +compression. + .\"********************************************************* .TP .B \-\-comp\-lzo [mode] diff --git a/src/openvpn/comp.c b/src/openvpn/comp.c index a945913..a34e64a 100644 --- a/src/openvpn/comp.c +++ b/src/openvpn/comp.c @@ -40,6 +40,20 @@ struct compress_context * comp_init(const struct compress_options *opt) { + switch (opt->alg) + { + case COMP_ALG_UNDEF: + case COMP_ALG_STUB: + case COMP_ALGV2_UNCOMPRESSED: + break; + default: + if (!(opt->flags & COMP_F_NOWARN)) + { + msg(M_INFO, "WARNING: Compression enabled, might be insure. " + "See --compress in the man page."); + } + } + struct compress_context *compctx = NULL; switch (opt->alg) { diff --git a/src/openvpn/comp.h b/src/openvpn/comp.h index 0dadd1e..0fa9b10 100644 --- a/src/openvpn/comp.h +++ b/src/openvpn/comp.h @@ -56,6 +56,7 @@ #define COMP_F_ASYM (1<<1) /* only downlink is compressed, not 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_NOWARN (1<<4) /* Suppress warning about insure compression */ /*