Mitigate guard discovery by pinning middle node

[asn]

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]

[asn]

FWIW, ideally we would make a more robust version of my mitigation, which allows people to pin all 3 hops through the torrc.

[teor]

Milestone renamed

[nickm]

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

[nickm]

Remove an old triaging keyword.

[mikeperry]

I offered to take this in Wilmington, and develop it into a stem-based prototype/performance evaluator.

[atagar]

Oh interesting. If you'd like help with any of the stem bits Mike then don't hesitate to let me know. :)

[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.

[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 the origin_circuit_t if you have access to that.

[mikeperry]

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 the origin_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.

[mikeperry]

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.

[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?

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.

[mikeperry]

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.

[mikeperry]

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.

[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?

[asn]

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!

[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.

[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.

[asn]

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.

[mikeperry]

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.

[asn]

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!

[mikeperry]

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.

[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).

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.

[mikeperry]

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.

[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

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.

[asn]

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!

[mikeperry]

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).

[nickm]

I've posted some comments on the 3 commits on oniongit. Roger says he'll do so too.

[arma]

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.

[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.

[mikeperry]

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)...

[nickm]

merged!