diff mbox series

[Openvpn-devel] Do not copy auth_token username to itself

Message ID 20221027160619.11894-1-selva.nair@gmail.com
State Accepted
Headers show
Series [Openvpn-devel] Do not copy auth_token username to itself | expand

Commit Message

Selva Nair Oct. 27, 2022, 5:06 a.m. UTC 
From: Selva Nair <selva.nair@gmail.com>

- Fixes a potential mis-behaviour (strncpy with
dest == src) introduced by commits ecad4839c (2.6)
and 3d792ae955 (2.5).
Reported by: Gert Doering <gert@greenie.muc.de>

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 src/openvpn/ssl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Gert Doering Oct. 27, 2022, 8:02 a.m. UTC | #1 
Acked-by: Gert Doering <gert@greenie.muc.de>

Testing this was fairly easy - I added a msg() call in that if()
branch, connected to a server with auth-token and verified that it did
the strncpynt() only on the "I have a real username here" call.  And
that token reauth still works ("username properly copied"), which
it all did.

Your patch has been applied to the master branch, for now - it needs
to go to 2.5 as well, but we're right in the middle of a 2.5.8 
release, and I need to check with Samuli & Frank if anything has been
done already that would cause havoc if I change the (preliminary)
v2.5.8 tag...

commit dbf142ffe597b21aa09a47677ea2061b74a9354e
Author: Selva Nair
Date:   Thu Oct 27 12:06:19 2022 -0400

     Do not copy auth_token username to itself

     Signed-off-by: Selva Nair <selva.nair@gmail.com>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20221027160619.11894-1-selva.nair@gmail.com>
     URL: https://www.mail-archive.com/search?l=mid&q=20221027160619.11894-1-selva.nair@gmail.com
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Gert Doering Oct. 27, 2022, 7:46 p.m. UTC | #2 
Hi,

... and here we go:

On Thu, Oct 27, 2022 at 09:02:52PM +0200, Gert Doering wrote:
> Your patch has been applied to the master branch, for now - it needs
> to go to 2.5 as well, but we're right in the middle of a 2.5.8 
> release, and I need to check with Samuli & Frank if anything has been
> done already that would cause havoc if I change the (preliminary)
> v2.5.8 tag...
> 
> commit dbf142ffe597b21aa09a47677ea2061b74a9354e
  commit cd50cf021bcfb797b6dacbe853e4c08b21a8e89d (release/2.5)

briefly tested in a setup with username+password and token, things
still work :-) - v2.5.8 has been updated, so this is in.

gert
diff mbox series

Patch

diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 3106c738..24e8ba63 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -2180,7 +2180,7 @@  key_method_2_write(struct buffer *buf, struct tls_multi *multi, struct tls_sessi
             goto error;
         }
         /* save username for auth-token which may get pushed later */
-        if (session->opt->pull)
+        if (session->opt->pull && up != &auth_token)
         {
             strncpynt(auth_token.username, up->username, USER_PASS_LEN);
         }