Okhttp: Socks 4 Proxy broken

[dain]

OkHttp relies on the JVM implementation of SOCKS, which seems to be broken for SOCKS4. When the JVM establishes the connection in java.net.SocksSocketImpl#connect, it first sends a SOCKS5 setup message:

        // This is SOCKS V5
        out.write(PROTO_VERS);
        out.write(2);
        out.write(NO_AUTH);
        out.write(USER_PASSW);
        out.flush();

and then reads a 2 byte response. Then it checks if the first byte of the response is not 5, and if so it tries a socks 4 message _using the same socket_.

The problem is since this above message is not a wellformed SOCKS4 message, the SOCKS4 server doesn't know how much data to read. Also a SOCKS4 response is 8 bytes, and the code only drains 2 bytes. This means that when it gets to the SOCKS4 connect, the socket is in an unknown state and always fails.

I'm not sure anything can be done other than hacking the JVM with reflection, or to write a new implementation of SOCKS 4 and 5.

Regardless of the solution the broken SOCKS 4 support should be documented, so no one else has to dig into the terrible SOCKS code in the JVM.

[swankjesse]

Any advice on how we should handle this in OkHttp?

• We could implement SOCKS in the application layer, which is annoying, but not _that_ annoying. We already do it for our test server.
• We could document that this is a problem. (Where would we do that?)

[dain]

I'm not sure there is any way around this other than to implement SOCKS in OkHttp. The protocol is pretty simple, and I have a testing SOCKS proxy implementation here https://github.com/airlift/airlift/blob/master/http-client/src/test/java/io/airlift/http/client/jetty/TestingSocksProxy.java.

As for where to document this, I'd add it to the FAQ. It would be good to test this on Android also, as it might correctly implement SOCKS.

[swankjesse]

Yup. Our testing proxy is here: https://github.com/square/okhttp/blob/master/okhttp-tests/src/test/java/com/squareup/okhttp/SocksProxy.java

Who is using SOCKS4?

[dain]

I only noticed it because my testing socks proxy only implemented SOCKS4.

[swankjesse]

I'm putting this in the icebox for now. If/when somebody really wants SOCKS4, they can bring it up here and probably submit a pull request.

[Vampire]

SOCKS 4 support in Java 8 is totally broken, pretty much as described here.
Due to erroneous implementation in java.net.SocksSocketImpl, it always did SOCKS 5 logic and never went the SOCKS 4 path.
In Java 9 this is fixed and the SOCKS 4 format is properly sent if configured.

[Vampire]

Yet OkHttp hinders proper SOCKS 4 handshake, because of

if (proxy.type() == Proxy.Type.SOCKS) {
  inetSocketAddresses.add(InetSocketAddress.createUnresolved(socketHost, socketPort));
} else {

in OkHttp code and

if (useV4) {
    // SOCKS Protocol version 4 doesn't know how to deal with
    // DOMAIN type of addresses (unresolved addresses here)
    if (epoint.isUnresolved())
        throw new UnknownHostException(epoint.toString());
    connectV4(in, out, epoint, deadlineMillis);
    return;
}

in java.net.SocksSocketImpl, as it only does SOCKS4 which does not work with hostnames and not SOCKS4a which could work with unresolved hostnames if the server supports it.

Hence this results in an UnknownHostException.

[Vampire]

4265 fixes what I mentioned in my last comment

[ogolovanov]

Hi.

As i understand its still not possible to use socks v4 proxies with okhttp client, right?
Not with java8, not with java9, not with java10?

Thanks.

[Vampire]

As long as @swankjesse dosn't change his mind and accepts my PR, you are right.
No SOCKS4, no SOCKS5 if the proxy cannot resolve the hostname but only the client, no custom resolution of hostnames to IPs by supplying a Dns instance.

Unless of course you supply a custom socket factory that can do it despite OkHttp actively trying to prevent it by creating explicitly unresolved InetSocketAddresses.

[ogolovanov]

As long as @swankjesse dosn't change his mind and accepts my PR, you are right.
No SOCKS4, no SOCKS5 if the proxy cannot resolve the hostname but only the client, no custom resolution of hostnames to IPs by supplying a Dns instance.

Unless of course you supply a custom socket factory that can do it despite OkHttp actively trying to prevent it by creating explicitly unresolved InetSocketAddresses.

Thanks.

Could you please point me to right direction, so i could try to implement it myself?
May be some template code or something else.
Really need socks4 support in okhttp.

[Vampire]

All you have to do is to convince Jesse. :-D
If he would accept my PR, SOCKS4 amongst other things would work with Java 9 and up.

Or you can build OkHttp yourself with my PR applied.

Or you can search for a lib that provides a socket factory that is able to handle SOCKS4 and supply it to OkHttpClient.

Or you can implement your own, but I didn't do this yet, so I cannot advise much on that way right now.

[swankjesse]

This will work for both of you: https://gist.github.com/swankjesse/132ecc49bee853c3c335d33b9a95bef7

[Vampire]

Ah, nice idea for a workaround to have an intermediate socket factory that simply resolves the hostname, thanks, this works around all three issues with both SOCKS4 and SOCKS5 I've seen and that my PR solves. I'll definitely give it a try when I'm back from vacation.

@ogolovanov you still need to use Java 9 or newer, SOCKS4 is completely and SOCKS5 partly broken in Java 8.

[yschimke]

This sort of thing might be a good contribution to https://github.com/MarkusBernhardt/proxy-vole, but the gist is probably good for specific corporate apps that have known proxies.

[Vampire]

Isn't proxy-vole about finding proxy settings from other programs or the OS? Not about providing an actual socket factory.

Why do you think the Gist is only good for specific known proxies? As far as I had understood from a quick skim over, it just resolves the hostname and then hands over to the standard socket factory, so basically what my patch does.

[yschimke]

Because this shouldn't be on by default.

If it's not on by default, then why confuse things about when a developer of a popular public Android app should enable this, roughly the answer is never.

Do you suggest we provide this as an optional class provided in okhttp that people could use? When should they enable this? I'm still stuck on the questions @swankjesse has e.g. https://github.com/square/okhttp/pull/4265#issuecomment-423153637

But developers writing apps using environment configured proxies e.g. writing corporate apps using a PAC config, using a library like proxy-vole probably want stuff to just work when the PAC file points at a SOCKS 4 proxy. So that was why I referenced that project.

Anyway just a suggestion.

[Vampire]

No, I'm not suggesting to add this to OkHttp.
I suggest to add my patch, hence the PR.
The effect is basically the same that Gist does, while my PR is a proper fix of the bug while the Gist is just a workaround.

[Vampire]

@swankjesse are you sure your Gist should work?
I adapted the idea but cannot get it to work, because of

rawSocket = proxy.type() == Proxy.Type.DIRECT || proxy.type() == Proxy.Type.HTTP
    ? address.socketFactory().createSocket()
    : new Socket(proxy);

So the socket factory is not even used for a socks proxy.

[swankjesse]

It works if you trick OkHttp into thinking there's no proxy.

[Vampire]

Besides that this is a pretty horrible work-around - except maybe if you know a SOCKS proxy is used - as you then also have to do the proxy-selection logic yourself, are you sure it would work then?
The same snippet I quoted will then not do new Socket(proxy);, but address.socketFactory().createSocket() and later call connect on that socket with the unresolved inetaddress, so if I'm not wrong to make it work you would also have to provice a custom Socket class and there do the address resolution.

[swankjesse]

I think it'll be a resolved address in that case.

[Vampire]

Ah right, because OkHttp only uses unresolved address if proxy type is SOCKS.
But I still think that with also having to have custom duplicated proxy selection logic and so on the workaround is pretty ugly compared to being able to just switch an option in OkHttp. :-(

Can't we just fix the bug in OkHttp?
If the option with all the boolean-giving-around is too intrusive, maybe then simply controlled by a system property that is for experts-only use that know when they want it, or that you can tell to people when they say "Hey, I need SOCKS4 support"?

[ogolovanov]

Sorry, any update on this?
Or any recommendations, please, how can i use socks4 in "ugly" way?

[Vampire]

@ogolovanov as I wrote, you can use the Gist from https://github.com/square/okhttp/issues/1359#issuecomment-423748310 when you also add some custom proxy selection logic, or you can apply my PR to a fork you use, or you can convince the guys here to fix the bug with my PR or something similar. :-)

[swankjesse]

@ogolovanov @Vampire please just use JDK 9 or newer, which has the bug fixed there. I’m not enthusiastic to add APIs to OkHttp to workaround bugs in old versions of Java.

[Vampire]

@swankjesse you are totally wrong.
In JDK <8 SOCKS4 is never used due to buggy implementation.
In JDK >=9 SOCKS4 implementation and also a bug in SOCKS5 implementation is fixed and works properly, but only if you have a resolved inet address.
The problem is, that OkHttp just provides an unresolved inet address, so SOCKS4 does not work with Java 9 unless you accept my PR or a similar solution, maybe just controlled by a system property instead of the boolean-giving-around.

[swankjesse]

Gotcha. Is there a tracking bug in JDK >= 9 ?

[yschimke]

In Java 9+, with an implicit (no flag) resolve only when proxy == SOCKS, would this work for SOCKS4 + SOCKS5?

@Vampire What's the main driver for this? Corporate usage? Or gamers getting around region blocks?

If we lost security only when Socks is enabled, but we could get it back by using a DNS over HTTPS implementation, then maybe that is a workable solution? In a corporate usage you legitimately may not be able to resolve the DNS from the client. But if its for region blocks, then it will probably work.

[Vampire]

@swankjesse
There is no bug entry for JDK as there is no bug in the JDK imho.
You explicitly give it an unresolved address, who is the JDK to ignore your explicit choice and "leak" the domain name to the DNS server?
Besides that, it would use the system configured DNS server instead of the OkHttp configured Dns instance.

@yschimke

In Java 9+, with an implicit (no flag) resolve only when proxy == SOCKS, would this work for SOCKS4 + SOCKS5?

Yes, it would.
Actually this was the first version of my PR, as not only SOCKS4 is broken by this, but also SOCKS5 in case the configured Dns instance could resolve the address while the proxy could not.
You can see it at https://github.com/Vampire/okhttp/commit/924c117530fba2f3370d414ecada59cbae59a64b.

There the concern of Jesse was, that for example for Tor the addresses would be "leaked" to the system DNS server. That was when I changed the PR to have a configuration option. If this makes the feature too obvious and confuses the users, my suggestion was to have an experts-only system property instead, that could be used to toggle that behavior.

Another option would be to do as you suggest and was my frist version, always resolve for SOCKS proxies. If you then implement something that should not leak to DNS, I'd expect it to set a custom Dns instance that always returns an empty list unconditionally, so the hostnames would not leak to the dns server.

[Aeroverra]

I would like to add that while I do love this library for my projects I am getting real close to leaving it behind due to this sock 4 problem. @Vampire has provided a proper fix a while back so why has it not been accepted or reasoned otherwise? I am currently using socket factories as if it was a proxy and needing to reflect and inject upon each sock 4 individually to get this to work properly and prooving a bit unstable. I have considered adding a branch to my project and compiling the fix myself but at that point, I would just rather move directly to the source of the requests. If you would like sock 4 proxies to test with I would be happy to generate some for you.

[swankjesse]

@Aeroverra what are you using SOCKS 4 support for?

[yschimke]

Closing because we have baulked at high quality PRs in the past, so let's be honest that this isn't happening in OkHttp. We would probably only consider minimal API changes that allow custom socket factory if that isn't already working (it may be).