Opened 7 years ago

Closed 7 years ago

#6297 closed enhancement (fixed)

SOCKS issues with bitcoin in .2.3.18-rc vs .2.2.37

Reported by: jrmithdobbs Owned by:
Priority: Low Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version: Tor: 0.2.3.18-rc
Severity: Keywords: tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Support for propagating and selectively connecting through tor's socks proxy for onion addresses has recently been merged into Bitcoin.

The support as it stands right now works with .2.2.37 but the socks connect fails with 0x01 (general failure) using .2.3.18-rc.

I have pcaps of the working .2.2.37 functionality on an OS X 10.7 client using the browser bundle binaries and of the broken functionality on wheezy using the .2.3.18-rc wheezy packages.

In addition I have pcaps of connect.c (ProxyCommand for ssh/etc to use socks) going through the same onion addresses and working on both versions. I did this by running an sshd listening on the bitcoin port for these tests.

Everything in the handshake looks right to me except for the error response in .2.3.18-rc.

(The onion addresses in the dumps are public nodes.)

I tried adding NoIsolateSOCKSAuth and NoIsolateClientAddr to the SocksPort declaration but it didn't make any difference.

All that shows up in the .2.3.18-rc logs is the following (debug):

Jul 04 14:57:42.000 [warn] Socks version 0 not recognized. (Tor is not an http proxy.)
Jul 04 14:57:42.000 [warn] Fetching socks handshake failed. Closing.
Jul 04 14:57:43.000 [warn] Socks version 0 not recognized. (Tor is not an http proxy.)
Jul 04 14:57:43.000 [warn] Fetching socks handshake failed. Closing.

Child Tickets

Attachments (6)

tor-bitcoin-socks-handshake-issue.tbz (108.3 KB) - added by jrmithdobbs 7 years ago.
packet captures
tor-bitcoin-socks-handshake-issue.tbz.sha512sum.txt (168 bytes) - added by jrmithdobbs 7 years ago.
torbtc-8333-osx10.7.4-0.2.3.18.pcap.bz2 (2.9 KB) - added by jrmithdobbs 7 years ago.
.2.3.18-rc dump from os x with single onion for clarity
torbtc-8333-osx10.7.4-0.2.3.18.pcap.bz2.sha512sum.txt (170 bytes) - added by jrmithdobbs 7 years ago.
connect.png (148.1 KB) - added by jrmithdobbs 7 years ago.
connect packet
refuse.png (138.6 KB) - added by jrmithdobbs 7 years ago.
error 0x01

Download all attachments as: .zip

Change History (26)

Changed 7 years ago by jrmithdobbs

packet captures

Changed 7 years ago by jrmithdobbs

comment:1 Changed 7 years ago by nickm

I think there's a bug in your socks implementation. The first part of SOCKS5 is for you to send:

  05 (one byte, socks version)
  N  (one byte, number of authentications methods you support)
  M  (N bytes, authentication methods, one byte each.)

If I am reading these dumps right, then connect.c is sending [05 02 00 02] to say "This is socks 5, I support two authentication methods. One is 00 (no auth); one is 02 (password auth, I think)."

But if I am reading this dump right, your bitcoin tool is saying [05 01 00 00]. There's an extra byte there that shouldn't be.

(I could be reading the dump wrong, of course.)

It looks like 0.2.2.x would accept and discard an extra byte here; 0.2.3.x is stricter.

Can you confirm the analysis above? Right now it's not looking like a tor bug to me.

Changed 7 years ago by jrmithdobbs

.2.3.18-rc dump from os x with single onion for clarity

Changed 7 years ago by jrmithdobbs

Attachment: connect.png added

connect packet

Changed 7 years ago by jrmithdobbs

Attachment: refuse.png added

error 0x01

comment:2 Changed 7 years ago by jrmithdobbs

I'm not seeing anything extra if you look at that wireshark output (analyze-> decode as -> transport -> socks) they look like valid values to me.

comment:3 Changed 7 years ago by jrmithdobbs

Ok, I re-read what you were saying and did some checking on how that initial handshake packet was being generated. There was an OBO in the code sending an extra 0x00 (thanks std:string -> char array conversions) so it looks like there's a bug on both ends.

Will get the bitcoin issue fixed but for really should be throwing the general failure or some other error message after the first malformed packet not leaving the trailing end of it on the receive buffer and then parsing it as the beginning of the next?

Don't know how high a priority that is really but couldn't this be used for some form of abuse with some imagination?

comment:4 Changed 7 years ago by jrmithdobbs

Openssh's -D socks5 server impl may be useful reference as it throws a malformed response when that initial handshake packet has extra data.

comment:5 Changed 7 years ago by nickm

Status: newneeds_revision

I am still not seeing how it is a bug in Tor not to discard extra bytes in SOCKS; the bug was, I think, in 0.2.2.x, where we allowed nonconformat SOCKS implementations to work anyway if they sent their extra junk with the right timing.

What do you mean by "Throw general failure?" It sounds like you want some other error reporting that would have been more helpful?

comment:6 Changed 7 years ago by jrmithdobbs

No, you're definitely correct that the .2.2 behavior is bad. The "general failure" i'm referring to is the socks 0x01 error sent after parsing the connect request. (with 0x00 appended in the buffer)

I'm just a little concerned that appended data in the auth proposal is parsed at all.

For instance: Openssh appears to do extra checking during the socks handshake and will send an error if there's appended data in that auth negotiation packet. This makes sense to me as if you still have data after finishing parsing the auth proposal there's no valid reason for it as the protocol requires a response from the server before continuing.

Couldn't this have security implications under certain uses of SocksPort that aren't over loopback?

comment:7 Changed 7 years ago by jrmithdobbs

That should say: "(with 0x00 prepended in the buffer.)"

comment:8 Changed 7 years ago by nickm

Status: needs_revisionneeds_information

I wouldn't mind a more accurate "Hey, there's some extra data there" warning to help programmers track down bugs in their socks implementations.

I'm not sure what the security argument is, though: if an adversary can modify data send on your connection to your socks port, they can do worse than inject extra bytes, and they can do bad stuff without injecting extra bytes.

Does the RFC require the server to do any particular behavior in this case?

comment:9 Changed 7 years ago by jrmithdobbs

The rfc is very vague in this area. It pretty much skips over validation completely (outside of auth) as far as I can tell.

As far as the security, after actually looking at your parser I'm much less concerned about that. However, I still think there may be an issue regarding an attacker that has the ability to inject but not necessarily intercept the original message.

For example, because of how it's parsed so long as you can successfully get your packets accepted (there's been various recent talks/papers re: inadequacy of syn cookies as it relates to this) it could be feasible to force a user's connection to a specific username/password for the socks connection which may affect how the connection is isolated vs their other connections and may provide a way for an inject-capable (no observation of traffic between the client and socks port needed, really) to be able to associate a client's connections that should, by the configuration, be isolated.

Does that help make more sense of my concerns?

comment:10 Changed 7 years ago by jrmithdobbs

If you don't feel it's a plausible security concern just mark this as a feature enhancement. :)

comment:11 Changed 7 years ago by nickm

If the attacker can inject data into your socks connection, you are pretty sure to lose no matter how socks is parsed. For example, the attacker could insert a SOCKS connect request to a host under their control immediately before your actual request. Or the attacker could insert an IMG for a document hosted at a hostile URL into an HTTP response.

If you want to be secure, I think you truly need to keep hostile parties from messing with your TCP streams between your applications and your socks server.

comment:12 Changed 7 years ago by jrmithdobbs

That's true. Hadn't considered that.

With that in mind maybe there should just be a very big warning in the man page/related documentation about the risks of binding the SocksPort to something external clients have access to. (Maybe with a mention of SocksPolicy + *authenticated* ipsec transport connections or similar as a workaround?)

This isn't really a preventable scenario so long as it's non-loopback and there's any chance at all of a malicious user on the network.

comment:13 Changed 7 years ago by jrmithdobbs

Priority: normalminor
Type: defectenhancement

If gssapi was implemented (not currently, not really useful for 99.999999% of people anyways) my concerns would be a little more valid as the actual data stream gets authenticated after the auth negotiation.

Without gssapi I don't think there's a correctable issue besides more clear warnings.

comment:14 Changed 7 years ago by arma

See CL_PORT_WARN_NONLOCAL and warn_nonlocal_client_ports() in src/or/config.c

So, we already do warn. Maybe not loudly enough, or precisely enough, but I think we're 95% of the way there already.

comment:15 Changed 7 years ago by jrmithdobbs

Ya, now that you point it out I do notice those messages when running an instance with a remote-accessible socksport.

I think those warnings need to be copy/pasted into the man page sources, essentially.

comment:16 Changed 7 years ago by arma

Status: needs_informationneeds_revision

Fine by me

comment:17 Changed 7 years ago by nickm

Milestone: Tor: 0.2.4.x-final

comment:18 Changed 7 years ago by nickm

Keywords: tor-client added

comment:19 Changed 7 years ago by nickm

Component: Tor ClientTor

comment:20 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_revisionclosed

Added that to the manpage in 504d4aa8c61c172f2b73bcee00cb3cfb0e6fff07. Closing. Thanks!

Note: See TracTickets for help on using tickets.