8028776: Java socks proxy to resolve DNS remotely

[joshdgreene]

Hello,

I would like to work on bug 8028776: Java socks proxy to resolve DNS remotely, and did an analysis of the current java.net code.

After calling url.openConnection(proxy).getInputStream(), DNS resolution takes place when a new InetSocketAddress is created in NetworkClient.java lines 176-184:

if (connectTimeout >= 0) {
s.connect(new InetSocketAddress(server, port), connectTimeout);
} else {
if (defaultConnectTimeout > 0) {
s.connect(new InetSocketAddress(server, port), defaultConnectTimeout);
} else {
s.connect(new InetSocketAddress(server, port));
}
}

As you can see, in three cases a new InetSocketAddress is created, and this constructor resolves the domain name.
My First idea was to get an unresolved InetSocketAddress when SOCKS was used:

InetSocketAddress targetAddress = proxy.type() == Proxy.Type.SOCKS ?
InetSocketAddress.createUnresolved(server, port) :
new InetSocketAddress(server, port);

if (connectTimeout >= 0) {
s.connect(targetAddress, connectTimeout);
} else {
if (defaultConnectTimeout > 0) {
s.connect(targetAddress, defaultConnectTimeout);
} else {
s.connect(targetAddress);
}
}

Then, SocksSocketImpl just had to send the domain name for unresolved endpoints. Luckily, this has already been implemented in SocksSocketImpl.java, lines 416-421:

if (epoint.isUnresolved()) {
out.write(DOMAIN_NAME);
out.write(epoint.getHostName().length());
out.write(epoint.getHostName().getBytes(StandardCharsets.ISO_8859_1));
out.write((epoint.getPort() >> 8) & 0xff);
out.write((epoint.getPort() >> 0) & 0xff);

While this works flawlessly for SOCKS v5 proxies, this won't work for SOCKS v4. Proxying DNS is not possible with SOCKS v4, so DNS should still be resolved for SOCKS v4. Unfortunately, there are several caveats:

* While the SocksProxy class has a protocolVersion property, this property cannot be read at the location of the change above, because URL.openConnection creates a new Proxy of type ApplicationProxy. According to the comment, this is as a security measure. This copy is not a full copy, because it lacks the SocksProxy protocolVersion property.
* SocksSocketImpl.useV4 (which determines whether to use v4 or v5) does not only check the proxy type (which AFAIK never is SocksProxy), but also DefaultProxySelector.socksProxyVersion() == 4 (line 74). While convenient, there could be a difference between the socksProxyVersion in the SocksProxy object that was originally created, and the version in the system property (even though the SocksProxy property cannot currently be set programmatically).
* The change in NetworkClient.java above could use the same trick, to check DefaultProxySelector.socksProxyVersion(). However, lines 397-406 prevent this. Starting at line 389:

// This is SOCKS V5
out.write(PROTO_VERS);
out.write(2);
out.write(NO_AUTH);
out.write(USER_PASSW);
out.flush();
byte[] data = new byte[2];
int i = readSocksReply(in, data, deadlineMillis);
if (i != 2 || ((int)data[0]) != PROTO_VERS) {
// Maybe it's not a V5 sever after all
// Let's try V4 before we give up
// 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;
}

So even if SOCKS v5 is configured, SocksSocketImpl tries v4 if v5 fails, and then v4 will always fail because the address is unresolved. The fallback from v5 to v4 breaks the first idea.

Before I found out that the proxy object was changed into an ApplicationProxy object, I thought about adding a 'proxyDns' property to the SocksProxy class. When this property would be explicitly set to true, proxying DNS is expected, so SOCKS v5 is mandatory. In this case, it wouldn't matter if the fallback would throw an exception. However, the SocksProxy information disappears in the ApplicationProxy copy, and as far as I see, there is no easy way to create a SocksProxy with the right version and a new property from code, without using system properties and the ProxySelector. Maybe a ProxyFactory could help out, but such a class does not exist.

Currently, the version of SOCKS can only be set in system property socksProxyVersion. We could introduce a new system property socksProxyDns, which tells NetworkClient to proxy DNS when using SOCKS, like this:

private static final String SOCKS_PROXY_DNS = "socksProxyDns";
public static boolean socksProxyDns() {
Boolean val = AccessController.doPrivileged(
new PrivilegedAction<Boolean>() {
@Override public Boolean run() {
return NetProperties.getBoolean(SOCKS_PROXY_DNS);
}
});
return val == null ? false : val;
}

Again, NetworkClient.java lines 176-184, now checking the new property:

InetSocketAddress targetAddress = proxy.type() == Proxy.Type.SOCKS && socksProxyDns() ?
InetSocketAddress.createUnresolved(server, port) :
new InetSocketAddress(server, port);

if (connectTimeout >= 0) {
s.connect(targetAddress, connectTimeout);
} else {
if (defaultConnectTimeout > 0) {
s.connect(targetAddress, defaultConnectTimeout);
} else {
s.connect(targetAddress);
}
}

This works, and compatibility-wise, I think this is almost risk free. But from an application programmer's point of view, I'd prefer a way to specify the SOCKS version and DNS proxying in type safe code instead of a system property.

What would be the preferred solution? What can I do next?

I already prepared some jtreg tests to test the proxy (SOCKS protocol is very easy to test). However, I'd like to guarantee that no local DNS query is made when SOCKS v5 with DNS proxying is enabled, and that a local DNS query is made when DNS proxying is not enabled. Unfortunately, I didn't find a clean way to replace the default NameService implementation. I tested this with a NetworkService class implementing an InvocationHandler, which I assigned to the static nameService property of InetAddress. Even though this currently works, it may not work in a future release:

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access to field java.net.InetAddress.nameService
WARNING: Please consider reporting this to the maintainers of this class
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release

Does anyone have any suggestions to replace the NameService in test code?

-Josh
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/net-dev/attachments/20200630/c8030ead/attachment-0001.htm>