search

UWNetworksLab / cordova-plugin-tun2socks

Cordova plugin to enable a system-wide VPN for Android devices.
Apache License 2.0
51 stars 24 forks source link

Plugin implementation #3

Closed alalamav closed 8 years ago

alalamav commented 8 years ago

This change is Reviewable

bemasc commented 8 years ago

Review status: 0 of 19 files reviewed at latest revision, 13 unresolved discussions.

_plugin.xml, line 27 [r2] (raw file):_


    <config-file target="AndroidManifest.xml" parent="/manifest/application">

Nit: blank line

src/android/libs/jsocks.jar, line 0 [r2] (raw file): Obviously it's a bit unfortunate to be committing a JAR file. How big is this file? Do we have an alternative way to include it in the build?

src/android/org/uproxy/tun2socks/DnsResolverService.java, line 28 [r2] (raw file):


// This class listens on a UDP socket for incoming DNS traffic, which is proxy'd
// through SOCKS over TCP to Honest DNS.

Nobody calls it Honest DNS :)

src/android/org/uproxy/tun2socks/DnsResolverService.java, line 141 [r2] (raw file):

          Log.i(LOG_TAG, "listening on " + udpSocket.getLocalSocketAddress().toString());
          try {
            udpSocket.receive(udpPacket);

What happens if someone sends a packet that's bigger than 512 bytes?

src/android/org/uproxy/tun2socks/DnsResolverService.java, line 199 [r2] (raw file):

      } catch (Throwable e) {
        if (!isInterrupted()) {
          e.printStackTrace();

What state is the app now in? Is it silently broken?

src/android/org/uproxy/tun2socks/DnsResolverService.java, line 208 [r2] (raw file):

    }

    private boolean validateDnsRequest(DatagramPacket packet) {

Nit: Prefer isValid over validate. Also, could this method be static?

src/android/org/uproxy/tun2socks/Tun2Socks.java, line 97 [r2] (raw file):

  }

  private boolean hasVpnService() {

Nit: I think it would be worth exposing this as a command. Then maybe we wouldn't need to check the OS version in the JS code; we could just ask the plugin whether it's going to work.

_src/android/org/uproxy/tun2socks/Tun2Socks.java, line 104 [r2] (raw file):_

  protected boolean prepareVpnService() throws ActivityNotFoundException {
    // VpnService: need to display OS user warning. If whole device
    // option is selected and we expect to use VpnService, so the prompt here in

Nit: "show the prompt"?

src/android/org/uproxy/tun2socks/Tunnel.java, line 127 [r2] (raw file):

  // synchronized functions as stop() is synchronized and a deadlock is possible as callbacks
  // can be called while stop holds the lock.
  @TargetApi(Build.VERSION_CODES.LOLLIPOP)

Why lollipop? Deserves a comment.

src/android/org/uproxy/tun2socks/TunnelManager.java, line 59 [r2] (raw file):


  // Implementation of android.app.Service.onStartCommand
  @TargetApi(Build.VERSION_CODES.M)

Why M? Needs comment.

src/badvpn/tun2socks/tun2socks.c, line 323 [r2] (raw file):


// Initializes a UDP socket, binds it locally and connects it to a local
// address.

What's the return value?

src/badvpn/tun2socks/tun2socks.c, line 337 [r2] (raw file):

        return 0;
    }
    // Bind to any local address in order to receive data

INADDR_ANY is not the same as "any local address". It's more like "the ANY address".

src/badvpn/tun2socks/tun2socks.h, line 48 [r2] (raw file):

//#define OVERRIDE_DEST_ADDR "10.111.0.2:2000"

// maximum number of bytes for udp send/recv

Is this ever applied to UDP sending?

Comments from Reviewable

alalamav commented 8 years ago

Review status: 0 of 19 files reviewed at latest revision, 13 unresolved discussions.

_plugin.xml, line 27 [r2] (raw file):_

Previously, bemasc (Benjamin M. Schwartz) wrote… > Nit: blank line >

Done.

src/android/libs/jsocks.jar, line [r2] (raw file):

Previously, bemasc (Benjamin M. Schwartz) wrote… > Obviously it's a bit unfortunate to be committing a JAR file. How big is this file? Do we have an alternative way to include it in the build? >

The JAR is 395 KB. The source code is in SourceForge, but seems like it's not actively maintained. We could create our fork, and include the JAR ad hoc. But I don't know if this is a better alternative.

_src/android/org/uproxy/tun2socks/DnsResolverService.java, line 28 [r2] (raw file):_

Previously, bemasc (Benjamin M. Schwartz) wrote… > Nobody calls it Honest DNS :) >

lol

src/android/org/uproxy/tun2socks/DnsResolverService.java, line 141 [r2] (raw file):

Previously, bemasc (Benjamin M. Schwartz) wrote… > What happens if someone sends a packet that's bigger than 512 bytes? >

As discussed offline, the DNS over UDP limit is 512 bytes and it's very unlikely that a query will exceed this size. However, the response could have a size of 32767 (max UDP packet size). I'll increase the buffer size to account for this.

src/android/org/uproxy/tun2socks/DnsResolverService.java, line 199 [r2] (raw file):

Previously, bemasc (Benjamin M. Schwartz) wrote… > What state is the app now in? Is it silently broken? >

I've refactored this class to catch all exceptions, so this catch is no longer needed and there's no ambiguity about what went wrong.

src/android/org/uproxy/tun2socks/DnsResolverService.java, line 208 [r2] (raw file):

Previously, bemasc (Benjamin M. Schwartz) wrote… > Nit: Prefer `isValid` over `validate`. Also, could this method be static? >

Done. It cannot because non-static nested classes are not allowed to declare static methods.

_src/android/org/uproxy/tun2socks/Tun2Socks.java, line 104 [r2] (raw file):_

Previously, bemasc (Benjamin M. Schwartz) wrote… > Nit: "show the prompt"? >

Done.

src/android/org/uproxy/tun2socks/Tunnel.java, line 127 [r2] (raw file):

Previously, bemasc (Benjamin M. Schwartz) wrote… > Why lollipop? Deserves a comment. >

Done.

src/android/org/uproxy/tun2socks/TunnelManager.java, line 59 [r2] (raw file):

Previously, bemasc (Benjamin M. Schwartz) wrote… > Why M? Needs comment. >

Done.

src/badvpn/tun2socks/tun2socks.c, line 323 [r2] (raw file):

Previously, bemasc (Benjamin M. Schwartz) wrote… > What's the return value? >

Done.

_src/badvpn/tun2socks/tun2socks.c, line 337 [r2] (raw file):_

Previously, bemasc (Benjamin M. Schwartz) wrote… > INADDR_ANY is not the same as "any local address". It's more like "the ANY address". >

Done.

_src/badvpn/tun2socks/tun2socks.h, line 48 [r2] (raw file):_

Previously, bemasc (Benjamin M. Schwartz) wrote… > Is this ever applied to UDP sending? >

See my previous comment on DNS/UDP limits.

Comments from Reviewable

bemasc commented 8 years ago

Review status: 0 of 19 files reviewed at latest revision, 7 unresolved discussions.

src/android/org/uproxy/tun2socks/DnsResolverService.java, line 141 [r2] (raw file):

Previously, albertolalama (alberto lalama) wrote… > As discussed offline, the DNS over UDP limit is 512 bytes and it's very unlikely that a query will exceed this size. However, the response could have a size of 32767 (max UDP packet size). > I'll increase the buffer size to account for this. >

Seems like the max UDP packet size is 65535. (However, I agree that 32K is probably plenty.)

_src/android/org/uproxy/tun2socks/TunnelManager.java, line 59 [r2] (raw file):_

Previously, albertolalama (alberto lalama) wrote… > Done. >

Hmm. I don't see a comment.

_src/badvpn/tun2socks/tun2socks.c, line 323 [r2] (raw file):_

Previously, albertolalama (alberto lalama) wrote… > Done. >

It looks like it returns the socket's file descriptor number, not the IP address.

src/badvpn/tun2socks/tun2socks.c, line 337 [r2] (raw file):

Previously, albertolalama (alberto lalama) wrote… > Done. >

OK ... this means that you can receive packets from anyone on the internet who manages to send a packet to this port, because ANY includes the public interface(s). Is that a problem?

Comments from Reviewable

alalamav commented 8 years ago

Review status: 0 of 19 files reviewed at latest revision, 7 unresolved discussions.

src/android/org/uproxy/tun2socks/Tun2Socks.java, line 97 [r2] (raw file):

Previously, bemasc (Benjamin M. Schwartz) wrote… > Nit: I think it would be worth exposing this as a command. Then maybe we wouldn't need to check the OS version in the JS code; we could just ask the plugin whether it's going to work. >

Done. I'll call this method from uProxy once it has been checked in.

src/android/org/uproxy/tun2socks/TunnelManager.java, line 59 [r2] (raw file):

Previously, bemasc (Benjamin M. Schwartz) wrote… > Hmm. I don't see a comment. >

This method used to call bindProcessToNetwork. The TargetApi annotation is no longer necessary. I updated the comment of pluginInitalize, which now calls bindProcessToNetwork.

src/badvpn/tun2socks/tun2socks.c, line 323 [r2] (raw file):

Previously, bemasc (Benjamin M. Schwartz) wrote… > It looks like it returns the socket's file descriptor number, not the IP address. >

yes. fixed now.

_src/badvpn/tun2socks/tun2socks.c, line 337 [r2] (raw file):_

Previously, bemasc (Benjamin M. Schwartz) wrote… > OK ... this means that you can receive packets from anyone on the internet who manages to send a packet to this port, because ANY includes the public interface(s). Is that a problem? >

Although unlikely, it is not impossible for an attacker to inject fake DNS responses. I'm binding the socket to a the loopback address instead.

Comments from Reviewable

trevj commented 8 years ago

Great work. I've been running VPN on my own phone, too, with good results.

I'm mostly looking at this from the perspective of maintainability:

I think we should expand the README. Thanks to chatting with @albertolalama and my own poking around, is this the following a fair summary?:

We re-use and have used as a starting point a bunch of open source from Psiphon, specifically https://github.com/mei3am/ps

alalamav commented 8 years ago

Thank you for compiling a succinct summary, Trevor. I'll make sure to update the README with the code sources. You raise an interesting question around code maintenance and how to keep up with upstream fixes. I don't have a straight answer at this point, but I think this is something we should discuss with @bemasc.

bemasc commented 8 years ago

:+1: with nits. I know you have more improvements queued up, so let's land this and get to the next one!

bemasc commented 8 years ago

src/android/libs/jsocks.jar, line [r2] (raw file):

Previously, albertolalama (alberto lalama) wrote… > The JAR is 395 KB. The source code is in SourceForge, but seems like it's not actively maintained. We could create our fork, and include the JAR ad hoc. But I don't know if this is a better alternative. >

Fixed!

Comments from Reviewable

bemasc commented 8 years ago

:lgtm: with one tiny nit

Review status: 0 of 18 files reviewed at latest revision, 1 unresolved discussion.

_src/android/org/uproxy/tun2socks/DnsResolverService.java, line 91 [r4] (raw file):_

Previously, albertolalama (alberto lalama) wrote… > Done. As per our discussion, setting the size to 64K. >

Nit: This is not related to Google DNS. The DNS TCP protocol only has two bytes for length, so larger packets are cannot even be represented.

_Comments from Reviewable_

alalamav commented 8 years ago

Review status: 0 of 18 files reviewed at latest revision, 2 unresolved discussions.

_src/android/org/uproxy/tun2socks/DnsResolverService.java, line 91 [r4] (raw file):_

Previously, bemasc (Benjamin M. Schwartz) wrote… > Nit: This is not related to Google DNS. The DNS TCP protocol only has two bytes for length, so larger packets are cannot even be represented. >

Done.

Comments from Reviewable