[Openvpn-devel,v2,2/2] Fix CR_RESPONSE mangaement message using wrong key_id

Message ID 20230522101138.2842378-2-arne@rfc2549.org
State Accepted
Headers show
Series [Openvpn-devel,v2,1/2] Introduce get_key_by_management_key_id helper function | expand

Commit Message

Arne Schwabe May 22, 2023, 10:11 a.m. UTC
the management interface expects the management key id instead
of the openvpn key id. In the past they often were the same for low ids
which hid the bug quite well.

Also do not pick uninitialised keystates (management key_id is not valid
in these).

Patch v2: do not add logging

Change-Id: If9fa1165a0e886b570b3738546ed810a32367cbe
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/push.c       | 4 ++--
 src/openvpn/ssl_common.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

Comments

Frank Lichtenheld July 4, 2023, 2 p.m. UTC | #1
On Mon, May 22, 2023 at 12:11:38PM +0200, Arne Schwabe wrote:
> the management interface expects the management key id instead
> of the openvpn key id. In the past they often were the same for low ids
> which hid the bug quite well.
> 
> Also do not pick uninitialised keystates (management key_id is not valid
> in these).
> 
> Patch v2: do not add logging

Similar patch was sent in via https://github.com/OpenVPN/openvpn/pull/359,
so kinda sorta acked?

> Change-Id: If9fa1165a0e886b570b3738546ed810a32367cbe
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>  src/openvpn/push.c       | 4 ++--
>  src/openvpn/ssl_common.h | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/openvpn/push.c b/src/openvpn/push.c
> index 8e9627199..8f0a534ac 100644
> --- a/src/openvpn/push.c
> +++ b/src/openvpn/push.c
> @@ -267,9 +267,9 @@ receive_cr_response(struct context *c, const struct buffer *buffer)
>      struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
>      struct man_def_auth_context *mda = session->opt->mda_context;
>      struct env_set *es = session->opt->es;
> -    int key_id = get_primary_key(c->c2.tls_multi)->key_id;
> +    unsigned int mda_key_id = get_primary_key(c->c2.tls_multi)->mda_key_id;
>  
> -    management_notify_client_cr_response(key_id, mda, es, m);
> +    management_notify_client_cr_response(mda_key_id, mda, es, m);
>  #endif
>  #if ENABLE_PLUGIN
>      verify_crresponse_plugin(c->c2.tls_multi, m);
> diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
> index ebfd25432..be0f18746 100644
> --- a/src/openvpn/ssl_common.h
> +++ b/src/openvpn/ssl_common.h
> @@ -733,7 +733,7 @@ get_key_by_management_key_id(struct tls_multi *multi, unsigned int mda_key_id)
>      for (int i = 0; i < KEY_SCAN_SIZE; ++i)
>      {
>          struct key_state *ks = get_key_scan(multi, i);
> -        if (ks->mda_key_id == mda_key_id)
> +        if (ks->mda_key_id == mda_key_id && ks->state > S_UNDEF)
>          {
>              return ks;
>          }

Regards,
Selva Nair July 4, 2023, 3:15 p.m. UTC | #2
It seems I missed to send this ACK to the list.. Here it is.

On Mon, May 22, 2023 at 6:12 AM Arne Schwabe <arne@rfc2549.org> wrote:

> the management interface expects the management key id instead
> of the openvpn key id. In the past they often were the same for low ids
> which hid the bug quite well.
>
> Also do not pick uninitialised keystates (management key_id is not valid
> in these).
>
> Patch v2: do not add logging
>
> Change-Id: If9fa1165a0e886b570b3738546ed810a32367cbe
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>  src/openvpn/push.c       | 4 ++--
>  src/openvpn/ssl_common.h | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/openvpn/push.c b/src/openvpn/push.c
> index 8e9627199..8f0a534ac 100644
> --- a/src/openvpn/push.c
> +++ b/src/openvpn/push.c
> @@ -267,9 +267,9 @@ receive_cr_response(struct context *c, const struct
> buffer *buffer)
>      struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
>      struct man_def_auth_context *mda = session->opt->mda_context;
>      struct env_set *es = session->opt->es;
> -    int key_id = get_primary_key(c->c2.tls_multi)->key_id;
> +    unsigned int mda_key_id =
> get_primary_key(c->c2.tls_multi)->mda_key_id;
>
> -    management_notify_client_cr_response(key_id, mda, es, m);
> +    management_notify_client_cr_response(mda_key_id, mda, es, m);
>  #endif
>  #if ENABLE_PLUGIN
>      verify_crresponse_plugin(c->c2.tls_multi, m);
> diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
> index ebfd25432..be0f18746 100644
> --- a/src/openvpn/ssl_common.h
> +++ b/src/openvpn/ssl_common.h
> @@ -733,7 +733,7 @@ get_key_by_management_key_id(struct tls_multi *multi,
> unsigned int mda_key_id)
>      for (int i = 0; i < KEY_SCAN_SIZE; ++i)
>      {
>          struct key_state *ks = get_key_scan(multi, i);
> -        if (ks->mda_key_id == mda_key_id)
> +        if (ks->mda_key_id == mda_key_id && ks->state > S_UNDEF)
>          {
>              return ks;
>          }
> --
> 2.39.2 (Apple Git-143)
>
>

Acked-by Selva Nair <selva.nair@gmail.com>
Gert Doering July 7, 2023, 4:28 p.m. UTC | #3
Thanks for reminding me that there was something pending here as well...

This is a bit complicated, though.  The bugfix needs to go to 2.6 as
well, so "2/2 applied", but the v2 as sent to the list has a hunk that
is really a bugfix for get_key_by_management_key_id(), which is in
1/2, and that is refactoring -> master.

So I've only applied the first hunk here, and the second hunk should
go together with all of 1/2 into a "master only" patch.  Right?

I have run this through the basic client test, which isn't exactly
excercising the management interface (not at all) but I have nothing
for that - so I'm relying on Selva's ACK and Jemmy Wang's test
results in GH #359 on "does it actually fix the bug".

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

commit 223baa9c9b818e4c542a9037f190f53ce6f7af5c (master)
commit 0caf0389454c75ae9dfeda096a820446d2d1a926 (HEAD -> release/2.6)
Author: Arne Schwabe
Date:   Mon May 22 12:11:38 2023 +0200

     Fix CR_RESPONSE mangaement message using wrong key_id

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Selva Nair <selva.nair@gmail.com>
     Message-Id: <20230522101138.2842378-2-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26719.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 8e9627199..8f0a534ac 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -267,9 +267,9 @@  receive_cr_response(struct context *c, const struct buffer *buffer)
     struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
     struct man_def_auth_context *mda = session->opt->mda_context;
     struct env_set *es = session->opt->es;
-    int key_id = get_primary_key(c->c2.tls_multi)->key_id;
+    unsigned int mda_key_id = get_primary_key(c->c2.tls_multi)->mda_key_id;
 
-    management_notify_client_cr_response(key_id, mda, es, m);
+    management_notify_client_cr_response(mda_key_id, mda, es, m);
 #endif
 #if ENABLE_PLUGIN
     verify_crresponse_plugin(c->c2.tls_multi, m);
diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index ebfd25432..be0f18746 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -733,7 +733,7 @@  get_key_by_management_key_id(struct tls_multi *multi, unsigned int mda_key_id)
     for (int i = 0; i < KEY_SCAN_SIZE; ++i)
     {
         struct key_state *ks = get_key_scan(multi, i);
-        if (ks->mda_key_id == mda_key_id)
+        if (ks->mda_key_id == mda_key_id && ks->state > S_UNDEF)
         {
             return ks;
         }