Fix SOCKSv5 method selection

PATCH In review Gert Doering 2 participants 6 messages Apr 13, 2014

lore

Share & tools

lore

Gert Doering

author

2014-04-13 11:43 UTC #0 lore raw

PATCH 1/1 Fix SOCKSv5 method selection

From: Yawning Angel <[email protected]...>

So, RFC 1928 doesn't say anything about the METHODS field in the Method Selection message being ordered in terms of preference or anything, and the server is free to pick any of the METHODS offered by the client.

Always sending a Method Selection message with NO AUTHENTICATION REQUIRED and USERNAME/PASSWORD set is broken on two fronts:

* If the OpenVPN client can't handle the server picking USERNAME/PASSWORD due to the credentials being missing, it shouldn't offer it to the server.

* If the OpenVPN client has credentials, then it should always attempt to authenticate. This is a security product. "You can misconfigure it and it will work" is not acceptable. Setting a username/password when the SOCKS server doesn't require/support that as an option is the user not configuring it correctly, and should be treated as such.

Also verify that the SOCKS server returned the auth that was requested.

---
 src/openvpn/socks.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

src/openvpn/socks.c +13 -3

index 1551da8..5cd27ac 100644
@@ -190,9 +190,12 @@ socks_handshake (struct socks_proxy_info *p,
   int len = 0;
   const int timeout_sec = 5;
 
-  /* VER = 5, NMETHODS = 2, METHODS = [0 (no auth), 2 (plain login)] */
-  const ssize_t size = send (sd, "\x05\x02\x00\x02", 4, MSG_NOSIGNAL);
-  if (size != 4)
+  /* VER = 5, NMETHODS = 1, METHODS = [0 (no auth)] */
+  char method_sel[3] = { 0x05, 0x01, 0x00 };
+  if (p->authfile[0])
+      method_sel[2] = 0x02; /* METHODS = [2 (plain login)] */
+  const ssize_t size = send (sd, method_sel, sizeof (method_sel), MSG_NOSIGNAL);
+  if (size != sizeof (method_sel))
     {
       msg (D_LINK_ERRORS | M_ERRNO, "socks_handshake: TCP port write failed on send()");
       return false;

@@ -252,6 +255,13 @@ socks_handshake (struct socks_proxy_info *p,
       return false;
     }
 
+  /* validate that the auth method returned is the one sent */
+  if (buf[1] != method_sel[2])
+    {
+      msg (D_LINK_ERRORS, "socks_handshake: Socks proxy returned unexpected auth");
+      return false;
+    }
+
   /* select the appropriate authentication method */
   switch (buf[1])
     {

Signature · 1 line

1.8.3.2

Gert Doering

author

2014-04-13 11:52 UTC #1 lore raw

Hi,

this came in as github pull req #14, and fixes the socks problem that people are complaining about in about every list, except here and in trac... - there *is* also trac#377.

The patch "as is" breaks windows compilation as...

src/openvpn/socks.c socks_handshake (struct socks_proxy_info *p, · line 197

Quoted 3 lines

src/openvpn/socks.c +3 -0

+  if (p->authfile[0])
+      method_sel[2] = 0x02; /* METHODS = [2 (plain login)] */
+  const ssize_t size = send (sd, method_sel, sizeof (method_sel), MSG_NOSIGNAL);

... doing the variable declaration of "size" after some other code has been executed is not permitted by MSVC. I'll amend that when committing.

To me, the patch is good, even though it might break user-visible behaviour (users specifying proxy auth credentials to a proxy that is not using them, this will now result in an error) - the reasoning behind it is valid.

More opinions are welcome, though...?

gert

Quoted 72 lines · Gert Doering

From: Yawning Angel <[email protected]...>
So, RFC 1928 doesn't say anything about the METHODS field in the Method Selection message being ordered in terms of preference or anything, and the server is free to pick any of the METHODS offered by the client.
Always sending a Method Selection message with NO AUTHENTICATION REQUIRED and USERNAME/PASSWORD set is broken on two fronts:
* If the OpenVPN client can't handle the server picking USERNAME/PASSWORD due to the credentials being missing, it shouldn't offer it to the server.
* If the OpenVPN client has credentials, then it should always attempt to authenticate. This is a security product. "You can misconfigure it and it will work" is not acceptable. Setting a username/password when the SOCKS server doesn't require/support that as an option is the user not configuring it correctly, and should be treated as such.
Also verify that the SOCKS server returned the auth that was requested.
---
 src/openvpn/socks.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)
src/openvpn/socks.c +13 -3
index 1551da8..5cd27ac 100644
@@ -190,9 +190,12 @@ socks_handshake (struct socks_proxy_info *p,
   int len = 0;
   const int timeout_sec = 5;
 
-  /* VER = 5, NMETHODS = 2, METHODS = [0 (no auth), 2 (plain login)] */
-  const ssize_t size = send (sd, "\x05\x02\x00\x02", 4, MSG_NOSIGNAL);
-  if (size != 4)
+  /* VER = 5, NMETHODS = 1, METHODS = [0 (no auth)] */
+  char method_sel[3] = { 0x05, 0x01, 0x00 };
+  if (p->authfile[0])
+      method_sel[2] = 0x02; /* METHODS = [2 (plain login)] */
+  const ssize_t size = send (sd, method_sel, sizeof (method_sel), MSG_NOSIGNAL);
+  if (size != sizeof (method_sel))
     {
       msg (D_LINK_ERRORS | M_ERRNO, "socks_handshake: TCP port write failed on send()");
       return false;
@@ -252,6 +255,13 @@ socks_handshake (struct socks_proxy_info *p,
       return false;
     }
 
+  /* validate that the auth method returned is the one sent */
+  if (buf[1] != method_sel[2])
+    {
+      msg (D_LINK_ERRORS, "socks_handshake: Socks proxy returned unexpected auth");
+      return false;
+    }
+
   /* select the appropriate authentication method */
   switch (buf[1])
     {
Signature · 13 lines
1.8.3.2


------------------------------------------------------------------------------
Put Bad Developers to Shame
Dominate Development with Jenkins Continuous Integration
Continuously Automate Build, Test & Deployment 
Start a new project now. Try Jenkins in the cloud.
http://p.sf.net/sfu/13600_Cloudbees
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Gert Doering

author

2014-04-13 13:01 UTC #2 lore raw

Hi,

Quoted 1 line · Gert Doering

I'll amend that when committing.

Here is the patch with the MSVC build change, plus some changes to the commit message to point to the pull request and trac.

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

From 19b662a2013769f62bcd4e8776a63641e3fb991d Mon Sep 17 00:00:00 2001
From: Yawning Angel <[email protected]...>
Date: Mon, 10 Mar 2014 03:47:58 +0000
Subject: [PATCH] Fix SOCKSv5 method selection

Always sending a Method Selection message with NO AUTHENTICATION REQUIRED and USERNAME/PASSWORD set is broken on two fronts:

* If the OpenVPN client can't handle the server picking USERNAME/PASSWORD due to the credentials being missing, it shouldn't offer it to the server.

Also verify that the SOCKS server returned the auth that was requested.

URL: https://github.com/OpenVPN/openvpn/pull/14
Fix trac #377

---
 src/openvpn/socks.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

src/openvpn/socks.c +15 -3

index 1551da8..ee746a3 100644
@@ -189,10 +189,15 @@ socks_handshake (struct socks_proxy_info *p,
   char buf[2];
   int len = 0;
   const int timeout_sec = 5;
+  const ssize_t size;
 
-  /* VER = 5, NMETHODS = 2, METHODS = [0 (no auth), 2 (plain login)] */
-  const ssize_t size = send (sd, "\x05\x02\x00\x02", 4, MSG_NOSIGNAL);
-  if (size != 4)
+  /* VER = 5, NMETHODS = 1, METHODS = [0 (no auth)] */
+  char method_sel[3] = { 0x05, 0x01, 0x00 };
+  if (p->authfile[0])
+      method_sel[2] = 0x02; /* METHODS = [2 (plain login)] */
+
+  size = send (sd, method_sel, sizeof (method_sel), MSG_NOSIGNAL);
+  if (size != sizeof (method_sel))
     {
       msg (D_LINK_ERRORS | M_ERRNO, "socks_handshake: TCP port write failed on send()");
       return false;

@@ -252,6 +257,13 @@ socks_handshake (struct socks_proxy_info *p,
       return false;
     }
 
+  /* validate that the auth method returned is the one sent */
+  if (buf[1] != method_sel[2])
+    {
+      msg (D_LINK_ERRORS, "socks_handshake: Socks proxy returned unexpected auth");
+      return false;
+    }
+
   /* select the appropriate authentication method */
   switch (buf[1])
     {

Signature · 1 line

1.8.3.2

Arne Schwabe

2014-04-13 13:50 UTC #3 lore raw

Am 13.04.14 15:01, schrieb Gert Doering:

Quoted 8 lines

Hi,
Quoted 1 line · Gert Doering
I'll amend that when committing.
Here is the patch with the MSVC build change, plus some changes to the commit message to point to the pull request and trac.

ACK from me for the patch. It looks reasonable and if the change breaks configs than these configs were fishy to begin with.

Please compile test the const ssize_t size; size = ... split. I think you need to drop the const keyword.

Arne

Gert Doering

author

2014-04-13 19:17 UTC #4 lore raw

Hi,

Quoted 11 lines · Arne Schwabe

Am 13.04.14 15:01, schrieb Gert Doering:
Quoted 5 lines · Gert Doering
Quoted 1 line
I'll amend that when committing.
Here is the patch with the MSVC build change, plus some changes to the commit message to point to the pull request and trac.
ACK from me for the patch. It looks reasonable and if the change breaks configs than these configs were fishy to begin with.
Please compile test the const ssize_t size; size = ... split. I think you need to drop the const keyword.

Thanks. Right you are, if the line is split, the "const" will give a compile error at assignment time. Learned something new today...

Build and t_client tested, including the tcp- and udp-socks-tests (but only "with no authentication", as my test bed doesn't have a socks proxy with auth).

### test run 1d: 'tcp4 / socks proxy / p2pm / top net30' ### ### test run 2d: 'UDP4 / socks proxy [on TCP!] / p2pm / top net30' ###

gert

Signature · 4 lines

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Gert Doering

author

2014-04-13 19:23 UTC #5 lore raw

In reply to Gert Doering

Patch has been applied to the master and release/2.3 branches (with some small amendments to fix MSVC compilation and "const" rules as pointed out by Arne Schwabe).

commit 2903eba5dfe35c981329a833845e24de3882161a (master) commit 34df13fdb65242b81c9006ee8ac83be4cc3f9e09 (release/2.3)

Author: Yawning Angel
Date:   Mon Mar 10 03:47:58 2014 +0000

     Fix SOCKSv5 method selection

     URL: https://github.com/OpenVPN/openvpn/pull/14
     Acked-by: Gert Doering <[email protected]...>
     Acked-by: Arne Schwabe <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/8488
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering