[Openvpn-devel,3/3] Print a --verb 1 warning when a connection uses compression

Message ID 1528020718-12721-3-git-send-email-steffan@karger.me
State Superseded
Headers show
Series
  • [Openvpn-devel,1/3] man: add security considerations to --compress section
Related show

Commit Message

Steffan Karger June 3, 2018, 10:11 a.m.
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 <steffan@karger.me>
---
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(-)

Comments

Selva Nair June 3, 2018, 3:51 p.m. | #1
Hi,

On Sun, Jun 3, 2018 at 6:11 AM, Steffan Karger <steffan@karger.me> 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 <steffan@karger.me>
> ---
> 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
Arne Schwabe June 3, 2018, 6:06 p.m. | #2
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
David Sommerseth June 4, 2018, 5:57 p.m. | #3
On 03/06/18 12:11, 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 <steffan@karger.me>
> ---
> 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
> 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. "

Did you mean /insecure/?

Patch

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 */
 
 
 /*