diff mbox series

[Openvpn-devel] Wipe Socks5 credentials after use

Message ID AM0PR04MB53311DBD4346F75A380F7198B1689@AM0PR04MB5331.eurprd04.prod.outlook.com
State Superseded
Headers show
Series [Openvpn-devel] Wipe Socks5 credentials after use | expand

Commit Message

walter.openvpn--- via Openvpn-devel March 19, 2021, 5:45 a.m. UTC 
Socks5 plaintext authentication is not exactly high security, but we
might as well memzero the credentials before leaving the function.

---
 src/openvpn/socks.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

Comments

Gert Doering March 19, 2021, 6:30 a.m. UTC | #1 
Hi,

On Fri, Mar 19, 2021 at 04:45:18PM +0000, Maximilian Fillinger via Openvpn-devel wrote:
[..]

The patch itself is OK (I think), but actually applying it will mess up the
Author: information in git, because you are sending from a domain that
has DMARC p=reject.  So mailman is massacring your From: line into this

From: Maximilian Fillinger via Openvpn-devel <openvpn-devel@lists.sourceforge.net>                                   

... and this is what will be the Author of the commit.

Can you send this from an account that is not set up to intentionally
harm mailing lists?

gert
walter.openvpn--- via Openvpn-devel March 19, 2021, 6:57 a.m. UTC | #2 
Sorry about that! I'll send it again from my personal account later.

-----Original Message-----
From: Gert Doering [mailto:gert@greenie.muc.de] 
Sent: vrijdag 19 maart 2021 18:30
To: Maximilian Fillinger <maximilian.fillinger@fox-it.com>
Cc: openvpn-devel@lists.sourceforge.net
Subject: Re: [Openvpn-devel] [Patch] Wipe Socks5 credentials after use

Hi,

On Fri, Mar 19, 2021 at 04:45:18PM +0000, Maximilian Fillinger via Openvpn-devel wrote:
[..]

The patch itself is OK (I think), but actually applying it will mess up the
Author: information in git, because you are sending from a domain that has DMARC p=reject.  So mailman is massacring your From: line into this

From: Maximilian Fillinger via Openvpn-devel <openvpn-devel@lists.sourceforge.net>                                   

... and this is what will be the Author of the commit.

Can you send this from an account that is not set up to intentionally harm mailing lists?

gert

--
"If was one thing all people took for granted, was conviction that if you  feed honest figures into a computer, honest figures come out. Never doubted  it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             gert@greenie.muc.de
diff mbox series

Patch

diff --git a/src/openvpn/socks.c b/src/openvpn/socks.c
index 36df7470..add7a6d4 100644
--- a/src/openvpn/socks.c
+++ b/src/openvpn/socks.c
@@ -104,12 +104,13 @@  socks_username_password_auth(struct socks_proxy_info *p,
     const int timeout_sec = 5;
     struct user_pass creds;
     ssize_t size;
+    bool ret = false;
 
     creds.defined = 0;
     if (!get_user_pass(&creds, p->authfile, UP_TYPE_SOCKS, GET_USER_PASS_MANAGEMENT))
     {
         msg(M_NONFATAL, "SOCKS failed to get username/password.");
-        return false;
+        goto cleanup;
     }
 
     if ( (strlen(creds.username) > 255) || (strlen(creds.password) > 255) )
@@ -117,7 +118,7 @@  socks_username_password_auth(struct socks_proxy_info *p,
         msg(M_NONFATAL,
             "SOCKS username and/or password exceeds 255 characters.  "
             "Authentication not possible.");
-        return false;
+        goto cleanup;
     }
     openvpn_snprintf(to_send, sizeof(to_send), "\x01%c%s%c%s", (int) strlen(creds.username),
                      creds.username, (int) strlen(creds.password), creds.password);
@@ -126,7 +127,7 @@  socks_username_password_auth(struct socks_proxy_info *p,
     if (size != strlen(to_send))
     {
         msg(D_LINK_ERRORS | M_ERRNO, "socks_username_password_auth: TCP port write failed on send()");
-        return false;
+        goto cleanup;
     }
 
     while (len < 2)
@@ -147,21 +148,21 @@  socks_username_password_auth(struct socks_proxy_info *p,
         get_signal(signal_received);
         if (*signal_received)
         {
-            return false;
+            goto cleanup;
         }
 
         /* timeout? */
         if (status == 0)
         {
             msg(D_LINK_ERRORS | M_ERRNO, "socks_username_password_auth: TCP port read timeout expired");
-            return false;
+            goto cleanup;
         }
 
         /* error */
         if (status < 0)
         {
             msg(D_LINK_ERRORS | M_ERRNO, "socks_username_password_auth: TCP port read failed on select()");
-            return false;
+            goto cleanup;
         }
 
         /* read single char */
@@ -171,7 +172,7 @@  socks_username_password_auth(struct socks_proxy_info *p,
         if (size != 1)
         {
             msg(D_LINK_ERRORS | M_ERRNO, "socks_username_password_auth: TCP port read failed on recv()");
-            return false;
+            goto cleanup;
         }
 
         /* store char in buffer */
@@ -182,10 +183,14 @@  socks_username_password_auth(struct socks_proxy_info *p,
     if (buf[0] != 5 && buf[1] != 0)
     {
         msg(D_LINK_ERRORS, "socks_username_password_auth: server refused the authentication");
-        return false;
+        goto cleanup;
     }
 
-    return true;
+    ret = true;
+cleanup:
+    secure_memzero(&creds, sizeof(creds));
+    secure_memzero(to_send, sizeof(to_send));
+    return ret;
 }
 
 static bool