diff mbox series

[Openvpn-devel,v3] dco: make key state desync recoverable

Message ID 20260721203807.5813-1-gert@greenie.muc.de
State New
Headers
Series [Openvpn-devel,v3] dco: make key state desync recoverable |

Commit Message

Gert Doering July 21, 2026, 8:38 p.m. UTC 
  From: Ralf Lici <ralf@mandelbit.com>

If installing a DCO key succeeds, always record the slot in the
corresponding key_state. The installed-key counter only tracks whether
the number of kernel slots grew, but a successful KEY_NEW still means
that this key_state is associated with the selected DCO slot.

Also replace DCO key-state assertions in dco_update_keys() with error
returns. This preserves the invariant checks but lets the existing
caller restart the connection instead of aborting the process if
userspace ever detects inconsistent DCO key state.

Github: https://github.com/OpenVPN/openvpn-private-issues/issues/143
Change-Id: I4d0887839b4a13a92e92250d1b315dec949698f3
Signed-off-by: Ralf Lici <ralf@mandelbit.com>
Acked-by: Antonio Quartulli <antonio@mandelbit.com>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1795
---

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/+/1795
This mail reflects revision 3 of this Change.

Acked-by according to Gerrit (reflected above):
Antonio Quartulli <antonio@mandelbit.com>

Comments

Gert Doering July 24, 2026, 11:43 a.m. UTC | #1 
This was initially reported as a security vulnerability, providing a
way to crash a running OpenVPN server process.  Detailed testing turned
out that this does not work, but the state machine and the ASSERT()s
are still a bit too fragile, so this is a welcome improvement.

With the new code, if we really manage to de-sync userland and kernel,
it will now only kill the client instance (on a p2mp server) or restart
the connection, not exit() the process.  Better.

Lightly tested on the DCO+release/2.7 and 2.6 t_server tests
(ovpn_dco_v2.ko / ovpn.ko).

Your patch has been applied to the master, release/2.7 and
release/2.6 branch (bugfix).

commit ea3bb67e2b1e130c90cd2b28ec5e31a455d60164 (master)
commit d0528fae559c44c1d7815459f1c96e32ac96a8f3 (release/2.7)
commit 5b17c050dc3020186d71bbbd422d7e8525f1d6b6 (release/2.6)
Author: Ralf Lici
Date:   Tue Jul 21 22:38:01 2026 +0200

     dco: make key state desync recoverable

     Signed-off-by: Ralf Lici <ralf@mandelbit.com>
     Acked-by: Antonio Quartulli <antonio@mandelbit.com>
     Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1795
     Message-Id: <20260721203807.5813-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg37732.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c
index e944547..2584368 100644
--- a/src/openvpn/dco.c
+++ b/src/openvpn/dco.c
@@ -73,9 +73,12 @@ 
 
     int ret = dco_new_key(multi->dco, multi->dco_peer_id, ks->key_id, slot, encrypt_key, encrypt_iv,
                           decrypt_key, decrypt_iv, ciphername, epoch);
-    if ((ret == 0) && (multi->dco_keys_installed < 2))
+    if (ret == 0)
     {
-        multi->dco_keys_installed++;
+        if (multi->dco_keys_installed < 2)
+        {
+            multi->dco_keys_installed++;
+        }
         ks->dco_status =
             (slot == OVPN_KEY_SLOT_PRIMARY) ? DCO_INSTALLED_PRIMARY : DCO_INSTALLED_SECONDARY;
     }
@@ -166,7 +169,13 @@ 
     /* if we have a primary key, it must have been installed already (keys
      * are installed upon generation in the TLS code)
      */
-    ASSERT(primary->dco_status != DCO_NOT_INSTALLED);
+    if (primary->dco_status == DCO_NOT_INSTALLED)
+    {
+        msg(D_DCO, "DCO key state mismatch: selected primary key is not installed "
+                   "(peer_id=%d, key_id=%d, dco_keys_installed=%d)",
+            multi->dco_peer_id, primary->key_id, multi->dco_keys_installed);
+        return false;
+    }
 
     struct key_state *secondary = dco_get_secondary_key(multi, primary);
     /* if the current primary key was installed as secondary in DCO,
@@ -190,6 +199,14 @@ 
                 primary->key_id);
         }
 
+        if (secondary && secondary->dco_status != DCO_INSTALLED_PRIMARY)
+        {
+            msg(D_DCO, "DCO key state mismatch: expected old primary key is not installed as DCO primary before swap "
+                       "(peer_id=%d, new_primary_key_id=%d, old_primary_key_id=%d, old_primary_dco_status=%d, dco_keys_installed=%d)",
+                multi->dco_peer_id, primary->key_id, secondary->key_id, secondary->dco_status, multi->dco_keys_installed);
+            return false;
+        }
+
         int ret = dco_swap_keys(dco, multi->dco_peer_id);
         if (ret < 0)
         {
@@ -200,7 +217,6 @@ 
         primary->dco_status = DCO_INSTALLED_PRIMARY;
         if (secondary)
         {
-            ASSERT(secondary->dco_status == DCO_INSTALLED_PRIMARY);
             secondary->dco_status = DCO_INSTALLED_SECONDARY;
         }
     }