Opened 7 years ago

Closed 5 years ago

#8117 closed defect (fixed)

Tor SOCKS handshake makes SOCKS circuit isolation non-functional for many apps

Reported by: cypherpunks Owned by:
Priority: High Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version: Tor: 0.2.3.25
Severity: Keywords: tor-client isolation 023-backport
Cc: ioerror, weasel Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Tor 0.2.3 is supposed to have SOCKS username+password isolation on by default. But with Pidgin and other apps, vidalia still shows circuits being shared between multiple apps using different SOCKS usernames and passwords.

I dug in with Wireshark, and it looks like the problem for Pidgin is that its SOCKS client handshake lists 2 "Client Authorization Methods": "No authentication" and "Username/password". Tor's SOCKS port replies that it only supports "No Authentication", so Pidgin doesn't send the username and password at all!

Tor should reply that it supports "Username/password" in this case if the SOCKS isolation feature is enabled.

Child Tickets

Attachments (1)

8117_023_v2.patch (1.6 KB) - added by nickm 7 years ago.
patch for 8117 against maint-0.2.3. needs testing

Download all attachments as: .zip

Change History (29)

comment:1 Changed 7 years ago by nickm

Keywords: tor-client isolation added
Status: newneeds_review

Agreed wrt priority and backportability.

It looks easy enough to fix at first glance: answer "username/password" if the client offers it; otherwise answer "no auth". I'm attaching a patch to do that.

I'm a little worried that there could be a failure mode here where a user's application offers username/password authentication even though it doesn't know a username/password combination, and then responds to Tor's selecting username/password authentication by asking the user for a username and password. If there are many apps like that, we'll need another fix here.

This patch needs testing: first to ensure that username/password isolation is working with programs that behave like pidgin. And second, to make sure that the failure mode above doesn't occur when no username and password are configured.

Changed 7 years ago by nickm

Attachment: 8117_023_v2.patch added

patch for 8117 against maint-0.2.3. needs testing

comment:2 Changed 7 years ago by nickm

Okay, I've attached the patch. Can anyone test it out with some applications? If you want to try it without wireshark, you can run tor with debug-level logs and look for the messages "socks5: accepted method 0 (no authentication)" and "socks5: accepted method 2 (username/password)".

comment:3 Changed 7 years ago by cypherpunks

The patch works properly on Tor's side. Wireshark reports Tor is now sending username/password in the response, and Pidgin sends the username and password, and then Tor says "success". Unfortunately, Pidgin immediately closes the SOCKS connection and then times out, seemingly forgetting what happened.

Other apps have similar issues. At least one began blasting data at the SOCKS port as soon as Tor said "username/password", which causes a warn about an improper SOCKS username/password version from Tor. Apparently it forgot that it still needed to actually send one as configured! What a mess.

Nothing has yet asked me for a password if I didn't provide one, though.

comment:4 Changed 7 years ago by nickm

So, it sounds like the bug here is in pidgin and the other app (what was it?), not necessarily in Tor now. Or maybe Tor is screwing up here somehow, but not in a way wireshark can notice?

What would happen if Tor just stops saying that NO_AUTH is okay? That's not something we could do by default, but it could be a socksport-specific option.

comment:5 Changed 7 years ago by nickm

So, I really want to do this change (if it makes a difference) but so far it sounds like that no tested applications are actually fixed by this fix so far. We should figure out:

  • what apps we need to test
  • whether this fix actually fixes any of them
  • whether this fix actually breaks any of them.

Any takers there?

comment:6 Changed 7 years ago by mikeperry

Cc: ioerror added

My opinion here is that it's actually good to break apps that are doing SOCKS u+p wrong, so long as we don't also break those apps when SOCKS u+p is not set. We really shouldn't tell users "Hey, use SOCKS u+p to isolate your apps!" and then not actually isolate anything because our handshake silently tells the app not to use u+p.

Also, if there are actually bugs in Pidgin/whatever's SOCKS u+p support, it's unlikely those will ever get fixed if our handshake always tells Pidgin not to use SOCKS u+p.

Adding ioerror to Cc because I think he's worked improving on Pidgin's SOCKS implementation for Tor in the past.

comment:7 Changed 7 years ago by ioerror

I would say that we should ensure that torsocks has isolated circuits/streams at all times. Pidgin should also have a bug opened about this issue, has anyone opened an issue or can someone detail their test case? I'll repro and file a bug if someone who best understands it details their experience...

comment:8 Changed 7 years ago by sysrqb

I was working on #8053 and noticed Tor was preferring NoAuth over u+p, luckily asn pointed me here. Both Tor and Torsocks perform as expected with patched Tor and the byte stream doesn't appear to have any artifacts or oddities, so I think this patch is good to go.

comment:9 Changed 7 years ago by nickm

Keywords: 023-backport added
Milestone: Tor: 0.2.3.x-finalTor: 0.2.4.x-final

(Let's try this in 0.2.4 first, then backport. Anything else we can test it on? Right now, I am seeing exactly two test reports above. I have no idea what it might break.)

comment:10 in reply to:  9 Changed 7 years ago by sysrqb

Replying to nickm:

(Let's try this in 0.2.4 first, then backport. Anything else we can test it on? Right now, I am seeing exactly two test reports above. I have no idea what it might break.)

I agree. It's sad that this may break some apps but it is a reality, so a testing phase will be good. I'll also check a few other popular apps and get patches rolling if necessary. If someone can provide steps to reproduce the pidgin failure for ioerror that'd be awesome, too.

comment:11 Changed 7 years ago by nickm

To be clear: I don't think we should merge this if it will break things that work today for a substantial number of users. I've tried the "break the nonconformant apps, and write patches as needed" approach to making only-nonfonformant-apps-will-break changes in the past, and it just mades for a huge pile of pain.

If this patch breaks something significant, the right fix is IMO to have it be enabled or disabled on a per-SOCKS-listener basis, so that if you need to run any applications that can't handle this fix, you can point them at a SOCKS port that doesn't have this fix turned on. It'll be more code, but potentially far less effort than upstreaming a big pile of fixes and/or upsetting a big mass of users.

comment:12 Changed 7 years ago by andrea

Status: needs_reviewneeds_revision

This patch looks fine to me, but due to potential app-breakage concerns I concur with nickm's comments about it and I think we should not merge this without being able to support multiple SOCKS listeners with this differently configured on each.

comment:13 Changed 7 years ago by nickm

Okay, will fix.

comment:14 Changed 7 years ago by nickm

Status: needs_revisionneeds_review

Branch "bug8117_023" in my public repository: it slices; it dices; it could use some review and testing.

comment:15 Changed 7 years ago by andrea

This looks fine to me now. I haven't tested it, though.

comment:16 Changed 7 years ago by sysrqb

This works well, at least for a non-broken app. I was initially uncertain about Tor only respecting socks_prefer_no_auth if NoAuth was send by the client. I'm wonder if there are any commonly used and Socks5-broken apps that say they only support u+p if they are configured with a username and password. If this was the case, then Tor would still reply to use u+p because NoAuth was not a client-supported method. There's not much Tor can do in this case, though, if the app doesn't know how to handle the response.

On a different note, how do you feel about creating isolated circuits for connections from apps that said they support u+p but Tor responded with NoAuth because PreferSOCKSNoAuth was set?

comment:17 Changed 7 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.3.x-final

Merged to 0.2.4; noting for possible backport to 0.2.3

comment:18 Changed 7 years ago by arma

Cc: weasel added

The backport here looks pretty big, and pretty feature-like (even if we thought the feature was already present). So I'm inclined to say we should skip it for 0.2.3.

That said, this is a good one to get weasel's opinion on to confirm: weasel, what would you think of trying to talk your Debian people into taking this fix in whatever debian ships 0.2.3 currently?

comment:19 Changed 7 years ago by weasel

Debian wheezy will, at least initially, ship with Tor 0.2.3.25 unless something really huge shows up yesterday.

The patch isn't actually that long, so I'm less owrried about patch size. What I don't like about this is that it's changing Tor's default behavior in what is then the stable tree (of both Tor and Debian). If this was forced onto us as part of a 0.2.3.26 that had lots of things we wanted we'd probably take it however.

comment:20 Changed 7 years ago by arma

I'm going to call it feature-like then, and suggest no backport.

comment:21 Changed 6 years ago by nickm

I'd be okay with that.

comment:22 Changed 6 years ago by mikeperry

Status: needs_reviewneeds_revision

If we're going to leave this broken in 0.2.3.x, please remove IsolateSOCKSAuth from the manpage, and deprecate the torrc option in config.c, so we tell users who try to use it that it's not supported and Tor exits if they try to use it.

It's really bad form to let people think they're getting a security bonus by using an option we decided to let remain silently broken..

comment:23 in reply to:  6 ; Changed 6 years ago by arma

Replying to mikeperry:

My opinion here is that it's actually good to break apps that are doing SOCKS u+p wrong, so long as we don't also break those apps when SOCKS u+p is not set. We really shouldn't tell users "Hey, use SOCKS u+p to isolate your apps!" and then not actually isolate anything because our handshake silently tells the app not to use u+p.

Turns out pidgin offers the username of "" and password of "", when Tor takes pidgin up on its offer to provide u+p. That is, pidgin says it knows how to provide u+p auth, and Tor says yes please, even when the Username and Password boxes in the pidgin UI are blank.

So, pidgin doesn't work at all with Tor 0.2.4.12-alpha. Filed as #8879.

comment:24 in reply to:  23 Changed 6 years ago by arma

Replying to arma:

So, pidgin doesn't work at all with Tor 0.2.4.12-alpha. Filed as #8879.

I now believe this (bug #8879) is Tor's fault, not pidgin's fault.

comment:25 Changed 6 years ago by nickm

username "", password "" is a valid username and password as far as we're concerned.

comment:26 Changed 6 years ago by arma

What's the status here? Did we merge anything into, well, anything? If not maybe we should get it into 0.2.5 pretty soon so people have a chance to learn whether it's broken? (Mike has a great point with the "users think it's working but it's not" comment.)

comment:27 Changed 6 years ago by nickm

The commits in nickm/bug8117_023 were a264c4fedab87ce7c8cbb94632657a90e95e7a4e and fa3c23773944788125db2f67bcb048376c17fec9. They got merged in Tor 0.2.4.12-alpha. Their changes entry was:

+
+    - Many SOCKS5 clients, when configured to offer a username/password,
+      offer both username/password authentication and "no authentication".
+      Tor had previously preferred no authentication, but this was
+      problematic when trying to make applications get proper stream
+      isolation with IsolateSOCKSAuth. Now, on any SOCKS port with
+      IsolateSOCKSAuth turned on (which is the default), Tor selects
+      username/password authentication if it's offered. If this confuses your
+      application, you can disable it on a per-SOCKSPort basis via
+      PreferSOCKSNoAuth. Fixes bug 8117; bugfix on 0.2.3.3-alpha.

I would be glad to take a documentation fix in 0.2.3. Anybody want to write one?

comment:28 Changed 5 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: 0.2.4.x-final
Resolution: fixed
Status: needs_revisionclosed

Marking a batch of tickets that had been under consideration for 0.2.3 backport as fixed-in-0.2.4. There is no plan for further 0.2.3 releases.

Note: See TracTickets for help on using tickets.