diff mbox series

[Openvpn-devel,v1] proxy.c: Clear sensitive data after use

Message ID 20240905100724.4105-1-gert@greenie.muc.de
State Accepted
Headers show
Series [Openvpn-devel,v1] proxy.c: Clear sensitive data after use | expand

Commit Message

Gert Doering Sept. 5, 2024, 10:07 a.m. UTC 
From: Selva Nair <selva.nair@gmail.com>

Usage of credentials  is a bit odd in this file.
Actually the copy of "struct user_pass" kept in p->up is not
required at all. It just defeats the purpose of auth-nocahe
as it never gets cleared.

Removing it is beyond the scope of this patch -- we just ensure
it's purged after use.

Change-Id: Ic6d63a319d272a56ac0e278f1356bc5241b56a34
Signed-off-by: Selva Nair <selva.nair@gmail.com>
Acked-by: Frank Lichtenheld <frank@lichtenheld.com>
---

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

Signed-off-by line for the author was added as per our policy.

Acked-by according to Gerrit (reflected above):
Frank Lichtenheld <frank@lichtenheld.com>

Comments

Gert Doering Sept. 8, 2024, 1:05 p.m. UTC | #1 
Thanks for looking into this - and I agree with the conclusion on
keeping the scope of this patch to "purge this", not refactor the
overall code to get rid of that copy completely.
 
I have not tested this "for real" as I do not currently have a proxy
setup that requires authentication - just stared at the code, and run
the normal client->proxy tests (and nothing broke).

Your patch has been applied to the master and release/2.6 branch
(useful and fairly isolated change, adding a bit of hardening).

commit dbe7e456954bf001420c4552c2b6e184ec6e068c (master)
commit 534609a2a7f0dcd56c8eab764c9c9c99834dcc6f (release/2.6)
Author: Selva Nair
Date:   Thu Sep 5 12:07:24 2024 +0200

     proxy.c: Clear sensitive data after use

     Signed-off-by: Selva Nair <selva.nair@gmail.com>
     Acked-by: Frank Lichtenheld <frank@lichtenheld.com>
     Message-Id: <20240905100724.4105-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg29061.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/proxy.c b/src/openvpn/proxy.c
index 5de0da4..eddacc9 100644
--- a/src/openvpn/proxy.c
+++ b/src/openvpn/proxy.c
@@ -247,7 +247,9 @@ 
     struct buffer out = alloc_buf_gc(strlen(p->up.username) + strlen(p->up.password) + 2, gc);
     ASSERT(strlen(p->up.username) > 0);
     buf_printf(&out, "%s:%s", p->up.username, p->up.password);
-    return (const char *)make_base64_string((const uint8_t *)BSTR(&out), gc);
+    char *ret = (char *)make_base64_string((const uint8_t *)BSTR(&out), gc);
+    secure_memzero(BSTR(&out), out.len);
+    return ret;
 }
 
 static void
@@ -736,6 +738,9 @@ 
                 ASSERT(0);
         }
 
+        /* clear any sensitive content in buf */
+        secure_memzero(buf, sizeof(buf));
+
         /* send empty CR, LF */
         if (!send_crlf(sd))
         {
@@ -983,6 +988,8 @@ 
                 {
                     goto error;
                 }
+                /* clear any sensitive content in buf */
+                secure_memzero(buf, sizeof(buf));
 
                 /* receive reply from proxy */
                 if (!recv_line(sd, buf, sizeof(buf), get_server_poll_remaining_time(server_poll_timeout), true, NULL, signal_received))
@@ -1086,10 +1093,12 @@ 
 #endif
 
 done:
+    purge_user_pass(&p->up, true);
     gc_free(&gc);
     return ret;
 
 error:
+    purge_user_pass(&p->up, true);
     register_signal(sig_info, SIGUSR1, "HTTP proxy error"); /* SOFT-SIGUSR1 -- HTTP proxy error */
     gc_free(&gc);
     return ret;