From 65fb67cd6c320a426567b2922c4282fb8738ba3f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Reynir=20Bj=C3=B6rnsson?= <reynir@reynir.dk>
Date: Thu, 16 May 2024 13:58:08 +0200
Subject: [PATCH] Only schedule_exit() once
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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>
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>
(cherry picked from commit 55bb3260c12bae33b6a8eac73cbb6972f8517411)
---
 src/openvpn/forward.c | 15 +++++++++++----
 src/openvpn/forward.h |  2 +-
 src/openvpn/push.c    |  4 +---
 3 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 042ba9e..26eca49 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -428,17 +428,24 @@ check_server_poll_timeout(struct context *c)
 }
 
 /*
- * 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 5585366..9dd9e47 100644
--- a/src/openvpn/forward.h
+++ b/src/openvpn/forward.h
@@ -328,7 +328,7 @@ send_control_channel_string_dowork(struct tls_multi *multi,
 void process_ip_header(struct context *c, unsigned int flags, struct buffer *buf);
 
 #if P2MP
-void schedule_exit(struct context *c, const int n_seconds, const int signal);
+bool schedule_exit(struct context *c);
 
 #endif
 
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index bc94c32..b8e7c93 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -266,8 +266,6 @@ send_auth_failed(struct context *c, const char *client_reason)
     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)
     {
@@ -317,7 +315,7 @@ send_auth_pending_messages(struct context *c, const char *extra)
 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);
 }
 
-- 
2.33.0

