Opened 5 months ago

Closed 3 months ago

#25248 closed enhancement (implemented)

DoS mitgation: improve documentation

Reported by: cypherpunks Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-dos, manpage, tor-doc, 033-triage-20180320, fast-fix, 033-included-20180326
Cc: Actual Points:
Parent ID: Points:
Reviewer: mikeperry Sponsor:

Description

(some reason for opening this is: a relay operator seemed confused and started to modify the source instead of using these torrc settings)
https://lists.torproject.org/pipermail/tor-relays/2018-February/014503.html

building on top of #25236

Lets add a high level overview of available DoS mitigations at the beginning of the section next to "The following options are useful only for a public relay. They control the Denial of Service mitigation subsystem."
as you did in the changelog already before going into the specific settings.

We could start by using a copy from your changelog:
https://gitweb.torproject.org/tor.git/tree/ChangeLog?h=tor-0.3.3.2-alpha#n8

something like:
"
Tor has 3 build-in mitigation options that can be individually enabled/disabled and fine-tuned, but by default Tor directory authorities will define reasonable values for relays and no explicit configuration is required to make use of these protections.
The mitigations are:

  • First: if a single client address makes too many concurrent connections (>100 "too many" is configurable via XXX), hang up on further connections.
  • Second: if a

single client IP address (v4 and v6 or does it just work with IPv4?) makes circuits too quickly (more than 3 per
second, with an allowed burst of 90) while also having too many
connections open (3), refuse new create cells for the next while
(1-2 hours).

  • Third: if a client asks to establish a rendezvous

point to you directly, ignore the request. These defenses can be
manually controlled by new torrc options, but relays will also
take guidance from consensus parameters, so there's no need to
configure anything manually.

"
instead of the static values add the config options in brackets.

https://www.torproject.org/docs/tor-manual-dev.html.en#DoSCircuitCreationEnabled

Does not say what 0 and 1 means. Maybe use the same wording as you use for most other boolean settings:
"If this option is set to 1, ...

  • The section "DENIAL OF SERVICE MITIGATION OPTIONS" refers to the consensus

for default values, lets tell the operator how to find the current consensus values so he has actually some information where they can say "that value is to low for me my system is idle" or "oh that is not defined in consensus" -> #25236

will these values show on https://consensus-health.torprojec.org?

Child Tickets

Change History (14)

comment:1 Changed 5 months ago by nickm

Milestone: Tor: 0.3.3.x-final

comment:2 Changed 4 months ago by nickm

Keywords: 033-triage-20180320 added

Marking all tickets reached by current round of 033 triage.

comment:3 Changed 4 months ago by nickm

Keywords: 033-removed-20180320 added

Mark all not-already-included tickets as pending review for removal from 0.3.3 milestone.

comment:4 Changed 4 months ago by dgoulet

Keywords: fast-fix added; 033-removed-20180320 removed
Owner: set to dgoulet
Status: newaccepted

This should be done so we improve our DoS documentation. Quick fix.

comment:5 Changed 4 months ago by nickm

Keywords: 033-included-20180326 added

comment:6 Changed 4 months ago by dgoulet

Status: acceptedneeds_review

See branch: ticket25248_033_01

comment:7 Changed 3 months ago by dgoulet

Reviewer: mikeperry

comment:8 Changed 3 months ago by mikeperry

Status: needs_reviewneeds_revision

Ok, I read the whole section and I have a few questions/comments:

  1. "Tor has 3 build-in mitigation options" -> "Tor has three built-in mitigation options"
  1. It is not clear how DoSCircuitCreationBurst applies. Does that counter get reset every time the values in DoSCircuitCreationRate and DoSCircuitCreationMinConnections fall above/below their threshold? So that first, a client IP has to exceed DoSCircuitCreationMinConnections, and then exceed DoSCircuitCreationRate, and then we start counting to 90 circuits for that IP? If so, we should state that. If not, we should state how bursts are counted and if/when that counter is reset.
  1. We should also state that the names for the consensus parameters are the same as the torrc names. This is not always the case.
  1. Can we include a statement about log lines people can check for to see if these limits are being hit on their relay? If they are warns, then just saying Tor will emit a warning is enough. If they are notices, then maybe we should have either the log string, or something people can grep for?
  1. This section should be right below the SERVER OPTIONS section, since that is what they are. The first paragraph should also say that these options are for Tor relays/servers (and not for onion services who may be under DoS, which could be another point of confusion here).
  1. Do we also want a dos-spec.txt with this info in torspec.git? I could see some of this stuff being moved to such a place and being cited instead. I don't feel super strongly about this, though.

comment:9 in reply to:  8 Changed 3 months ago by dgoulet

Status: needs_revisionneeds_review

Replying to mikeperry:

Ok, I read the whole section and I have a few questions/comments:

  1. "Tor has 3 build-in mitigation options" -> "Tor has three built-in mitigation options"

Fixed! 3cca21d86b

  1. It is not clear how DoSCircuitCreationBurst applies. Does that counter get reset every time the values in DoSCircuitCreationRate and DoSCircuitCreationMinConnections fall above/below their threshold? So that first, a client IP has to exceed DoSCircuitCreationMinConnections, and then exceed DoSCircuitCreationRate, and then we start counting to 90 circuits for that IP? If so, we should state that. If not, we should state how bursts are counted and if/when that counter is reset.

It goes like this. If the relay reaches DoSCircuitCreationMinConnections for a client IP address, the defense is activated that is we start looking at the circuit rate. Before that, whatever rate you have, we let go.

The bucket value is set to DoSCircuitCreationBurst. We use the DoSCircuitCreationRate to refill the bucket. If the bucket values goes down to 0, we consider it a positive detection.

So the bucket is refilled opportunistically when a new CREATE cell is seen using the timestamp we last refilled it. If you go down DoSCircuitCreationMinConnections, the values stay as is if you ever go up that threshold again, because the timestamp is further in the future, you will max out your bucket at the first CREATE cell. But if you flap very quickly, we'll still have the counters to what we were before so an attacker can't game it by resetting the counter on purpose.

Not sure what you would like to see in the man page out of the above?

  1. We should also state that the names for the consensus parameters are the same as the torrc names. This is not always the case.

They are all the same that is torrc options and consensus parameter. And they all specify that they also obey a parameter.

  1. Can we include a statement about log lines people can check for to see if these limits are being hit on their relay? If they are warns, then just saying Tor will emit a warning is enough. If they are notices, then maybe we should have either the log string, or something people can grep for?

Good idea. Fixed. 3cca21d86b

  1. This section should be right below the SERVER OPTIONS section, since that is what they are. The first paragraph should also say that these options are for Tor relays/servers (and not for onion services who may be under DoS, which could be another point of confusion here).

Commit 2955e48ace

  1. Do we also want a dos-spec.txt with this info in torspec.git? I could see some of this stuff being moved to such a place and being cited instead. I don't feel super strongly about this, though.

Wouldn't be a stupid idea because then anyone can start from there to propose DoS mitigation improvements in the form of a proposal.

Still in ticket25248_033_01

comment:10 Changed 3 months ago by mikeperry

Status: needs_reviewmerge_ready

Ok I think the details of the token bucket don't need to be in the manpage. That is something for the spec I think.

I pushed another fixup commit to mikeperry/ticket25248_033_01 to clarify that the options apply at relays, the consensus params are the same as torrrc options, and to fix a typo.

I think the version in my branch is now merge ready.

comment:11 Changed 3 months ago by dgoulet

Neat. Thanks mike for the fixups!

lgtm;

Branch to squash and merge: mikeperry/ticket25248_033_01

comment:12 Changed 3 months ago by nickm

This doesn't squash cleanly -- I get rebase conflicts. Would one of you be able to squash it for me so I don't mess it up?

comment:13 in reply to:  12 Changed 3 months ago by dgoulet

Replying to nickm:

This doesn't squash cleanly -- I get rebase conflicts. Would one of you be able to squash it for me so I don't mess it up?

Yes, fixup commit from Mike wasn't squashing cleanly so I made an extra commit for his fixes:

See branch in my repo: ticket25248_033_02 (which I rebased on latest maint-0.3.3).

comment:14 Changed 3 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Thanks, David! Merged it to 0.3.3 and forward.

Note: See TracTickets for help on using tickets.