| Message ID | 20230318141330.1315235-1-selva.nair@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel] Improve error message on short read from socks proxy | expand |
Acked-by: Gert Doering <gert@greenie.muc.de> Thanks, this change makes sense. I have not actively tried to provoke it (like, by connecting to a "fake SOCKS server" that will trigger it), but the change is obviously an improvement to "if it's not ==1, it must be a TCP error!"). I do test the SOCKS code in my t_client runs, but all my SOCKS servers are well-behaved (and the regular t_client cannot handle "expect failure" cases anyway). Room for improvement. So, I've tested this on a regular client using a well-behaved SOCKS proxy, and as expected, "it does not break anything" :-) Your patch has been applied to the master and release/2.6 branch. commit 172640189277c940439d24fd31a59b8faffd0b3e (master) commit c20a15844829a186b4d5256b0e8d76b8eb074845 (release/2.6) Author: Selva Nair Date: Sat Mar 18 10:13:30 2023 -0400 Improve error message on short read from socks proxy Signed-off-by: Selva Nair <selva.nair@gmail.com> Acked-by: Gert Doering <gert@greenie.muc.de> Message-Id: <20230318141330.1315235-1-selva.nair@gmail.com> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26437.html Signed-off-by: Gert Doering <gert@greenie.muc.de> -- kind regards, Gert Doering
> > > > Thanks, this change makes sense. I have not actively tried to provoke > it (like, by connecting to a "fake SOCKS server" that will trigger it), > but the change is obviously an improvement to "if it's not ==1, it > must be a TCP error!"). > An easy way to "provoke" this is to use openssh proxy (say, -D 1080) and use it to proxy to a udp server. SSH will close the connection as it does not support udp association. Probably it should return one of the socks5 error code instead, but doesn't. Even if it did, our recv_socks_reply() is not capable of handling such errors. Selva
Hi, On Mon, Mar 20, 2023 at 02:07:14PM -0400, Selva Nair wrote: > > Thanks, this change makes sense. I have not actively tried to provoke > > it (like, by connecting to a "fake SOCKS server" that will trigger it), > > but the change is obviously an improvement to "if it's not ==1, it > > must be a TCP error!"). > > An easy way to "provoke" this is to use openssh proxy (say, -D 1080) and > use it to proxy to a udp server. SSH will close the connection as it does > not > support udp association. Probably it should return one of the socks5 error > code instead, but doesn't. Even if it did, our recv_socks_reply() is not > capable of > handling such errors. Ah, this is helpful. I did use "socks -D" for the very first t_client.sh "test with SOCKS", but I found this cumbersome for regular use (need to ensure the session is up, etc.) I went for "dante" instead - v4, v6, UDP, TCP. And a config language worse than OpenVPN... *cough*. gert
diff --git a/src/openvpn/socks.c b/src/openvpn/socks.c index 6a672c25..2cf0cc9f 100644 --- a/src/openvpn/socks.c +++ b/src/openvpn/socks.c @@ -357,11 +357,16 @@ recv_socks_reply(socket_descriptor_t sd, size = recv(sd, &c, 1, MSG_NOSIGNAL); /* error? */ - if (size != 1) + if (size < 0) { msg(D_LINK_ERRORS | M_ERRNO, "recv_socks_reply: TCP port read failed on recv()"); return false; } + else if (size == 0) + { + msg(D_LINK_ERRORS, "ERROR: recv_socks_reply: empty response from socks server"); + return false; + } if (len == 3) {