Opened 4 years ago
Closed 13 months ago
#13837 closed defect (implemented)
Mitigate guard discovery by pinning middle node
Reported by: | asn | Owned by: | mikeperry |
---|---|---|---|
Priority: | Medium | Milestone: | Tor: 0.3.3.x-final |
Component: | Core Tor/Tor | Version: | |
Severity: | Normal | Keywords: | tor-hs, tor-guard, guard-discovery-prop247-controller, review-group-30 |
Cc: | isis | Actual Points: | |
Parent ID: | #9001 | Points: | |
Reviewer: | asn | Sponsor: | SponsorV-can |
Description
Hello,
inspired by the recent discussions on guard discovery, I went ahead
and implemented a small patch for Tor that tries to help defend
against Hidden Service guard discovery attacks.
It basically allows the operator to specify a set of nodes that will
be pinned as middle nodes in Hidden Service rendezvous circuits. The
option only affects HS rendezvous circuits and nothing else.
Of course, it doesn't fix guard discovery, it just pushes guard
discovery to the next hop, so that they need to compromise two boxes
to win.
You can find my branch in 'sticky_mids' at
https://git.torproject.org/user/asn/tor.git .
(Here it is in HTTP shape:
https://gitweb.torproject.org/user/asn/tor.git/shortlog/refs/heads/sticky_mids )
[This is the trac version of https://lists.torproject.org/pipermail/tor-dev/2014-November/007730.html]
Child Tickets
Ticket | Status | Owner | Summary | Component |
---|---|---|---|---|
#23101 | closed | mikeperry | Predict and build specific HS purpose circuits (rather than GENERAL) | Core Tor/Tor |
Change History (40)
comment:1 Changed 4 years ago by
Milestone: | → Tor: 0.2.??? |
---|
comment:2 Changed 4 years ago by
Cc: | isis added |
---|
comment:4 Changed 2 years ago by
Keywords: | tor-03-unspecified-201612 added |
---|---|
Milestone: | Tor: 0.3.??? → Tor: unspecified |
Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.
comment:5 Changed 21 months ago by
Keywords: | tor-03-unspecified-201612 removed |
---|
Remove an old triaging keyword.
comment:6 Changed 21 months ago by
Keywords: | guard-discovery added |
---|---|
Severity: | → Normal |
comment:7 Changed 21 months ago by
Parent ID: | → #9001 |
---|
comment:8 Changed 20 months ago by
Owner: | set to mikeperry |
---|---|
Status: | new → assigned |
I offered to take this in Wilmington, and develop it into a stem-based prototype/performance evaluator.
comment:9 Changed 20 months ago by
Oh interesting. If you'd like help with any of the stem bits Mike then don't hesitate to let me know. :)
comment:10 Changed 20 months ago by
Sponsor: | → SponsorV-can |
---|
comment:11 follow-up: 12 Changed 19 months ago by
Just FYI - I updated asn's branch to current origin/master and renamed the option to HSLayer2Guards. It also now applies to all HS circuit purpose types (not just rends).
New branch location is mikeperry/prop247_torrc-rebased.
Adding HSLayer3Guards next. After that, we can play with stem + onionperf.
comment:12 follow-up: 13 Changed 19 months ago by
Replying to mikeperry:
Just FYI - I updated asn's branch to current origin/master and renamed the option to HSLayer2Guards. It also now applies to all HS circuit purpose types (not just rends).
New branch location is mikeperry/prop247_torrc-rebased.
Adding HSLayer3Guards next. After that, we can play with stem + onionperf.
Sounds good. Took a look at the code and looks reasonable. WRT to:
* XXX: Hrmm.. HSDIR fetches might be CIRCUIT_PURPOSE_C_GENRAL.. How do * we differentiate those?
perhaps you can check for the rend_data
field on the origin_circuit_t
if you have access to that.
comment:13 Changed 19 months ago by
Replying to asn:
Replying to mikeperry:
Just FYI - I updated asn's branch to current origin/master and renamed the option to HSLayer2Guards. It also now applies to all HS circuit purpose types (not just rends).
New branch location is mikeperry/prop247_torrc-rebased.
Adding HSLayer3Guards next. After that, we can play with stem + onionperf.
Sounds good. Took a look at the code and looks reasonable. WRT to:
* XXX: Hrmm.. HSDIR fetches might be CIRCUIT_PURPOSE_C_GENRAL.. How do * we differentiate those?perhaps you can check for the
rend_data
field on theorigin_circuit_t
if you have access to that.
I don't yet. What I did instead was to set a special purpose for HSDIR fetches. Was tricky, but seems to work. I pushed a couple of commits for this and am now testing it with stem.
Also, I noticed that because this patch disables cannibalization, it makes building predicted circuits for hidden services pointless. We also need to alter circuit_predict_and_launch_new() to build the correct purpose breakdown for the HS purposes we need for prediction to do anything for us here... This might impact performance of the prototype. Bleh.
comment:14 Changed 19 months ago by
The repo for the stem-based prototype is now https://github.com/mikeperry-tor/vanguards. It requires the tor patches in https://gitweb.torproject.org/mikeperry/tor.git/log/?h=prop247_torrc-rebased
I also filed #23101 for the prediction issue, and #23100 for a CBT issue.
comment:15 follow-up: 16 Changed 19 months ago by
Just skimmed the simulation code -- this looks like a plausible place to begin with the measurements. Do we have a plan to do these measurements, or should we make one?
Also, I'm assuming that this simulation isn't trying to simulate the exact way in which guard sets change over time. If it is, we need to bring it into conformance with prop271.
comment:16 Changed 19 months ago by
Replying to nickm:
Just skimmed the simulation code -- this looks like a plausible place to begin with the measurements. Do we have a plan to do these measurements, or should we make one?
There is a rough outline of a plan in the comments of that script, but I've been thinking a bit more about it since then. Basically, I think we want to run several onionperf instances with different values for each of the NUM_LAYERN_GUARDS values. We may also want to play with cutting off various percentiles of the network (though this capability is not written in the prototype yet).
We're going to need to measure variance of these instances somehow, or ideally even capture the full performance density distribution for a given parameter set. Variance in performance over time will be the key thing that changes with the parameters. We want to minimize this variance for sane parameter values. I think what this means is that we want to scale the rotation times down, so as to capture the effect of rotation on our performance variance over time for a fixed parameter set.
Also, I'm assuming that this simulation isn't trying to simulate the exact way in which guard sets change over time. If it is, we need to bring it into conformance with prop271.
I thought about this and I don't think we want something very much like prop271 at all. prop271 has a lot of logic about trying to determine connectivity and detect and protect against various guard biasing attempts. For this code, I think we should trust the consensus completely, and have the notion of a "fallback set" and the "primary set", where we prefer the "primary set" if they are in the consensus, but fall back to members of the "fallback set" otherwise. This is kind of what the code does, but hat part is a bit wonky and could be done better.
comment:17 Changed 19 months ago by
FYI: I will be writing a more thorough experimentation plan, a README, and looking at onionperf after I complete more work on subtickets of #9001, but this may not happen until September.
comment:18 Changed 16 months ago by
Keywords: | SponsorV-033 added |
---|
comment:19 Changed 16 months ago by
Keywords: | guard-discovery-033 added; guard-discovery SponsorV-033 removed |
---|
comment:20 Changed 16 months ago by
Keywords: | guard-discovery-prop247-controller added; guard-discovery-033 removed |
---|---|
Milestone: | Tor: unspecified → Tor: 0.3.3.x-final |
comment:21 follow-up: 22 Changed 15 months ago by
Status: | assigned → needs_review |
---|
I have a branch for this in https://oniongit.eu/mikeperry/tor/commits/bug13837. It has two commits: one that just does the dead-simple layer2 and layer3 guard thing, and then a second that adds purposes for hsdirs, so that those use pinned guards too.
This patch disables cannibalization and predictive circuit building for vanguard circuits. That is bug #23101.
Note also that the branch is based on the branch for #23114 currently, since I need all of that code for https://github.com/mikeperry-tor/vanguards to work properly.
I'm not sure the best way to test this. I've been testing it with https://github.com/mikeperry-tor/vanguards. Do we have other suggestions?
comment:22 Changed 15 months ago by
Status: | needs_review → needs_revision |
---|
Replying to mikeperry:
I have a branch for this in https://oniongit.eu/mikeperry/tor/commits/bug13837. It has two commits: one that just does the dead-simple layer2 and layer3 guard thing, and then a second that adds purposes for hsdirs, so that those use pinned guards too.
This patch disables cannibalization and predictive circuit building for vanguard circuits. That is bug #23101.
Note also that the branch is based on the branch for #23114 currently, since I need all of that code for https://github.com/mikeperry-tor/vanguards to work properly.
I'm not sure the best way to test this. I've been testing it with https://github.com/mikeperry-tor/vanguards. Do we have other suggestions?
Very nice! I reviewed the code but haven't tested it yet. Looks good!!!
I suggested some logs and such that would make testing easier. I haven't tried your vanguards project yet either.
After the logs are added (or Ill add them if I need to), I'll start testing this on the realnet as well!
Cheers!
comment:23 follow-up: 24 Changed 15 months ago by
Furthermore, this feature seems to come with at least 3 (major) design changes:
- Adds extra hops to reduce linkability.
- Disables cannibalization.
- Disable prev hop path restriction.
I wonder where's the best way to document these design changes. On the manual page? On a spec somewhere? Not sure.
comment:24 follow-up: 25 Changed 15 months ago by
Replying to asn:
Furthermore, this feature seems to come with at least 3 (major) design changes:
- Adds extra hops to reduce linkability.
- Disables cannibalization.
- Disable prev hop path restriction.
I wonder where's the best way to document these design changes. On the manual page? On a spec somewhere? Not sure.
Unless I fucked up, all of these design changes should only apply when the option is enabled. The first one can be a manpage line. The second two are actually filed as bugs #23101 and #24487. I don't think they warrant much more than a changelog note, because I want them fixed in the 0.3.3 release cycle, if not in the very same 0.3.3 point release.
comment:25 Changed 15 months ago by
Replying to mikeperry:
Replying to asn:
Furthermore, this feature seems to come with at least 3 (major) design changes:
- Adds extra hops to reduce linkability.
- Disables cannibalization.
- Disable prev hop path restriction.
I wonder where's the best way to document these design changes. On the manual page? On a spec somewhere? Not sure.
Unless I fucked up, all of these design changes should only apply when the option is enabled. The first one can be a manpage line. The second two are actually filed as bugs #23101 and #24487. I don't think they warrant much more than a changelog note, because I want them fixed in the 0.3.3 release cycle, if not in the very same 0.3.3 point release.
Agreed.
comment:26 Changed 15 months ago by
Reviewer: | → asn |
---|
comment:27 Changed 15 months ago by
Status: | needs_revision → needs_review |
---|
Ok, a whole bunch of fixups are now at mikeperry/bug13837. I replied to all of the comments on oniongit with the commit hash that addresses them, or with a comment if I didn't think that the change was a good idea (or not a good idea right as part of this ticket).
I am kind of itching to rebase this on top of master, and #23101 on top of it, to minimize conflicts between the two sets of fixups. Let me know if now is a good time to do that, or if we should wait for more fixups.
comment:28 follow-up: 30 Changed 14 months ago by
Status: | needs_review → needs_revision |
---|
OK, fixes mostly look great! I went through all the #13837 discussion on gitlab and made sure everything is answered/addressed.
Please see my branch bug13837-rebased
for some doc fixes and simplifications (the branch is squashed as well).
I encountered some annoying failures during testing this which I reported here: https://oniongit.eu/mikeperry/tor/commit/7e962536f2d89ab0e2b8dd8821503ed66bd115ac#note_1804
I will continue testing this branch this week.
I think the next step here is to squash the branch (including my fixes) and make an oniongit merge request for the squashed branch so that review can continue with Nick. I will also add any new issues I find during testing in the new oniongit merge request.
Cheers!
comment:29 Changed 14 months ago by
Ok, I did the circuit_should_use_vanguards() function name fixup (https://oniongit.eu/mikeperry/tor/commit/47e4055a05c390e726bac4f488d543f07873733d), and then squashed it and rebased everything on to master as mikeperry/bug13837-squashed. I will also rebase the latest squashed bug23101 on top of that.
I'm not going to make a merge request yet, because that issue you found in https://oniongit.eu/mikeperry/tor/commit/7e962536f2d89ab0e2b8dd8821503ed66bd115ac#note_1804 is concerning. I will be afk from now until Jan 2, so I won't be able to fix that until then. So we should just wait on that.
comment:30 follow-up: 31 Changed 14 months ago by
Replying to asn:
I encountered some annoying failures during testing this which I reported here: https://oniongit.eu/mikeperry/tor/commit/7e962536f2d89ab0e2b8dd8821503ed66bd115ac#note_1804
I am pretty sure this is two separate bugs that are orthogonal to this code:
First, we are failing to find a second or third hop for the path because you specified an IP network mask in HSLayer2Guards and HSLayer3Guards. It seems that routersets have a bug/quirk in their network mask handling. See routerset_contains(). They only return "true" for address range checks if the match REJECTED the specified address. If I change that routerset_contains() check to return true if the match is ACCEPTED, the very same netmasks suddenly work. However, if I just patch that routerset_contains function, disparate things that use routersets like excludenodes and exitpolicies suddenly break (in fact, about 12 unittests fail when I change this).
Since we don't need network masks for our usecase, one option is to simply forbid their use for these options. I worry that if we try to do anything complicated than we absolutely need here, we will miss the 0.3.3 merge deadline. Is there something easier/saner we could do here?
Second, I don't think that changing circuit_build_failed() to ignore guard failures if circ->n_chan is NULL is correct. If you can't connect to a guard at all in other circumstances, that should still count as a failure. One thing we could do, though, is add a special check in circuit_build_failed() to not fault the guard node if vanguards are in use *and* we know that the vanguard failed because none of the specified vanguards are available. I can try to do this in a fixup, at least.
comment:31 Changed 14 months ago by
Replying to mikeperry:
Replying to asn:
I encountered some annoying failures during testing this which I reported here: https://oniongit.eu/mikeperry/tor/commit/7e962536f2d89ab0e2b8dd8821503ed66bd115ac#note_1804
I am pretty sure this is two separate bugs that are orthogonal to this code:
First, we are failing to find a second or third hop for the path because you specified an IP network mask in HSLayer2Guards and HSLayer3Guards. It seems that routersets have a bug/quirk in their network mask handling. See routerset_contains(). They only return "true" for address range checks if the match REJECTED the specified address. If I change that routerset_contains() check to return true if the match is ACCEPTED, the very same netmasks suddenly work. However, if I just patch that routerset_contains function, disparate things that use routersets like excludenodes and exitpolicies suddenly break (in fact, about 12 unittests fail when I change this).
Phew, it is not this complicated. routerset_contains() is just written confusingly. It turns out that the /16 restriction is still in effect, so when you specified the same /16 for Layer2 and Layer3 guards, that was actually invalid. I will just work on fixups to make sure we don't blame the entry guard for this.
comment:32 follow-up: 33 Changed 14 months ago by
Ok, two fixups so that we don't blame the entry guard, and emit a log message for some cases of this:
https://oniongit.eu/mikeperry/tor/commit/e9c4aa7823d1396fe2f34173d25f6aa602e9c0ef
https://oniongit.eu/mikeperry/tor/commit/cb1595040bbf4521f25548f00e93860f08aed683
If you like those, and the stuff I am posting in response to your remaining comments on https://oniongit.eu/mikeperry/tor/commit/2829a5fe4a093e27083b4a865380964c94420320, then I will rebase and squash all of this stuff down, and mark it as needs review with a new merge request.
comment:33 Changed 14 months ago by
Replying to mikeperry:
Ok, two fixups so that we don't blame the entry guard, and emit a log message for some cases of this:
https://oniongit.eu/mikeperry/tor/commit/e9c4aa7823d1396fe2f34173d25f6aa602e9c0ef
https://oniongit.eu/mikeperry/tor/commit/cb1595040bbf4521f25548f00e93860f08aed683
Hello, I posted some comments on gitlab about those two patches.
If you like those, and the stuff I am posting in response to your remaining comments on https://oniongit.eu/mikeperry/tor/commit/2829a5fe4a093e27083b4a865380964c94420320, then I will rebase and squash all of this stuff down, and mark it as needs review with a new merge request.
I think the above two patches are the only remaining issues right now, since everything else seems stellar. Let's figure out what to do with those two patches and we can move forward! Woo!
comment:34 Changed 14 months ago by
Status: | needs_revision → merge_ready |
---|
Ok, I fixed those remaining issues, rebased onto origin/master, squashed, and pushed to https://oniongit.eu/mikeperry/tor/commits/bug13837-mergeready. The full chain of fixups is at mikeperry/bug23101-squashed-docs (note this fixup branch still has the e9c4aa7823d1396fe2f34173d25f6aa602e9c0ef fixup, which we decided to drop due to redundancy+possibe user confusion).
comment:35 Changed 13 months ago by
Keywords: | review-group-30 added |
---|
comment:36 Changed 13 months ago by
I've posted some comments on the 3 commits on oniongit. Roger says he'll do so too.
comment:37 Changed 13 months ago by
For commit 6b14e46d:
s/canibalization/cannibalization/ in the commit message
I think I mentioned this in an oniongit comment too, but: in the user- and outward-facing text, we should say onion rather than hidden. We've been saying onion even in the code guts too lately, but that's less critical.
I agree with Nick's point that in the man page, we should be clear about what is a hidden service circuit, and we should also give the user some guidance about why/how/whether to set these things -- if we don't, we run the risk of strange cargo cult instruction documents starting to float around, with people insisting that "HSLayer2Guards {ru}" is the only way to be safe.
comment:38 follow-up: 39 Changed 13 months ago by
One comment on commit f79f4eec:
There keeps being this pattern
- if (purpose == CIRCUIT_PURPOSE_C_GENERAL) { + if (purpose == CIRCUIT_PURPOSE_C_GENERAL || + purpose == CIRCUIT_PURPOSE_S_HSDIR_POST || + purpose == CIRCUIT_PURPOSE_C_HSDIR_GET) {
Is that the perfect time for a macro of some sort? Bonus if the name describes what the three have in common that they keep showing up together.
comment:39 Changed 13 months ago by
Replying to arma:
One comment on commit f79f4eec:
There keeps being this pattern
- if (purpose == CIRCUIT_PURPOSE_C_GENERAL) { + if (purpose == CIRCUIT_PURPOSE_C_GENERAL || + purpose == CIRCUIT_PURPOSE_S_HSDIR_POST || + purpose == CIRCUIT_PURPOSE_C_HSDIR_GET) {Is that the perfect time for a macro of some sort? Bonus if the name describes what the three have in common that they keep showing up together.
We discussed this before, and I still don't have clear ideas here that will make the code obviously cleaner. Note that many of these occurrences are in longer switch statements that would become more confusing if part of them was broken out to check a macro, and several others are combined with other purposes for various reasons...
As for the cases where these three do occur together by themselves, the unifying condition seems to be CIRCUIT_PURPOSE_CAN_SOMETIMES_HAVE_STREAMS_WITH_A_SPECIFIC_EXIT(). And there are only two of those cases. The others involve stream-related behaviors and usually include the rend purpose (but not always)...
FWIW, ideally we would make a more robust version of my mitigation, which allows people to pin all 3 hops through the torrc.