diff mbox series

[Openvpn-devel,master+release/2.5] Tune down verbosity for suspected retransmits

Message ID 20211117035321.249-1-lstipakov@gmail.com
State Rejected
Headers show
Series [Openvpn-devel,master+release/2.5] Tune down verbosity for suspected retransmits | expand

Commit Message

Lev Stipakov Nov. 16, 2021, 4:53 p.m. UTC 
From: Lev Stipakov <lev@openvpn.net>

There are cases when control packet is not acked fast enough,
for example when handling PUSH_REPLY, which requires setting up
tunnel. In those cases packet will be retransmitted.

OpenVPN 2 changes packet-id on retransmission, so it passes
replay protection and got rejected as a replay by reliability layer,
which checks another packet-id (sequence id) which is used to assemble
our TCP-like stream.

OpenVPN 3, however, doesn't change packet-id on retransmission,
which triggers replay protection and causes level 1 nonfatal errors
in logs.

When replay protection sees the packet with the same timestamp
and packet-id as previously received one, this is likely retransmission
from OpenVPN 3. To not to scare users, tune verbosity down in this case.

Signed-off-by: Lev Stipakov <lev@openvpn.net>
---
 src/openvpn/crypto.c    | 5 ++++-
 src/openvpn/packet_id.c | 3 +++
 src/openvpn/packet_id.h | 1 +
 src/openvpn/ssl.c       | 6 ++++--
 4 files changed, 12 insertions(+), 3 deletions(-)

Comments

Gert Doering Nov. 21, 2021, 9:22 p.m. UTC | #1 
Hi,

On Wed, Nov 17, 2021 at 05:53:21AM +0200, Lev Stipakov wrote:
> OpenVPN 3, however, doesn't change packet-id on retransmission,
> which triggers replay protection and causes level 1 nonfatal errors
> in logs.
> 
> When replay protection sees the packet with the same timestamp
> and packet-id as previously received one, this is likely retransmission
> from OpenVPN 3. To not to scare users, tune verbosity down in this case.

Wouldn't "fix this in OpenVPN 3, instead of hiding the warning in 2.x"
be a better approach?

This complicates 2.x code, which is complicated enough...

(If merged at all, this would not match our criteria for release/2.5 
either - it's neither a bugfix, nor a long-term compatibility thing)

gert
Lev Stipakov Nov. 21, 2021, 11:20 p.m. UTC | #2 
I've discussed this with James, in his opinion it is not needed to be
fixed in openvpn3 server side, since those duplicated packets are
dropped anyway in openvpn2/3 clients. OpenVPN3 client doesn't display
those errors but increments error counter, which is then dumped to log
on disconnect. The problem is mostly with openvpn2 client, which
displays those replay errors with very high verbosity levels:

  #define D_REPLAY_ERRORS      LOGLEV(1, 6, M_NONFATAL)   /* show
packet replay errors */
  #define D_TLS_ERRORS         LOGLEV(1, 3, M_NONFATAL)   /* show TLS
control channel errors */

The first one can be muted with --mute-replay-warnings, but we still
have the second one.

--
-Lev
Gert Doering Nov. 21, 2021, 11:43 p.m. UTC | #3 
Hi,

On Mon, Nov 22, 2021 at 12:20:08PM +0200, Lev Stipakov wrote:
> I've discussed this with James, in his opinion it is not needed to be
> fixed in openvpn3 server side, since those duplicated packets are
> dropped anyway in openvpn2/3 clients. 

Well.  I'd say this is a matter of protocol correctness - if the 
protocol requires something, and OpenVPN 3 Server does something
else, it needs to be fixed in 3.

No matter if "3 clients" will drop this without warning...

(And maybe if they would *not* drop it, it would be even better?)

gert
diff mbox series

Patch

diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index 1dfc760f..5a0775c1 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -335,7 +335,10 @@  crypto_check_replay(struct crypto_options *opt,
     {
         if (!(opt->flags & CO_MUTE_REPLAY_WARNINGS))
         {
-            msg(D_REPLAY_ERRORS, "%s: bad packet ID (may be a replay): %s -- "
+            /* openvpn3 doesn't change packet-id on retransmit, this is
+             * likely the case so tune verbosity down */
+            int verb = opt->packet_id.rec.retransmit ? D_PID_DEBUG : D_REPLAY_ERRORS;
+            msg(verb, "%s: bad packet ID (may be a %s): %s -- "
                 "see the man page entry for --no-replay and --replay-window for "
                 "more info or silence this warning with --mute-replay-warnings",
                 error_prefix, packet_id_net_print(pin, true, gc));
diff --git a/src/openvpn/packet_id.c b/src/openvpn/packet_id.c
index 19bf3c51..09434bbb 100644
--- a/src/openvpn/packet_id.c
+++ b/src/openvpn/packet_id.c
@@ -201,6 +201,8 @@  packet_id_test(struct packet_id_rec *p,
 {
     packet_id_type diff;
 
+    p->retransmit = false;
+
     packet_id_debug(D_PID_DEBUG, p, pin, "PID_TEST", 0);
 
     ASSERT(p->initialized);
@@ -250,6 +252,7 @@  packet_id_test(struct packet_id_rec *p,
                 }
                 else
                 {
+                    p->retransmit = true;
                     /* raised from D_PID_DEBUG_LOW to reduce verbosity */
                     packet_id_debug(D_PID_DEBUG_MEDIUM, p, pin, "PID_ERR replay", diff);
                     return false;
diff --git a/src/openvpn/packet_id.h b/src/openvpn/packet_id.h
index 8f705964..e47d671f 100644
--- a/src/openvpn/packet_id.h
+++ b/src/openvpn/packet_id.h
@@ -141,6 +141,7 @@  struct packet_id_rec
     struct seq_list *seq_list; /* packet-id "memory" */
     const char *name;
     int unit;
+    bool retransmit;          /* true if last packet is suspected retransmit */
 };
 
 /*
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index b2dc48be..10f227d1 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1531,8 +1531,10 @@  read_control_auth(struct buffer *buf,
         openvpn_decrypt(buf, null, &ctx->opt, NULL, BPTR(buf));
         if (!buf->len)
         {
-            msg(D_TLS_ERRORS,
-                "TLS Error: incoming packet authentication failed from %s",
+            /* openvpn3 doesn't change packet-id on retransmit, this is
+             * likely the case so tune verbosity down */
+            int verb = ctx->opt.packet_id.rec.retransmit ? D_TLS_DEBUG : D_TLS_ERRORS;
+            msg(verb, "TLS Error: incoming packet authentication failed from %s",
                 print_link_socket_actual(from, &gc));
             goto cleanup;
         }