Opened 6 years ago

Closed 5 years ago

#9221 closed defect (fixed)

obfsproxy does not have a SOCKS5 listener

Reported by: JBennett Owned by: asn
Priority: Medium Milestone:
Component: Obfuscation/Obfsproxy Version: Obfsproxy: 0.1.4
Severity: Keywords:
Cc: yawning Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

When using the latest git snapshot

While trying to pipe openvpn through obfsproxy, the socks5 handshake never finishes. It never gets past the last line here:

2013-07-07 14:12:11,585 [DEBUG] socks_fact_0x1fc27e8: New connection.
2013-07-07 14:12:11,585 [DEBUG] Starting obfs2 with shared secret: 2aBxIFS0t8NxhLpfw
2013-07-07 14:12:11,586 [DEBUG] socks_up_0x1fc1f90: Received SOCKS handshake data.

Using curl with the --socks5 option also hangs at the same place. When using socksv4 or 4a with curl, it completes perfectly.

Child Tickets

Attachments (1)

0001-Use-SOCKS5-instead-of-SOCKS4-a.patch (22.0 KB) - added by yawning 5 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 6 years ago by asn

Hey, the new Python obfsproxy does not support SOCKS5. It uses Twisted which only implements a SOCKS4 listener. If someone sends me a patch for a SOCKS5 listener, I will accept it. (I might also write one, when I get some time).

If the above answers your question, I will close this ticket.

(BTW, I think you leaked your shared-secret.)

comment:2 Changed 6 years ago by JBennett

Resolution: not a bug
Status: newclosed

That totally explains it. Unfortunate, though, because openvpn only supports socks5. Thanks for the answer.

It would be handy to have the ability to tunnel openvpn through obfsproxy. Certain countries restrict vpn access, and tunneling openvpn through obfsproxy could be a robust solution. I'm sure it would be possible to use an intermediary like ssh to make the tunneling work, but there would be a sizable performance penalty.

The leaked shared-secret was for testing only, not used in production.

comment:3 Changed 5 years ago by asn

Resolution: not a bug
Status: closedreopened
Summary: Socks5 proxy not workingobfsproxy does not have a SOCKS5 listener

Reopening this, since it would be nice to have it fixed eventually.

comment:4 Changed 5 years ago by asn

https://twistedmatrix.com/trac/ticket/1330 seems to be the relevant Twisted ticket.

There is some code there (from 4 years ago), but it doesn't have any tests and it's not merged to mainline Twisted yet.

comment:5 Changed 5 years ago by infinity0

Did you have a look at the txsocksx library? It's what I use for obfs-flash.

comment:6 in reply to:  5 Changed 5 years ago by asn

Replying to infinity0:

Did you have a look at the txsocksx library? It's what I use for obfs-flash.

txsocksx implements server-side SOCKS too? I thought it was just the client-side.
If it does, we should give it a try. AFAIK it is maintained by Twisted devs.

comment:7 Changed 5 years ago by yawning

Cc: yawning added

Bam.

https://github.com/Yawning/obfsproxy/commit/ebbe9e1167376278b9f4200c34c83f6400e494df

Things that it needs:

  • Someone should test IPv6, though it should work unless I screwed up
  • If there were any SOCKS unit tests, they're now totally broken
  • When the TCP connect fails outright, it should send the appropriate SOCKS5 error codes from the errBack.
  • Cleanups/Review. (Probably a bunch of style/idiom stuff, I'm not a python programmer)

Things that involve some design work:

  • In an ideal world, each transport would call SOCKSv5Protocol.send_reply(code) after the handshake is finished, instead of it being called from SOCKSv5Outgoing.connectionMade()

I was working under the assumption that something that was suitable for merging upstream would require monkey patching, so I didn't bother writing something that is easy to contribute back up. They've had the bug open for 8 years, they can write their own SOCKS5 server if they want one.

Changed 5 years ago by yawning

comment:8 Changed 5 years ago by yawning

Status: reopenedneeds_review

I spent some more time working on this today, and did most of what I had under "Things that it needs".

Please see the attached patch, which is my github branch squashed.

comment:9 Changed 5 years ago by yawning

Apart from a minor brain fart that I fixed in my github branch:

-                addr = socket.inet_ntop(socket.AF_INET6, msg[4:16])
+                addr = socket.inet_ntop(socket.AF_INET6, msg[4:20])

IPv6 appears to work. Bootstrapping stalls at 20% sometimes for longer than I would expect even with #9229, and I have no idea why (something very well may be broken), but that might be an artifact of "Still haven't bothered to get a tunnel, use the loopback address".

What I did on the client side in the torrc beyond pointing it at my patched obfsproxy:

Bridge obfs3 [::1]:52808

What I did on the bridge side:

ServerTransportListenAddr obfs3 [::1]:52808

comment:10 Changed 5 years ago by asn

Haven't thoroughly reviewed the code yet, but some comments from skimming it:

  • We will need to write unit tests. I would like good code coverage. I can definitely help write those if you want.
  • Instead of inlining struct.pack and struct.unpack, it might be better to define some higher-level functions (like get_uint16(), set_uint8(), etc.) in obfsproxy/common/serialize.py and use those instead. Might make the code more readable.
  • I wonder how much effort would be to submit this upstream. We would have to fit the Twisted API.

More review coming soon.

comment:11 Changed 5 years ago by asn

I reviewed up to 0583a45 More changes based on feedback. Code looks good, ready to merge when it gets a few unittests.

As a nitpick, why do you use this methodcaller thing? Surely the code can be easily changed to not require it, right?

Maybe some docs in __ByteBuffer on whether add_uint32() (etc.) does it in little-endian or big-endian (depending on htonl) would also be good.

Also, __ByteBuffer:get(self, len) uses len as the name of an argument, but len is a built-in identifier. btw, such small errors can be found easily with pylint.

comment:12 Changed 5 years ago by yawning

I changed the things you just pointed out, thanks. Is there any preferred way to write tests for this, or should I just look at how Twisted tests the SOCKSv4 code and do what they do?

comment:13 Changed 5 years ago by hellais

Here is a review of 0583a45.

## Proper error handling

You should trap the errors inside of handleCmdConnectFailure by doing:

failure.trap(error.NoRouteError, error.ConnectionRefusedError, etc.)

Failure to do so will lead to the exception being re-raised.

## sendReply function

I would put the code for constructing the reply corresponding to a successful
request inside of SOCKSv5Outgoing.connectionMade rather than making it
conditional inside of sendReply.

Something like:

`
def sendReply(self, reply, addr=0, port=0, atype=_SOCKS_ATYP_IP_V4):

msg = _ByteBuffer()
msg.add_uint8(_SOCKS_VERSION)
msg.add_uint8(reply)
msg.add_uint8(_SOCKS_RSV)
msg.add_uint8(atype)
msg.add(addr)
msg.add_uint16(port, True)
self.transport.write(str(msg))

`

## logging

It is advisable to not call log directly but to wrap the calls to
twisted.python.log by defining a log attribute inside of SOCKSv5Protocol.
Then replace log.msg() with self.log() and call log.msg() inside of self.log.
This makes it easier for a user of the protocol to disable logging.

Also it seems like self.logging is not used anywhere. What is the purpose of it?

## Possible bugs

I think at line 67 of socks5.py there is a typo. self.socks.send_reply should be self.socks.sendReply.

## Unittests

The code should have unittests if there are plans to have it merged upstream
into twisted. It's also probably a good idea to have them anyways.

## Class naming

It's general not advisable to have two classes have the same name. You should
probably change the names of the classes inside of socks.py to be
OBFSSOCKSv5Outgoing, OBFSSOCKSv5Protocol and OBFSSOCKSv5Factory or something
similar.

Apart from this it looks like pretty good code, good job :)

comment:14 Changed 5 years ago by yawning

Thanks, I fixed the things you pointed out, and pushed.

https://github.com/Yawning/obfsproxy/commit/75835331c6be8087314c6fa20311abf7be28e364

For the logging, I just used the global obfsproxy logger since at the moment I'm not particularly concerned about how easy it is to submit upstream (Sorry :( ).

Next on my todo list for this is the unit tests so it's easy to maintain.

comment:15 Changed 5 years ago by yawning

Not the best tests, but this is fairly comprehensive.

Places I skimped:

  • Only tested SOCKSv5Protocol, the Outgoing class is boilerplate and is mostly unchanged from the old SOCKSv4 code.
  • Only tested one of the exception to SOCKSv5 response mappings.
  • Didn't bother testing short writes where the server would need to buffer and wait for more data.

Probably good enough for now since it's a massive improvement over what exists (both the Twisted SOCKSv5 code and what was in obfsproxy).

https://github.com/Yawning/obfsproxy/commit/fe3a0b2820e8e68543ef1c7aa6f3ffa1d572fb9c

Let me know if there are any other changes I should make, if not, I'll make a cleaned up branch with the bulk of the commits I made actually implementing all this squashed down.

comment:16 Changed 5 years ago by asn

Nice!

Asa preliminary review, I noticed that the poor test_socks_args_splitting unit test is now failing because the split_socks_args() function has been renamed. On this note, you might also enjoy #10240.

comment:17 Changed 5 years ago by asn

OK, this looks ready to merge~!

BTW, some trailing white space seems to be lying around the tests.

Also, have you tested this on Windows and in managed mode with Tor?

comment:18 Changed 5 years ago by asn

During testing of the external mode I got:

  File "/home/doge/obfsproxy/obfsproxy/network/socks.py", line 86, in __init__
    self.ACCEPTABLE_CMDS.remove(socks5._SOCKS_CMD_BIND)
exceptions.ValueError: list.remove(x): x not in list

Seems like you remove the stuff once, so next time you have nothing to remove.

BTW, do you actually need to remove them dynamically? Maybe you can just strip them off ACCEPTABLE_CMDS entirely; we don't particularly care about implementing them anyway (and if we ever do, we can just add them to ACCEPTABLE_CMDS at that point).

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

comment:19 Changed 5 years ago by yawning

Fixed, went with removing them from ACCEPTABLE_CMDS. The observed behavior is the same since the commands in question were just stubbed out to return Command Not Supported anyway.

No to testing on Windows, dedicated video game box doesn't have a development environment on it, sorry. Works great with managed mode, that's primarily how I've been testing it.

Squashed branch at: https://github.com/Yawning/obfsproxy/tree/socks5-listener_squashed

comment:20 Changed 5 years ago by asn

Merged! Thanks!

Closing this! Let's open different tickets if any bugs are found.

comment:21 Changed 5 years ago by JBennett

This is great! It allows openvpn to use the obfsproxy framework, and should be of great help to a lot of people. Thank you guys for your work on this.

comment:22 Changed 5 years ago by asn

BTW, I can't close this ticket because it's the parent of #10412. Yawning, what should we do about this? Maybe detach #10412 from this one?

comment:23 Changed 5 years ago by asn

Resolution: fixed
Status: needs_reviewclosed
Note: See TracTickets for help on using tickets.