Opened 8 years ago

Closed 8 years ago

Last modified 6 years ago

#3000 closed defect (fixed)

Clients do not clear HS descriptor cache on SIGNAL NEWNYM

Reported by: rransom Owned by: rransom
Priority: High Milestone:
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Currently, Tor clients do not clear their hidden service descriptor cache when they process a SIGNAL NEWNYM control port command. Thus, anyone who can ‘tag’ a client with a unique HS descriptor, and then test for the presence of that descriptor later, can track the client until the descriptor expires up to 72 hours later or until it is stopped and restarted, whichever happens first. We should fix this.

Child Tickets

Change History (23)

comment:1 Changed 8 years ago by nickm

Seems like an easy fix; are you working on a patch here?

comment:2 in reply to:  1 ; Changed 8 years ago by rransom

Status: newneeds_review

Replying to nickm:

Seems like an easy fix; are you working on a patch here?

Writing a fix for this took most of the last week. Making NEWNYM nuke the HS descriptor cache was trivial; making it cancel whatever HS descriptor fetches are in progress required only a little thought. Cleaning up the mess that nuking the descriptor cache made was a PITA.

See bug3000-021 ( ssh://mob@repo.or.cz/srv/git/tor/rransom.git bug3000-021 ) for a branch on maint-0.2.1; see bug3000-022 ( ssh://mob@repo.or.cz/srv/git/tor/rransom.git bug3000-022 ) for a branch on maint-0.2.2. The merges were non-trivial, and need to be reviewed carefully.

I also put a fix for #1930 (also known as #1024) on this branch, mainly because I didn't figure out that nuking the HS descriptor cache more often wouldn't cause #1930 to occur more often until after I had written that fix.

This branch will need to be merged to maint-0.2.1.

comment:3 Changed 8 years ago by nickm

Reviewing bug3000-021, it looks superfically okay, meaning that if there are any issues, it's not issues in the code but rather in the code's ramifications in other parts of Tor. We should meditate on that.

Already noted to you in IRC: on e05cdf9b8dfea30 , I think you need a "return -1" after tor_fragile_assert(): tor_fragile_assert() can be defined to not assert at all, in which case you'd run off the bottom of the function.

What have you done to test this out so far?

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

Replying to nickm:

Reviewing bug3000-021, it looks superfically okay, meaning that if there are any issues, it's not issues in the code but rather in the code's ramifications in other parts of Tor. We should meditate on that.

Already noted to you in IRC: on e05cdf9b8dfea30 , I think you need a "return -1" after tor_fragile_assert(): tor_fragile_assert() can be defined to not assert at all, in which case you'd run off the bottom of the function.

What have you done to test this out so far?

I compiled and ran the tree as of ‘Ignore SIGNAL NEWNYM on relay-only Tor instances’ in order to make sure I didn't get the conditionals in that patch or ‘Don't allow v0 HS auths to act as clients’ backwards, and then send SIGNAL NEWNYM while trying to connect to the archive.tpo HS to see what I broke by nuking the HS descriptor cache.

Then, many changes later, I tested the tree as of ‘Fix bug 1930’ by repeatedly sending SIGNAL NEWNYM while repeatedly trying to connect to the archive.tpo HS again. Nothing broke, and I was eventually able to view the HS with Firefox 3.6 without Polipo or the Firefox SOCKS timeout patch. I am still running that build as my Tor client.

Then, to test the merges, I tried compiling the source files after performing each merge. (I was unable to link Tor on the maint-0.2.2 branches due to idiocy in the FreeBSD libevent2 port.)

comment:5 Changed 8 years ago by Sebastian

Should we add comment tweaks for 0.2.1.x? It seems that the security team of debian for example has a hard time with large diffs when deciding what to do about an update, so just fixing the bug might be nicer? There's still a big diff left

comment:6 Changed 8 years ago by nickm

Strange. When I merge your bug3000-022 onto maint-0.2.2, it doesn't compile. I may be doing this wrong.

The issue seems to be that when I merge your branch onto maint-0.2.2, I get something that calls rend_client_fetch_v0_renddesc, which no longer exists in 0.2.2. I might be looking at this wrong.

Other than that, I get results from my merge that are close enough to those from your merge that I basically trust your merge.

I could take or leave the comment tweaks for 0.2.1.x; I would really like 0.2.1.x patches to remain Serious Bugfixes Only at this point.

comment:7 Changed 8 years ago by Sebastian

the merge commit fc170cd6b96 is causing trouble. It adds functionality back that was supposed to be removed in 0.2.2 (see the hunk that adds rend_client_fetch_renddescs() back in)

comment:8 Changed 8 years ago by Sebastian

Why are v0 hsdir auths not allowed to run a client?

comment:9 in reply to:  8 Changed 8 years ago by Sebastian

Replying to Sebastian:

Why are v0 hsdir auths not allowed to run a client?

nickm| Sebastian: because if you ever give them a newnym, they'll clear their cache.

comment:10 in reply to:  2 Changed 8 years ago by rransom

Replying to rransom:

The merges were non-trivial, and need to be reviewed carefully.

Note to self: Do not ever write ‘non-trivial hand merge’ in a Git merge commit message. The only sane way to do a non-trivial merge is to cherry-pick the changes over so that people can review them properly (rather than letting git diff --cc hide the newly introduced rend_client_fetch_renddescs function so I don't notice that it needs to be updated for 0.2.2), then do a keep-ours merge.

I also put a fix for #1930 (also known as #1024) on this branch, mainly because I didn't figure out that nuking the HS descriptor cache more often wouldn't cause #1930 to occur more often until after I had written that fix.

I am no longer convinced that clearing the HS descriptor cache on NEWNYM will not make #1930 occur more often.

comment:11 Changed 8 years ago by Sebastian

arma and i worked on a minimal branch for 0.2.1. it's in arma's repository in the bug3k_021 branch.

comment:12 Changed 8 years ago by Sebastian

pushed an update to fix check-spaces complaint. See bug3k_021 in my repository.

comment:13 Changed 8 years ago by Sebastian

bug3k_021_squashed in my repository has the squashed bug3k_021 branch, so I can start the merge. rransom has verified that he likes bug3k_021 on IRC.

comment:14 Changed 8 years ago by Sebastian

I did a merge of bug3k_021 into maint-0.2.2. The result is in bug3k_022.

comment:15 Changed 8 years ago by rransom

I pushed a cleanup patch for the merge to bug3k_022 ( ssh://mob@repo.or.cz/srv/git/tor/rransom.git bug3k_022 ); there were only two minor issues (not quite problems) with the merge.

comment:16 Changed 8 years ago by Sebastian

I believe sebastian/bug3k_021_squashed should be ready to go into origin/maint-0.2.1. rransom/bug3k_022 should get the fix squashed into the merge, and then I think it can be merged into origin/maint-0.2.2.

comment:17 Changed 8 years ago by nickm

ok; merged those onto 0.2.1 and 0.2.2 and master.

The remaining issues from the original bug3000 branch should get picked out and merged onto 0.2.2.

comment:18 in reply to:  17 Changed 8 years ago by rransom

Replying to nickm:

ok; merged those onto 0.2.1 and 0.2.2 and master.

The remaining issues from the original bug3000 branch should get picked out and merged onto 0.2.2.

I've cherry-picked the most important remaining commits onto bug3000-022-part2 ( ssh://mob@repo.or.cz/srv/git/tor/rransom.git bug3000-022-part2 ). I've left out the comment-cleanup and refactoring commits for now, as well as 3a70d5ca232ca7302e07d (Adjust circuits' states in several calls to rend_client_fetch_renddescs), which depended heavily on the refactoring commits. This branch still needs to be tested (I can't currently compile it on my main workstation).

comment:19 Changed 8 years ago by nickm

Merging that. Thanks!

comment:20 Changed 8 years ago by rransom

Resolution: fixed
Status: needs_reviewclosed

comment:21 Changed 7 years ago by nickm

Keywords: tor-client added

comment:22 Changed 7 years ago by nickm

Component: Tor ClientTor

comment:23 Changed 6 years ago by nickm

Milestone: Tor: 0.2.1.x-final

Milestone Tor: 0.2.1.x-final deleted

Note: See TracTickets for help on using tickets.