Re: [Openvpn-devel] [PATCH 2/3] Reject unadvertised compression algorithms
Hi On Sun, Jun 03, 2018 at 12:48:36PM +0200, Gert Doering wrote: > On Sun, Jun 03, 2018 at 12:11:57PM +0200, Steffan Karger wrote: > > A server should not push us compression algorithms we didn't specify. If > > the server does so anyway, reject the compression algorithm. > > I can see why you do this, but if I understand this right, this will > break lots of currently-working OpenVPN setups - where we use the IV_ > variables to parse out "what can the client do?" and send matching > "compress " PUSH_REPLYs. > > So, feature-NAK. As clarified on IRC, I misread the text. So I withdraw the feature-NAK and need to read this more closely and better think through the problem space. gert -- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress Gert Doering - Munich, Germany g...@greenie.muc.de signature.asc Description: PGP signature -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 1/2] make tls-auth a per-connection-block option
Hi, On 02-06-18 05:42, Antonio Quartulli wrote: > Different VPN servers may use different tls-auth keys. For this > reason it is convenient to make tls-auth a per-connection-block > option so that the user is allowed to specify one key per remote. Want! This also helps with tls-auth key rollover. Feature-ACK. > If no tls-auth option is specified in a given connection block, > the global one, if any, is used. > > Trac: #720 > Cc: Steffan Karger > Signed-off-by: Antonio Quartulli > --- > doc/openvpn.8 | 1 + > src/openvpn/init.c| 10 +++--- > src/openvpn/options.c | 82 ++- > src/openvpn/options.h | 5 +++ > 4 files changed, 77 insertions(+), 21 deletions(-) > > diff --git a/doc/openvpn.8 b/doc/openvpn.8 > index 4114f408..e7bc3f4f 100644 > --- a/doc/openvpn.8 > +++ b/doc/openvpn.8 > @@ -372,6 +372,7 @@ block: > .B remote, > .B rport, > .B socks\-proxy, > +.B tls\-auth, > .B tun\-mtu and > .B tun\-mtu\-extra. Shouldn't this also include key-direction? (Didn't really review or test yet, but otherwise looks good at first glance.) -Steffan -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 3/3] Print a --verb 1 warning when a connection uses compression
Hi,
On Sun, Jun 3, 2018 at 6:11 AM, Steffan Karger wrote:
> 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. "
May I suggest to change that to M_INFO|M_WARN so that it gets the
warning tag "W" that could be parsed by UI's. OpenVPN Windows GUI we
like to highlight (in colour) warnings and errors and this will slip
through as a info message.
Ideally we should redefine M_WARN as M_WARN + verb1 or make a
M_WARN_GENERIC that prints only in verb 1 and use it everywhere, but
that is a different topic.
Selva
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 1/2] make tls-auth a per-connection-block option
Hi, On 03/06/18 16:27, Steffan Karger wrote: > Hi, > > On 02-06-18 05:42, Antonio Quartulli wrote: >> Different VPN servers may use different tls-auth keys. For this >> reason it is convenient to make tls-auth a per-connection-block >> option so that the user is allowed to specify one key per remote. > > Want! This also helps with tls-auth key rollover. Feature-ACK. > >> If no tls-auth option is specified in a given connection block, >> the global one, if any, is used. >> >> Trac: #720 >> Cc: Steffan Karger >> Signed-off-by: Antonio Quartulli >> --- >> doc/openvpn.8 | 1 + >> src/openvpn/init.c| 10 +++--- >> src/openvpn/options.c | 82 ++- >> src/openvpn/options.h | 5 +++ >> 4 files changed, 77 insertions(+), 21 deletions(-) >> >> diff --git a/doc/openvpn.8 b/doc/openvpn.8 >> index 4114f408..e7bc3f4f 100644 >> --- a/doc/openvpn.8 >> +++ b/doc/openvpn.8 >> @@ -372,6 +372,7 @@ block: >> .B remote, >> .B rport, >> .B socks\-proxy, >> +.B tls\-auth, >> .B tun\-mtu and >> .B tun\-mtu\-extra. > > Shouldn't this also include key-direction? > good catch! I added the manpage change at the end and I forgot about key-direction. Will wait a bit more before sending v2. > (Didn't really review or test yet, but otherwise looks good at first > glance.) > Thanks so far Cheers, -- Antonio Quartulli signature.asc Description: OpenPGP digital signature -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH 3/3] Print a --verb 1 warning when a connection uses compression
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
*/
/*
--
2.7.4
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH 2/3] Reject unadvertised compression algorithms
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;
}
[Openvpn-devel] [PATCH 1/3] man: add security considerations to --compress section
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 --- 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] -- 2.7.4 -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 2/3] Reject unadvertised compression algorithms
Hi, On Sun, Jun 03, 2018 at 12:11:57PM +0200, Steffan Karger wrote: > A server should not push us compression algorithms we didn't specify. If > the server does so anyway, reject the compression algorithm. I can see why you do this, but if I understand this right, this will break lots of currently-working OpenVPN setups - where we use the IV_ variables to parse out "what can the client do?" and send matching "compress " PUSH_REPLYs. So, feature-NAK. Adding warnings (1/3) and a warning-override (3/3) is good measure, but interfering with server-to-client pushing of options needs more consideration. gert -- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress Gert Doering - Munich, Germany g...@greenie.muc.de signature.asc Description: PGP signature -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH applied] Re: man: add security considerations to --compress section
This makes sense. Whatever else we do, explaining the *why* parts is helping users make an educated choice. Acked-By: Gert Doering Your patch has been applied to the master and release/2.4 branch. commit a59fd1475089eda4c89942d345070bb942180223 (master) commit 6795a5f3d55f658fc1a28eb9f3b11d1217e3329c (release/2.4) Author: Steffan Karger Date: Sun Jun 3 12:11:56 2018 +0200 man: add security considerations to --compress section Signed-off-by: Steffan Karger Acked-by: Gert Doering Message-Id: <1528020718-12721-1-git-send-email-stef...@karger.me> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg16919.html Signed-off-by: Gert Doering -- kind regards, Gert Doering -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 3/3] Print a --verb 1 warning when a connection uses compression
Am 03.06.18 um 12:11 schrieb Steffan Karger: > +msg(M_INFO, "WARNING: Compression enabled, might be insure. " > +"See --compress in the man page."); With my client maintainer hat on, this message is too vague. People will ask why compression is insure, because the warning message does not really tell much. I think the warning message should be more verbose something like WARNING: Compression enabled, Compression has been used in the past to break encryption and for strengthening security, it is recommended to disable it. For more details see --compress in the man page Arne -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 1/2] make tls-auth a per-connection-block option
Hi all, On 02/06/18 11:42, Antonio Quartulli wrote: > Different VPN servers may use different tls-auth keys. For this > reason it is convenient to make tls-auth a per-connection-block > option so that the user is allowed to specify one key per remote. > > If no tls-auth option is specified in a given connection block, > the global one, if any, is used. > > Trac: #720 > Cc: Steffan Karger > Signed-off-by: Antonio Quartulli as reported by Steffan on IRC, this feature breaks when using "--persist-key". It happens because, when moving to the next connection block, OpenVPN won't load the new tls-auth key and therefore will trigger an assertion. After further discussing this issue, it was agreed that we have two main options to tackle this issue: 1) pre-load all the tls-auth keyfiles (like if they were embedded in the config file) 2) make per-connection-block tls-auth keys mutually exclusive with --persist-key while point 2) would be the easiest option and would require the least amount of code, we believe that 1) is still the best from the user perspective and from the option semantics point of view (as it would not lead to any behaviour change). Therefore a v2 patch will be sent implementing approach 1). Cheers, -- Antonio Quartulli signature.asc Description: OpenPGP digital signature -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
