Incorrect IPv6/hostname/port displayed in SOCKS5 connection error #944

Closed
DonKult opened this Issue Aug 6, 2016 · 7 comments

Projects

None yet

3 participants

@DonKult
DonKult commented Aug 6, 2016

While writing a socks5 client, testing the results & comparing it to other implementations

I did this

socat -x tcp-listen:2903,reuseaddr system:"echo -n '$CODE' | xxd -r -p"
with three different $CODEs being:

05 00 05 04 00 01 ac 10 fe 01 1f 90
05 00 05 04 00 03 09 68 6f 73 74 6c 6f 63 61 6c 1f 90
05 00 05 04 00 04 20 01 0d b8 ac 10 fe 00 00 00 00 00 00 00 00 00 1f 90

(aka: pretending to be a socks5 server with no auth and failing the request with IPv4, hostname & IPv6)
and each time: curl --proxy socks5h://localhost:2903 http://example.org

I expected the following

For the first (ipv4) code the result is correct:
curl: (7) Can't complete SOCKS5 connection to 172.16.254.1:8080. (4)
The hostname one should have been hostlocal:8080, but is:
curl: (7) Can't complete SOCKS5 connection to localhost:29804. (4)
The IPv6 should be [2001:0DB8:AC10:FE00:0000:0000:0000:0000]:8080, but is:
curl: (7) Can't complete SOCKS5 connection to 2001:0db8:ac10:686f:7374:1f90:0000:8000:44048. (4)

Looking at the source I think this len = 10; is the start of the issue at least for the IPv6 as that is "just" the right value for IPv4. The wrong port is the result of a copy&paste from IPv4 I presume and the wrong hostname is caused by displaying the hostname the request was made for completely discarding what the server reports as hostname it has connected to (which is likely the same – at least if your starting point was a hostname and not an ip –, but still). In case you wonder: Depending on the length of the request the code is using uninitialized memory for IPv6 (aka the later octets of the address), but I fail to imagine a way to exploit that…

The non-error case does deal with all three types correctly btw – that isn't very hard through as it is just discarding the data. I would have kinda expected a debug message here but nope.

So in summary: Worst bugs ever… if I would know how to set a minor severity on github issues I would do it.

[dropping the rest of the bug template as presumably not relevant]

@bagder
Member
bagder commented Aug 6, 2016

I don't understand.

The hostname one should have been hostlocal:8080, but is:
curl: (7) Can't complete SOCKS5 connection to localhost:29804. (4)

Why "hostlocal" ? And did it really say 29804 when you connected to 2903? That seems really weird.

The IPv6 should be [2001:0DB8:AC10:FE00:0000:0000:0000:0000]:8080, but is:

Sorry, but why/how should it be that? And is this perhaps related to https://curl.haxx.se/docs/knownbugs.html#SOCKS_proxy_not_working_via_IPv6 ?

@DonKult
DonKult commented Aug 6, 2016

With the codes like 05 00 05 04 00 03 09 68 6f 73 74 6c 6f 63 61 6c 1f 90 I use socat to pretend to curl that it is talking to a socks5 proxy, which in this case is negotiating for no authorization and denies the connection with error code 4 to the server detailed in the rest of the reply. The 68 6f 73 74 6c 6f 63 61 6c is the hostname which is "hostlocal" in hex. I should probably have chosen a different hostname, but I was lazy by just moving octets around in testing. The two following octets defining the port are equally made up just for testing (I picked port 8080 just because). So, no real socks5 proxy is involved, I am just simulating what a socks5 would do to observe what the client does for testing.

Regarding port, just look at the source, for IPv6 its perhaps clearest: https://github.com/curl/curl/blob/master/lib/socks.c#L689
For all three types it chooses the 9th and 10th octet to calculate the port. That is correct for IPv4, but incorrect for hostname (logically, that can't be a fixed number given that the hostname is variable length and the port follows after the hostname) and for IPv6 the 9th and 10th octet are in the middle of the IPv6 number itself, so that logically can't really be producing the right port either.

These are all just display issues in error messages – no effect on successful runs and hence also unrelated to the known bugs – through as far as I know socks4 doesn't support IPv6 – one of the reasons for socks5…

I hope that makes it slightly clearer. Sorry for not being more explicit – having just recently read the relevant RFCs makes this all super obvious, but in a few days I would probably haven't understood myself either. ;)

@mback2k mback2k added a commit that referenced this issue Aug 14, 2016
@mback2k mback2k socks.c: Move error output after reading the whole response packet
First commit to fix issue #944 regarding SOCKS5 error handling.

Reported-by: David Kalnischkies
59580e1
@mback2k mback2k added a commit that referenced this issue Aug 14, 2016
@mback2k mback2k socks.c: Do not modify and invalidate calculated response length
Second commit to fix issue #944 regarding SOCKS5 error handling.

Reported-by: David Kalnischkies
cc3384a
@mback2k mback2k added a commit that referenced this issue Aug 14, 2016
@mback2k mback2k socks.c: Correctly calculate position of port in response packet
Third commit to fix issue #944 regarding SOCKS5 error handling.

Reported-by: David Kalnischkies
b7ee531
@mback2k
Member
mback2k commented Aug 14, 2016 edited

@DonKult Except not using the returned hostname, the incomplete response and invalid port issues should be fixed by the 3 commits mentioned above. Please try the current master. I did not fix the hostname part, because I think validating that the server did not return a malicious combination of hostname and its length is not worth the effort for this error case. So curl will display the requested hostname instead of the returned hostname in this error case.

@mback2k mback2k self-assigned this Aug 14, 2016
@DonKult
DonKult commented Aug 14, 2016

(not tested the commits yet)
The thing with the hostname is that the hostname displayed is the one used in the request, not the hostname the server returned. Most of the time it will be the same, but lets imagine your socks-proxy does support SRV records, so that you will request example.org and e.g. (via SRV lookup) get back foobar.example.net. Nothing malicious about it, but if its worth to fix it or not is your call of course.

@mback2k
Member
mback2k commented Aug 14, 2016

In any case it should probably return both the requested and the returned IP or hostname. That would probably be the most clearest solution.

@mback2k
Member
mback2k commented Aug 20, 2016

@DonKult I just pushed a series of commits to the master branch in order to streamline the verbose debug and error messages during the SOCKS4 and SOCKS5 connection sequences. At least in case of an error in the final phase of the connection sequence, the destination (IPv4, hostname or IPv6) returned by the proxy server will be displayed. Please check if the issue can now be considered fixed.

@bagder
Member
bagder commented Aug 28, 2016

Presumably fixed, closing

@bagder bagder closed this Aug 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment