Opened 7 years ago

Closed 4 years ago

#7582 closed defect (fixed)

Don't disable exits so harshly for unexpected END_REASON_EXITPOLICY

Reported by: nickm Owned by:
Priority: High Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client 023-backport
Cc: weasel Actual Points:
Parent ID: #5456 Points:
Reviewer: Sponsor:

Description

See Roger's tor-dev message at https://lists.torproject.org/pipermail/tor-dev/2012-November/004179.html and ensuing discussion.

It's wrong to completely disable an exit for the crime of having exceptions to its policy summary, since that's just a summary.

Child Tickets

Change History (17)

comment:1 Changed 7 years ago by nickm

Status: newneeds_review

I've written a sketchy fix in branch "bug7582". Instead of disabling an exit permanently, it disables it only for 20 seconds if we were only "pretty sure" that the exit should have supported the connection.

This isn't a great + permanent fix, but it's likely better than nothing, and comparatively simple as things for 0.2.3 might go; please review? We can also consider more elaborate fixes for 0.2.4 and later

comment:2 Changed 7 years ago by mikeperry

Parent ID: #5456

comment:3 Changed 7 years ago by nickm

Status: needs_reviewneeds_revision

Roger had a good idea at https://lists.torproject.org/pipermail/tor-dev/2012-December/004280.html :

I had this same thought while rereading my earlier message: just prepend a reject rule for this ip:port to our local version of the relay's exit policy.

It does let the exit "tag" you with an IP:port combo that you'll never come back to it with. But that seems a small risk compared to the risk of an exit relay with a complex enough policy that it causes clients to spend two circuits for fetching each component of web pages.

comment:4 in reply to:  3 ; Changed 7 years ago by rransom

Replying to nickm:

Roger had a good idea at https://lists.torproject.org/pipermail/tor-dev/2012-December/004280.html :

I had this same thought while rereading my earlier message: just prepend a reject rule for this ip:port to our local version of the relay's exit policy.

It does let the exit "tag" you with an IP:port combo that you'll never come back to it with. But that seems a small risk compared to the risk of an exit relay with a complex enough policy that it causes clients to spend two circuits for fetching each component of web pages.

It's not such a small risk for an IPv6-capable web-browsing client.

How hard would it be to put that ‘tag’ on the circuit's data structure instead of the relay's?

comment:5 in reply to:  4 Changed 7 years ago by rransom

Replying to rransom:

It's not such a small risk for an IPv6-capable web-browsing client.

Or for any client -- there are quite a lot of port numbers (although not quite 216) usable for tagging on each IPv4 address.

comment:6 Changed 7 years ago by nickm

How hard would it be to put that ‘tag’ on the circuit's data structure instead of the relay's?

Not all that hard honestly, if you do it as an addr_policy_t in origin_circuit_t.

comment:7 Changed 6 years ago by nickm

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

comment:8 Changed 6 years ago by nickm

Status: needs_revisionneeds_review

Okay, here's an implementation : branch bug7582_v2 in my public repository.

It's done against 0.2.4, though the backport to 0.2.3 shouldn't be screamingly hard.

comment:9 in reply to:  8 Changed 6 years ago by andrea

Replying to nickm:

Okay, here's an implementation : branch bug7582_v2 in my public repository.

It's done against 0.2.4, though the backport to 0.2.3 shouldn't be screamingly hard.

This looks fine to me I think.

comment:10 Changed 6 years ago by nickm

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

Oop! I forgot to add an addr_policy_list_free() call in circuit_free(). Added that and merged. Thanks! Marking for possible 0.2.3 backport.

comment:11 Changed 6 years ago by arma

Cc: weasel added

On the one hand, I want to ignore this until 0.2.3 expires.

On the other hand, it's a real attack. We haven't seen it in the wild, but it's the sort of targetable attack that we wouldn't necessarily see.

I bet if we get it a CVE, weasel will be able to convince his peers.

(Weasel, can you confirm/deny?)

Also, how big is the 0.2.3 patch? Actually, *is* there an 0.2.3 patch yet?

comment:12 Changed 6 years ago by nickm

I did the most straightforward backport possible as bug7582_023.

It could probably be smaller, but I thought that just taking the commits from 0.2.4 would have the advantage of giving users seomthing more tested. fixup commits to make it smaller would be fine.

comment:13 Changed 6 years ago by weasel

If you say this is a significant enough security problem that we need to update the 0.2.3.x tree then merging it makes sense.

Depending on the severity of the issue we either can try to get the next 0.2.3.x release into the first wheezy point release (julyishish?) or we can go the Debian security route (though that appears unwarranted from what my impression of this is so far).

comment:14 Changed 6 years ago by arma

Man. That's a lot of code. Maybe we should regard 0.2.4 as having the feature that it's safer than 0.2.3.

comment:15 Changed 6 years ago by nickm

I dunno; the issue in question is pretty obnoxious, and if Debian users are going to be having the 0.2.3 client indefinitely, I'm not comfortable calling a client version "supported" if we're not going to fix this kind of issue.

Can we consider a simpler fix or something?

comment:16 Changed 6 years ago by andrea

I think this patch looks fine to me as long as it's been tested to a reasonable extent. If 0.2.3 is going to be around that much longer, then yeah, merge.

comment:17 Changed 4 years ago by nickm

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

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.