diff mbox series

[Openvpn-devel,v3] Only schedule_exit() once

Message ID 20240516120434.23499-1-gert@greenie.muc.de
State Accepted
Headers show
Series [Openvpn-devel,v3] Only schedule_exit() once | expand

Commit Message

Gert Doering May 16, 2024, 11:58 a.m. UTC 
From: Reynir Björnsson <reynir@reynir.dk>

If an exit has already been scheduled we should not schedule it again.
Otherwise, the exit signal is never emitted if the peer reschedules the
exit before the timeout occurs.

schedule_exit() now only takes the context as argument. The signal is
hard coded to SIGTERM, and the interval is read directly from the
context options.

Furthermore, schedule_exit() now returns a bool signifying whether an
exit was scheduled; false if exit is already scheduled. The call sites
are updated accordingly. A notable difference is that management is only
notified *once* when an exit is scheduled - we no longer notify
management on redundant exit.

This patch was assigned a CVE number after already reviewed and ACKed,
because it was discovered that a misbehaving client can use the (now
fixed) server behaviour to avoid being disconnected by means of a
managment interface "client-kill" command - the security issue here is
"client can circumvent security policy set by management interface".

This only affects previously authenticated clients, and only management
client-kill, so normal renegotion / AUTH_FAIL ("your session ends") is not
affected.

CVE: 2024-28882

Change-Id: I9457f005f4ba970502e6b667d9dc4299a588d661
Signed-off-by: Reynir Björnsson <reynir@reynir.dk>
Acked-by: Arne Schwabe <arne-openvpn@rfc2549.org>
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/555
This mail reflects revision 3 of this Change.

Acked-by according to Gerrit (reflected above):
Arne Schwabe <arne-openvpn@rfc2549.org>

Comments

Gert Doering May 17, 2024, 6:44 a.m. UTC | #1 
Thanks :-) - this is mostly the same as v2, with a whitespace fix,
and remodeling receive_exit_message() to avoid -Werror failures 
when comping without ENABLE_MANAGEMENT ("unused variable").

Tested on GHA, buildbots and the server testbed.

Your patch has been applied to the master and release/2.6 branch.

The CC EEN functionality is not part of 2.5, so the "clients can use
this to get not dropped off" attack does not exist - and the patch
is not applicable.

commit 55bb3260c12bae33b6a8eac73cbb6972f8517411 (master)
commit 65fb67cd6c320a426567b2922c4282fb8738ba3f (release/2.6)
Author: Reynir Björnsson
Date:   Thu May 16 13:58:08 2024 +0200

     Only schedule_exit() once

     Signed-off-by: Reynir Björnsson <reynir@reynir.dk>
     Acked-by: Arne Schwabe <arne-openvpn@rfc2549.org>
     Message-Id: <20240516120434.23499-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg28679.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 8d10f25..01165b2 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -514,17 +514,24 @@ 
 }
 
 /*
- * Schedule a signal n_seconds from now.
+ * Schedule a SIGTERM signal c->options.scheduled_exit_interval seconds from now.
  */
-void
-schedule_exit(struct context *c, const int n_seconds, const int signal)
+bool
+schedule_exit(struct context *c)
 {
+    const int n_seconds = c->options.scheduled_exit_interval;
+    /* don't reschedule if already scheduled. */
+    if (event_timeout_defined(&c->c2.scheduled_exit))
+    {
+        return false;
+    }
     tls_set_single_session(c->c2.tls_multi);
     update_time();
     reset_coarse_timers(c);
     event_timeout_init(&c->c2.scheduled_exit, n_seconds, now);
-    c->c2.scheduled_exit_signal = signal;
+    c->c2.scheduled_exit_signal = SIGTERM;
     msg(D_SCHED_EXIT, "Delayed exit in %d seconds", n_seconds);
+    return true;
 }
 
 /*
diff --git a/src/openvpn/forward.h b/src/openvpn/forward.h
index 6fb5a18..422c591 100644
--- a/src/openvpn/forward.h
+++ b/src/openvpn/forward.h
@@ -303,7 +303,7 @@ 
 
 void process_ip_header(struct context *c, unsigned int flags, struct buffer *buf);
 
-void schedule_exit(struct context *c, const int n_seconds, const int signal);
+bool schedule_exit(struct context *c);
 
 static inline struct link_socket_info *
 get_link_socket_info(struct context *c)
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 1b406b9..17e7e80 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -204,7 +204,11 @@ 
      * */
     if (c->options.mode == MODE_SERVER)
     {
-        schedule_exit(c, c->options.scheduled_exit_interval, SIGTERM);
+        if (!schedule_exit(c))
+	{
+		/* Return early when we don't need to notify management */
+		return;
+	}
     }
     else
     {
@@ -391,7 +395,7 @@ 
 void
 send_auth_failed(struct context *c, const char *client_reason)
 {
-    if (event_timeout_defined(&c->c2.scheduled_exit))
+    if (!schedule_exit(c))
     {
         msg(D_TLS_DEBUG, "exit already scheduled for context");
         return;
@@ -401,8 +405,6 @@ 
     static const char auth_failed[] = "AUTH_FAILED";
     size_t len;
 
-    schedule_exit(c, c->options.scheduled_exit_interval, SIGTERM);
-
     len = (client_reason ? strlen(client_reason)+1 : 0) + sizeof(auth_failed);
     if (len > PUSH_BUNDLE_SIZE)
     {
@@ -492,7 +494,7 @@ 
 void
 send_restart(struct context *c, const char *kill_msg)
 {
-    schedule_exit(c, c->options.scheduled_exit_interval, SIGTERM);
+    schedule_exit(c);
     send_control_channel_string(c, kill_msg ? kill_msg : "RESTART", D_PUSH);
 }