| Message ID | 20221213225430.1892940-3-arne@rfc2549.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,1/3] Improve debug logging of DCO swap key message and Linux dco_new_peer | expand |
Hi, On 13/12/2022 23:54, Arne Schwabe wrote: > We have 6 key slots but normally only consider 3 of them to be > active/valid keys. Especially the secondary key of TM_LAME_DUCK can > in rare corner cases have a key that is still installed in the kernel. > > While this should not cause any issues since I do not see way for this > key to become active ever again, it is better to keep the state correctly. > > Signed-off-by: Arne Schwabe <arne@rfc2549.org> I am fine with this because it is just "safer". But I truly dislike that we have to "Defend ourselves" from conditions which we don't even know when they may happen.... Still, this is a rant for another patch/cleanup. This patch makes sense Acked-by: Antonio Quartulli <a@unstable.cc> > --- > src/openvpn/dco.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c > index 36bfbf10a..20196fe5d 100644 > --- a/src/openvpn/dco.c > +++ b/src/openvpn/dco.c > @@ -221,13 +221,17 @@ dco_update_keys(dco_context_t *dco, struct tls_multi *multi) > multi->dco_keys_installed = 1; > } > > - /* all keys that are not installed are set to NOT installed */ > - for (int i = 0; i < KEY_SCAN_SIZE; ++i) > + /* all keys that are not installed are set to NOT installed. Include also > + * keys that might even be considered as active keys to be sure*/ > + for (int i = 0; i < TM_SIZE; ++i) > { > - struct key_state *ks = get_key_scan(multi, i); > - if (ks != primary && ks != secondary) > + for (int j = 0; j < KS_SIZE; j++) > { > - ks->dco_status = DCO_NOT_INSTALLED; > + struct key_state *ks = &multi->session[i].key[j]; > + if (ks != primary && ks != secondary) > + { > + ks->dco_status = DCO_NOT_INSTALLED; > + } > } > } > return true;
I've done a quick read over ssl_common.h to be sure the indexes are
valid, and fed this to a light test run (client-side only, for the
start). Looks good, passes.
I can see why Antonio grumbles, but then, ensuring stuff is in a really
well-known state if we suspect it might not always be is a valid line
of defense.
Your patch has been applied to the master and release/2.6 branch.
commit 4cf7409e82580f2890c391372d60ed713ba4650c (master)
commit 5c26918f482f21484527f4b065818863c459b226 (release/2.6)
Author: Arne Schwabe
Date: Tue Dec 13 23:54:30 2022 +0100
Set DCO_NOT_INSTALLED also for keys not in the get_key_scan range
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Antonio Quartulli <antonio@openvpn.net>
Message-Id: <20221213225430.1892940-3-arne@rfc2549.org>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25681.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c index 36bfbf10a..20196fe5d 100644 --- a/src/openvpn/dco.c +++ b/src/openvpn/dco.c @@ -221,13 +221,17 @@ dco_update_keys(dco_context_t *dco, struct tls_multi *multi) multi->dco_keys_installed = 1; } - /* all keys that are not installed are set to NOT installed */ - for (int i = 0; i < KEY_SCAN_SIZE; ++i) + /* all keys that are not installed are set to NOT installed. Include also + * keys that might even be considered as active keys to be sure*/ + for (int i = 0; i < TM_SIZE; ++i) { - struct key_state *ks = get_key_scan(multi, i); - if (ks != primary && ks != secondary) + for (int j = 0; j < KS_SIZE; j++) { - ks->dco_status = DCO_NOT_INSTALLED; + struct key_state *ks = &multi->session[i].key[j]; + if (ks != primary && ks != secondary) + { + ks->dco_status = DCO_NOT_INSTALLED; + } } } return true;
We have 6 key slots but normally only consider 3 of them to be active/valid keys. Especially the secondary key of TM_LAME_DUCK can in rare corner cases have a key that is still installed in the kernel. While this should not cause any issues since I do not see way for this key to become active ever again, it is better to keep the state correctly. Signed-off-by: Arne Schwabe <arne@rfc2549.org> --- src/openvpn/dco.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)