Opened 5 years ago

Closed 3 years ago

#12535 closed defect (fixed)

goptlib should expose a SOCKS5 server instead of SOCKS4a.

Reported by: yawning Owned by: yawning
Priority: Medium Milestone:
Component: Circumvention/Pluggable transport Version:
Severity: Normal Keywords: goptlib, socks
Cc: dcf, gk, isis Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Title says it all.

As far as I am aware goptlib based PTs are the last thing that remains that uses SOCKS4(a), and this is a nice to have for IPv6 and eventual Prop. 229 support.

Child Tickets

Attachments (4)

goptlib-socks5-draft.diff (14.7 KB) - added by yawning 5 years ago.
First pass implementation (needs tests).
goptlib-socks5-draft2.diff (18.4 KB) - added by yawning 5 years ago.
Take 2 at the implementation.
goptlib-socks5-draft3.diff (18.2 KB) - added by yawning 5 years ago.
Take 3, with some bugs fixed.
goptlib-socks5-draft4.diff (35.3 KB) - added by yawning 5 years ago.
Now with unit tests (stolen from obfsproxy), and minor bugfixes.

Download all attachments as: .zip

Change History (31)

Changed 5 years ago by yawning

Attachment: goptlib-socks5-draft.diff added

First pass implementation (needs tests).

Changed 5 years ago by yawning

Attachment: goptlib-socks5-draft2.diff added

Take 2 at the implementation.

comment:1 Changed 5 years ago by yawning

Ok, this still needs tests but it resolves most of the things in first revision that I was unhappy about by using the magic of goroutines and channels.

Now, each SocksListener has a goroutine that handles accepting new connections, and dispatches another goroutine that does the SOCKS handshake and places the SocksConn into a channel. Accept consumes off the channel instead (so can't fail unless trying to Accept on a SocksListener that has been closed). Error handling is done entirely by the goroutine(s).

This also fixes a bug inherited from the original code. If either of the SetDeadline calls in AcceptSocks fail (unlikely), the file descriptor will be leaked (Unless the GC will happen to take care of closing them).

Eratta:
Line 136 should be ln.wg.Wait().

I'm not sure if I should be using a mutex + waitgroup to make the close process race condition free or if there's a more idiomatic channel based way to do this.

Last edited 5 years ago by yawning (previous) (diff)

comment:2 Changed 5 years ago by yawning

Parent ID: #12130

I really want this complete before obfs4proxy sees use by the general public, so adding the obfs4 deployment bug as a parent so I don't forget.

Changed 5 years ago by yawning

Attachment: goptlib-socks5-draft3.diff added

Take 3, with some bugs fixed.

comment:3 Changed 5 years ago by yawning

The previous versions had some brain damage that would cause a deadlock on Close(), that I fixed because I've been using this code for my obfs4 testing. Seems to work ok, once I'm done with the obfs4 refactor I'll add unit tests to this. Sorry for the delay.

Changed 5 years ago by yawning

Attachment: goptlib-socks5-draft4.diff added

Now with unit tests (stolen from obfsproxy), and minor bugfixes.

comment:4 Changed 5 years ago by yawning

Status: newneeds_review

Ok, I cribbed the obfsproxy socks5 unit tests and fixed some minor bugs and cleaned up behavior in a few places. The SOCKS5 protocol handling code should have reasonable coverage now, though the goroutine/channel related goo does not (not sure what the best way to test that sort of thing is).

comment:5 in reply to:  1 Changed 5 years ago by dcf

Replying to yawning:

This also fixes a bug inherited from the original code. If either of the SetDeadline calls in AcceptSocks fail (unlikely), the file descriptor will be leaked (Unless the GC will happen to take care of closing them).

Could you fix this bug in a separate commit? (Just go ahead and commit it.) It's important to me to keep bug fixes separate from new features. This way the bug ends up getting recorded in the commit history too.

comment:6 Changed 5 years ago by dcf

I'm taking a look at this patch now. Do you have have a branch that's factored into separate commits? I don't find it in your github nor in git.torproject.org/user/yawning.

comment:7 Changed 5 years ago by dcf

Status: needs_reviewneeds_revision

Nice patch. It suffers somewhat from a lack of separation of concerns—not everything is directly related to SOCKS5 support. The biggest thing is the handling of concurrent connections, which is a new feature, and should be committed separately, if at all. Apart from that I just have some minor questions. I haven't looked at the connection logic yet nor the tests. See my comments below.

Please avoid rearranging function order and adding blank lines to existing functions. It's fine to make those kinds of changes directly on the trunk; they shouldn't show up in a feature branch.

 // The package implements a SOCKS4a server sufficient for a Tor client transport
 // plugin.
 //
 // http://ftp.icm.edu.pl/packages/socks/socks4/SOCKS4.protocol

Put a URL to good SOCKS5 documentation in pt.go. I guess RFC 1928.

+	socksVersionString = "socks5"
...
+// Returns "socks5", suitable to be included in a call to Cmethod.
+func (ln *SocksListener) Version() string {
+	return socksVersionString
+}

Let's remove socksVersionString and put the magic string inside the function.

+	// the password string sent by the client.
+	Password string

Capitalize the comment.

 // Send a message to the proxy client that access to the given address is
-// granted. If the IP field inside addr is not an IPv4 address, the IP portion
-// of the response will be four zero bytes.
+// granted.  For interface backwards compatibility reasons, this does not set
+// BND.ADDR/BND.PORT correctly, however very few if any clients examine the
+// values of this field.
 func (conn *SocksConn) Grant(addr *net.TCPAddr) error {
-	return sendSocks4aResponseGranted(conn, addr)
+	// Addr in the SOCKS 4 code was the destination address, which is not sent
+	// in SOCKS 5.
+	return sendSocks5ResponseGranted(conn, nil)
 }

I don't understand why addr is ignored? The comments don't make sense to me; isn't BND.ADDR/BND.PORT the destination address?

-// Send a message to the proxy client that access was rejected or failed.
+// Send a message to the proxy client that access was rejected or failed.  This
+// method exists for backwards compatibility and will only send the "General
+// Failure" error code.
 func (conn *SocksConn) Reject() error {
-	return sendSocks4aResponseRejected(conn)
+	return conn.RejectReason(SocksRepGeneralFailure)
+}
+
+// Send a message to the proxy client that access was rejected with the
+// specified reason.
+func (conn *SocksConn) RejectReason(reason byte) error {
+	return sendSocks5ResponseRejected(conn, reason)
 }

Adding a RejectReason function is fine. It does tie us to SOCKS5 forever, but realistically we're not ever likely to change from SOCKS5. Don't characterize plain Reject as being for backward compatibility only; just document that it sends a general error; I don't think people should be required to use a specific code if they don't care.

Here's related reading on the whole net.Error/err.Temporary thing (i.e. "Does this happen ever?").
https://gitweb.torproject.org/pluggable-transports/goptlib.git/commitdiff/3030f080eecf72b0e896236fca5fabd245c00bdb

The most troubling part of the patch is the new complex code surrounding AcceptSocks. First, thank you for taking the time to explore different ways of handling concurrent connections in a non-blocking way. However, if you leave that part out, the SOCKS5 code works just as well as the SOCKS4a code did, right? That is, it blocks a second connection while the first is still pending, but otherwise works correctly as long as SOCKS handshakes finish quickly? If so, then the new additional concurrency really needs to be in a separate patch. It's not intrinsically related to SOCKS5, and in fact you could make the same kind of enhancement to the SOCKS4a code as it stands.

So I'd like to ask you to remove the new concurrency code and thereby simplify the patch. Even then, we should spend some more time thinking about the best way to implement the concurrency feature.

 type SocksListener struct {
 	net.Listener
+	sync.Mutex
+
+	isClosed bool
+	ch       chan *SocksConn
+	wg       sync.WaitGroup
 }

A Mutex, a WaitGroup, a new boolean flag—there has to be a simpler way. If there's not a simpler way, maybe the new feature isn't worth it. Correctness is a virtue, but simplicity is also a virtue. In particular, you seem to be doing a lot of work to ensure that you can do a RejectReason to all concurrently connecting clients—I'm not sure that's worth the infrastructure required. We should aim for an implementation that does not require overloading the Close method in SocksListener.

comment:8 in reply to:  7 ; Changed 5 years ago by yawning

Replying to dcf:

Nice patch. It suffers somewhat from a lack of separation of concerns—not everything is directly related to SOCKS5 support. The biggest thing is the handling of concurrent connections, which is a new feature, and should be committed separately, if at all. Apart from that I just have some minor questions. I haven't looked at the connection logic yet nor the tests. See my comments below.

Snipping stuff that's "ok, will fix".

 // Send a message to the proxy client that access to the given address is
-// granted. If the IP field inside addr is not an IPv4 address, the IP portion
-// of the response will be four zero bytes.
+// granted.  For interface backwards compatibility reasons, this does not set
+// BND.ADDR/BND.PORT correctly, however very few if any clients examine the
+// values of this field.
 func (conn *SocksConn) Grant(addr *net.TCPAddr) error {
-	return sendSocks4aResponseGranted(conn, addr)
+	// Addr in the SOCKS 4 code was the destination address, which is not sent
+	// in SOCKS 5.
+	return sendSocks5ResponseGranted(conn, nil)
 }

I don't understand why addr is ignored? The comments don't make sense to me; isn't BND.ADDR/BND.PORT the destination address?

Despite what certain things say (eg: Wikipedia), it's the local address/port of the outgoing socket from the SOCKS server to the destination.

   In the reply to a CONNECT, BND.PORT contains the port number that the
   server assigned to connect to the target host, while BND.ADDR
   contains the associated IP address.  The supplied BND.ADDR is often
   different from the IP address that the client uses to reach the SOCKS
   server, since such servers are often multi-homed.

It's presumably done this way because clients can't call getsockname() on connections through a proxy, but in practice not many SOCKS clients require or use this information. We could go and modify the application code that is affected by the switch to send back the right address as well.

The most troubling part of the patch is the new complex code surrounding AcceptSocks. First, thank you for taking the time to explore different ways of handling concurrent connections in a non-blocking way. However, if you leave that part out, the SOCKS5 code works just as well as the SOCKS4a code did, right? That is, it blocks a second connection while the first is still pending, but otherwise works correctly as long as SOCKS handshakes finish quickly? If so, then the new additional concurrency really needs to be in a separate patch. It's not intrinsically related to SOCKS5, and in fact you could make the same kind of enhancement to the SOCKS4a code as it stands.

So I'd like to ask you to remove the new concurrency code and thereby simplify the patch. Even then, we should spend some more time thinking about the best way to implement the concurrency feature.

 type SocksListener struct {
 	net.Listener
+	sync.Mutex
+
+	isClosed bool
+	ch       chan *SocksConn
+	wg       sync.WaitGroup
 }

A Mutex, a WaitGroup, a new boolean flag—there has to be a simpler way. If there's not a simpler way, maybe the new feature isn't worth it. Correctness is a virtue, but simplicity is also a virtue. In particular, you seem to be doing a lot of work to ensure that you can do a RejectReason to all concurrently connecting clients—I'm not sure that's worth the infrastructure required. We should aim for an implementation that does not require overloading the Close method in SocksListener.

It's kind of complicated because on ln.Close(), the channel of pending connections needs to get flushed since there will be no further AcceptSocks() calls by the application code. The "clean" thing to do is to fail the pending requests gracefully, though rudely closing off all the connections that are waiting pending Accept() is also an option.

I'd be ok with removing the concurrency support, since the "incorrect" code will work well enough for our purposes (handshakes should get to the point where the command was received rather quickly).

comment:9 in reply to:  8 Changed 5 years ago by dcf

Replying to yawning:

Replying to dcf:

 // Send a message to the proxy client that access to the given address is
-// granted. If the IP field inside addr is not an IPv4 address, the IP portion
-// of the response will be four zero bytes.
+// granted.  For interface backwards compatibility reasons, this does not set
+// BND.ADDR/BND.PORT correctly, however very few if any clients examine the
+// values of this field.
 func (conn *SocksConn) Grant(addr *net.TCPAddr) error {
-	return sendSocks4aResponseGranted(conn, addr)
+	// Addr in the SOCKS 4 code was the destination address, which is not sent
+	// in SOCKS 5.
+	return sendSocks5ResponseGranted(conn, nil)
 }

I don't understand why addr is ignored? The comments don't make sense to me; isn't BND.ADDR/BND.PORT the destination address?

Despite what certain things say (eg: Wikipedia), it's the local address/port of the outgoing socket from the SOCKS server to the destination.

   In the reply to a CONNECT, BND.PORT contains the port number that the
   server assigned to connect to the target host, while BND.ADDR
   contains the associated IP address.  The supplied BND.ADDR is often
   different from the IP address that the client uses to reach the SOCKS
   server, since such servers are often multi-homed.

It's presumably done this way because clients can't call getsockname() on connections through a proxy, but in practice not many SOCKS clients require or use this information. We could go and modify the application code that is affected by the switch to send back the right address as well.

Oh fascinating. That does seem pretty useless for our purposes. I think you made the right call with your design. In the function comment, state explicitly that addr is ignored. I think I would remove the special handling for addr == nil in sendSocks5Response and push emptyAddr into the callers, just for the sake of explicitness. And in fact, it looks like you could just hard-code an IPv4 emptyAddr in sendSocks5Response and remove the addr parameter, as there doesn't seem to be any external way to cause a non-nil addr to be set. You can make the call.

comment:10 Changed 5 years ago by yawning

Status: needs_revisionneeds_review

I decoupled the concurrency changes from the SOCKS5 code and started using github to stage goptlib stuff.

https://github.com/Yawning/goptlib/compare/bug12535

Sorry for the changes around the function formerly know as readSocks4aConnect looking a bit tangled in the compare output, SOCKS5 processing is a bit more involved so there's more code (and the handshake process got split into multiple functions to reflect what SOCKS5 needs to do), and it confuses diff.

The tests were updated to account for no longer being able to send back responses with non-zero address/port (and my tests pass), and obfs4proxy works with my branch.

On a side note: I may be able to simplify the concurrency stuff a bit after thinking about it but if I ever decide to poke at that again, I'll file a separate ticket for it.

comment:11 Changed 5 years ago by yawning

Cc: gk added
Keywords: tbb-4.5-alpha TorBrowserTeam201503 added

Ok, this branch apparently has gotten a silly amount of testing because the psiphon people forked my github repo and have been using it for a while. Since we haven't merged it yet, I made this branch to apply it as a patch when we build Tor Browser. Ideally we can ship 4.5a5 with this, and get field testing on our own.

Would one of the kind tor browser people please pull this in? <3

https://github.com/Yawning/tor-browser-bundle/compare/bug12535

comment:12 Changed 5 years ago by mikeperry

Keywords: TorBrowserTeam201503R added; TorBrowserTeam201503 removed

comment:13 Changed 5 years ago by gk

Keywords: tbb-4.5-alpha TorBrowserTeam201503R removed

Looks good. Merged in commit 5e26066db070aa30d879a022237478a37bb5d944.

comment:14 Changed 5 years ago by gk

Keywords: tbb-4.5-alpha added

comment:15 Changed 5 years ago by gk

Status: needs_reviewassigned

comment:16 Changed 5 years ago by mikeperry

For some reason, this patch didn't cleanly apply when I tried to build with tor-browser-bundle.git tag tbb-4.5a5-build3. We reverted the commit to get the release out, but we'll still take this for 4.5-stable of course.

comment:17 Changed 5 years ago by gk

I think the problem is that applying to master works but not to 0.2.

comment:18 in reply to:  17 Changed 5 years ago by gk

Replying to gk:

I think the problem is that applying to master works but not to 0.2.

That's actually not true. I does not apply cleanly to master either. I wonder what I've been testing back then...

comment:19 Changed 5 years ago by yawning

Ugh, sorry about this, I blindly format-patched my feature branch without checking to see if would need changes, or what tag the alpha build was done with.

Option 1:

  • Fix whatever issues remain that's keeping this from being merged, merge it into goptlib, tag 0.5.
  • Update the versions file to pull in goptlib 0.5

Option 2:

  • I'll create a new patch for goptlib 0.4, update my branch so that it pulls in goptlib 0.4 and applies my newly created overlay.

Pick one. I can do either.

comment:20 Changed 5 years ago by gk

If you get done option 1 within, say, the next two weeks in order to get it into 4.5-stable then let's go that road as it seems cleaner to me. If not, option 2. And if that sucks just give us a working patch for a random option 3. :)

comment:21 Changed 5 years ago by yawning

Ok, here is an updated goptlib branch based off master (aka goptlib 0.4).

https://github.com/Yawning/goptlib/compare/bug12535_v2

It's dcf's call as to if this should be merged, if I don't hear shortly, I will go with option 2 and update my tor-browser-bundle patch to apply this branch as an overlay.

comment:22 Changed 5 years ago by mikeperry

Keywords: TorBrowserTeam201504 added

comment:23 Changed 5 years ago by yawning

Option 3:

I'm personally leaning toward this approach since I'm tired of this sitting in limbo, this fixes the problem from my perspective, and all copies of obfs4proxy will have IPv6 support regardless of what version of goptlib they are linked against (Though golang-goptlib-dev needs to be rebuilt anyway since it's lacking the workaround for #15240, I should bother the packager about that.).

comment:24 in reply to:  23 Changed 5 years ago by gk

Keywords: tbb-4.5-alpha TorBrowserTeam201504 removed

Replying to yawning:

Option 3:

That's done. Thus, I take this bug off of our 4.5 radar.

comment:25 Changed 4 years ago by isis

Cc: isis added
Parent ID: #12130

Removing #12130 as the parent ticket so that I can close #12130.

comment:26 Changed 3 years ago by dcf

Severity: Normal

I merged ​https://github.com/Yawning/goptlib/compare/bug12535_v2 at 93bcaf4c7d5b8e3dc29142ca50f96197639c38cf and tagged goptlib 0.6.

comment:27 Changed 3 years ago by dcf

Resolution: fixed
Status: assignedclosed

Somehow I never closed this ticket.

Note: See TracTickets for help on using tickets.