android: handling ipv6 literals

android: handling ipv6 literals

[Peter Gervai]

Hello,

Andoid client mess up ipv6 literals: input accept the - correct -
[1234:5678::1]:12345 format, but on the next edit the field gets
transformed to 1234:5678::1:12345 which obviously won't quite work.

A sidenote: pinging from the client could be useful (I have network
tools installed, but without it it's hard to tell what's going on).


Peter

[PATCH android] config: fix wrong Peer endpoint string format

[Zhao Gang]

When a tunnel is running, saving the tunnel's config with an IPv6 address endpoint like [::1]:42 would result in the wrong format ::1:42. This patch fixes it.

For endpoints with an IPv6 address(e.g. [::1]:42). Since the default endpoint InetSocketAddress is created unresolved, getEndpointString() returns "[::1]:42" (InetSocketAddress.getHostString() returns the literal hostname). After the endpoint is resolved, getEndpointString() returns "::1:42" (InetSocketAddress.getHostString() returns the IPv6 address without the square brackets). This inconsistent return values caused the above mentioned bug.

With this patch, function getEndpointString would return the right format string whether the endpoint is resolved or not. Also changed the function getResolvedEndpointString to call getEndpointString instead of making the string itself.

Signed-off-by: Zhao Gang <gang.zhao.42@gmail.com>
---
 app/src/main/java/com/wireguard/config/Peer.java | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/app/src/main/java/com/wireguard/config/Peer.java b/app/src/main/java/com/wireguard/config/Peer.java
index 49c8b70..4c8703c 100644
--- a/app/src/main/java/com/wireguard/config/Peer.java
+++ b/app/src/main/java/com/wireguard/config/Peer.java
@@ -71,7 +71,11 @@ public class Peer {
     private String getEndpointString() {
         if (endpoint == null)
             return null;
-        return String.format("%s:%d", endpoint.getHostString(), endpoint.getPort());
+
+        if (endpoint.getHostString().contains(":") && !endpoint.getHostString().contains("["))
+            return String.format("[%s]:%d", endpoint.getHostString(), endpoint.getPort());
+        else
+            return String.format("%s:%d", endpoint.getHostString(), endpoint.getPort());
     }
 
     public int getPersistentKeepalive() {
@@ -102,13 +106,8 @@ public class Peer {
             endpoint = new InetSocketAddress(endpoint.getHostString(), endpoint.getPort());
         if (endpoint.isUnresolved())
             throw new UnknownHostException(endpoint.getHostString());
-        if (endpoint.getAddress() instanceof Inet6Address)
-            return String.format("[%s]:%d",
-                    endpoint.getAddress().getHostAddress(),
-                    endpoint.getPort());
-        return String.format("%s:%d",
-                endpoint.getAddress().getHostAddress(),
-                endpoint.getPort());
+
+        return getEndpointString();
     }
 
     public void parse(final String line) {
-- 
2.18.0

Re: [PATCH android] config: fix wrong Peer endpoint string format

[Jason A. Donenfeld]

Hi Gang,

Thanks for the patch, I'll merge the first hunk of this but not the
second, because:

On Thu, Aug 16, 2018 at 11:58 AM Zhao Gang <gang.zhao.42@gmail.com> wrote:
> -        return String.format("%s:%d", endpoint.getHostString(), endpoint.getPort());
> +
> +        if (endpoint.getHostString().contains(":") && !endpoint.getHostString().contains("["))
> +            return String.format("[%s]:%d", endpoint.getHostString(), endpoint.getPort());
> +        else
> +            return String.format("%s:%d", endpoint.getHostString(), endpoint.getPort());

This seems like a fix for the bug. But:

> -        if (endpoint.getAddress() instanceof Inet6Address)
> -            return String.format("[%s]:%d",
> -                    endpoint.getAddress().getHostAddress(),
> -                    endpoint.getPort());
> -        return String.format("%s:%d",
> -                endpoint.getAddress().getHostAddress(),
> -                endpoint.getPort());

The purpose of the code here is to resolve the hostname to an IP
address, and then return ip+port, with no hostnames in it. Replacing
it with a call to the prior fixed function isn't the intended
behavior.

I've committed your fix here as:
https://git.zx2c4.com/wireguard-android/commit/?id=ace2a77bc9019d050112bed5a5d1eb2a3363d971

Thanks again,
Jason

[PATCH android v2] config: fix wrong Peer endpoint string format

[Zhao Gang]

When a tunnel is running, saving the tunnel's config with an IPv6 address endpoint like [::1234]:42 would result in the wrong format ::1234:42. This patch fixes it.

For endpoints with an IPv6 address(e.g. [::1234]:42). Since the default endpoint InetSocketAddress is created unresolved, getEndpointString() returns "[::1234]:42" (InetSocketAddress.getHostString() returns the literal hostname). After the endpoint is resolved, getEndpointString() returns "::1234:42" (InetSocketAddress.getHostString() returns the IPv6 address without the square brackets). This inconsistent return values caused the above mentioned bug.

With this patch, function getEndpointString would return the right format string whether the endpoint is resolved or not. Also changed the function getResolvedEndpointString to call getEndpointString instead of making the string itself.

Reported-by: Peter Gervai <grin@grin.hu>
Signed-off-by: Zhao Gang <gang.zhao.42@gmail.com>
---
 app/src/main/java/com/wireguard/config/Peer.java | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/app/src/main/java/com/wireguard/config/Peer.java b/app/src/main/java/com/wireguard/config/Peer.java
index 49c8b70..40b77ff 100644
--- a/app/src/main/java/com/wireguard/config/Peer.java
+++ b/app/src/main/java/com/wireguard/config/Peer.java
@@ -15,7 +15,6 @@ import android.support.annotation.Nullable;
 import com.android.databinding.library.baseAdapters.BR;
 import com.wireguard.crypto.KeyEncoding;
 
-import java.net.Inet6Address;
 import java.net.InetSocketAddress;
 import java.net.URI;
 import java.net.URISyntaxException;
@@ -71,7 +70,11 @@ public class Peer {
     private String getEndpointString() {
         if (endpoint == null)
             return null;
-        return String.format("%s:%d", endpoint.getHostString(), endpoint.getPort());
+
+        if (endpoint.getHostString().contains(":") && !endpoint.getHostString().contains("["))
+            return String.format("[%s]:%d", endpoint.getHostString(), endpoint.getPort());
+        else
+            return String.format("%s:%d", endpoint.getHostString(), endpoint.getPort());
     }
 
     public int getPersistentKeepalive() {
@@ -102,13 +105,8 @@ public class Peer {
             endpoint = new InetSocketAddress(endpoint.getHostString(), endpoint.getPort());
         if (endpoint.isUnresolved())
             throw new UnknownHostException(endpoint.getHostString());
-        if (endpoint.getAddress() instanceof Inet6Address)
-            return String.format("[%s]:%d",
-                    endpoint.getAddress().getHostAddress(),
-                    endpoint.getPort());
-        return String.format("%s:%d",
-                endpoint.getAddress().getHostAddress(),
-                endpoint.getPort());
+
+        return getEndpointString();
     }
 
     public void parse(final String line) {
-- 
2.18.0