Opened 7 years ago

Last modified 13 months ago

#4581 needs_revision defect

Dir auths should defend themselves from too many begindir requests per address

Reported by: arma Owned by:
Priority: High Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor: 0.2.7
Severity: Normal Keywords: prop258, tor-dos, tor-dirauth
Cc: ln5 Actual Points:
Parent ID: Points: 3
Reviewer: Sponsor: SponsorV-can

Description

#4580 would not have been so bad if we'd had a "you already sent me 5 begindir cells and I haven't even learned what you wanted to request on them yet. I am going to refuse the sixth one." feature.

Alas, the bug causes us to make requests over time, and that will cause us to have multiple OR conns open, so the defense cannot simply be "look at how many other streams we have open on this circuit". I guess some sort of map from IP address to count would do it?

I put this as an 0.2.2 milestone, but if the patch is complex I'll probably not be excited about backporting it.

Child Tickets

Change History (54)

comment:1 Changed 7 years ago by arma

How many is the right number to allow? Clients do descriptor fetches in parallel, and I think the number that they do is a function of the total number of descriptors they plan to fetch. In that case the number of "allowable" parallel fetches should be a function of the size of the network?

But the problem here is consensus fetches, not descriptor fetches, and more than one consensus fetch in parallel to a given dir auth is probably a poor idea.

If only we had some way to know what they will ask for, before they ask it.

comment:2 Changed 7 years ago by nickm

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

We have more questions than answers here; this is far likelier for 0.2.4 than 0.2.2 or 0.2.3. Please move it back if you've got a good idea here.

comment:3 Changed 7 years ago by ln5

Cc: ln5 added

comment:4 Changed 6 years ago by nickm

Keywords: maybe-proposal added

comment:5 Changed 6 years ago by nickm

Keywords: tor-auth added

comment:6 Changed 6 years ago by nickm

Component: Tor Directory AuthorityTor

comment:7 Changed 6 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final
Priority: normalmajor

comment:8 Changed 5 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.???

comment:9 Changed 4 years ago by nickm

Milestone: Tor: 0.2.???Tor: 0.2.7.x-final

These may be worth looking at for 0.2.7.

comment:10 Changed 4 years ago by nickm

Status: newassigned

comment:11 Changed 4 years ago by nickm

Keywords: 027-triaged-1-in added

Marking some tickets as triaged-in for 0.2.7 based on early triage

comment:12 Changed 4 years ago by isabela

Keywords: SponsorU added
Points: medium
Version: Tor: 0.2.7

comment:13 Changed 4 years ago by andrea

Owner: set to andrea

comment:14 Changed 4 years ago by andrea

So, I think the right place to add this check is in connection_exit_connect_dir() or immediately up its call chain. It's easy enough to make this fail on the basis of some criterion, but I believe it's possible for these to occur either from a single-hop circuit (we know the client's real IP) or anonymized, in which case perhaps the criterion should be begindirs from the same circuit rather than the same IP so the would-be attacker at least must work. Thoughts on the right filter to implement, anyone?

comment:15 Changed 4 years ago by andrea

Some notes on this, including testing strategies. We'll need a modified Tor client with the ability to originate directory connections by an arbitrary method to an arbitrary IP/port to perform some of the DoSy client behaviors this ticket envisions filtering for.

To poke dirauths for testing:

 - We'll need an interface to create/teardown directory connections on demand
   to test any filtering we build on the dirauth side; this is going to require
   most of Tor as a subset, so implement by modified Tor client with extra
   control port commands to create/close dirconns.

 - We originate all directory connections through directory_initiate_command()/
   directory_initiate_command_rend() path in directory.c.  Modify this to accept
   an additional dir_connection_t ** param to pass out the newly created
   connection, and to check if the resource/payload/payload_len params are all
   NULL/zero, and avoid calling directory_send_command() if so.

   - Alternately, copy and edit down this function to present a more flexible/
     abusable interface for testing purposes (e.g., eliminate the automatic
     dirport vs. orport selection and allow manual specification of any ip/port
     for any connection method).

 - There are no tests or asserts on resource/payload/payload_len in
   directory_initiate_command_rend(); how does directory_send_command()
   behave if they are NULL/zero?

 - The method directory_initiate_command_rend() uses to set up the connection
   is controlled by use_begindir = directory_command_should_use_begindir(options,
   _addr, or_port, router_purpose, indirection); and anonymized_connection =
   dirind_is_anon(indirection);

   - if !anonymized_connection && !use_begindir, we try connection_connect()
   - otherwise, the anonymized_connection and use_begindir flags control the
     args to connection_ap_make_link(); use_begindir is passed directly, and
     anonymized_connection() controls whether iso_flags is ISO_STREAM or
     ISO_SESSIONGRP.  The conn->dirconn_direct flag is also passed to
     connection_make_ap_link(), and is set to !anonymized_connection.

  - The choice of isolation on the basis of how the connection is tunneled
    will make it impossible to perform some of the DoS attempts we're trying
    to filter for using this function; argues in favor of cloning and editing
    down for a testing interface.

To detect weird/aggressive/DoSy dirauth client behavior:

 - Can we *always* declare receiving an extra begindir cell on a tunneled
   connection a weird behavior we should filter? (from directory.c)

1013     /* Anonymized tunneled connections can never share a circuit.
1014      * One-hop directory connections can share circuits with each other
1015      * but nothing else. */
1016     int iso_flags = anonymized_connection ? ISO_STREAM : ISO_SESSIONGRP;

  - But we can only classify connections as tunneled or not-tunneled on the
    basis of whether the previous hop is a relay in the consensus, and if a
    relay also acts as a client and originated one-hop connections, we will
    always have some false positives.

comment:16 Changed 4 years ago by andrea

Checkpointing the testing tool to generate large numbers of directory connections as my ticket4581_poke_dirauths branch, modulo a couple small TODOs. I do not recommend merging this as it's only really useful for load testing like this and would just make life that much easier for script kiddies and the like. If it were to be merged, that long handle_control_dirhack() function could probably stand to be split up and commented better first.

The other side of this with the connection counting and dropping is forthcoming.

comment:17 Changed 4 years ago by nickm

Keywords: TorCoreTeam201507 added

comment:18 Changed 4 years ago by nickm

Keywords: TorCoreTeam201508 added; TorCoreTeam201507 removed

comment:19 Changed 4 years ago by nickm

Keywords: TorCoreTeam201509 added; TorCoreTeam201508 removed

comment:20 Changed 4 years ago by nickm

Milestone: Tor: 0.2.7.x-finalTor: 0.2.8.x-final

comment:21 Changed 4 years ago by andrea

This grew an EWMA implementation in the process; I don't want to fold "refactor circuitmux_ewma.c" into this, and they would need separate pools of counters since they take their time constants from different sources, but it'd probably be a good idea to consider for the future implementing a generic EWMA module with counter pools that can handle dynamic changes of time constant and refactor both the initial implementation of this and circuitmux_ewma.c to use it. I'm not 100% sure circuitmux_ewma.c gets adjusting the existing counters for decay rate changes correct anyway.

comment:22 Changed 3 years ago by nickm

Keywords: 028-triaged added

comment:23 Changed 3 years ago by nickm

Keywords: SponsorU removed
Sponsor: SponsorU

Bulk-replace SponsorU keyword with SponsorU field.

comment:24 Changed 3 years ago by andrea

Severity: Normal

Completed initial version for review in my ticket4581 branch; unit tests forthcoming.

comment:25 Changed 3 years ago by nickm

Status: assignedneeds_review

comment:26 Changed 3 years ago by andrea

Some small bugfixes in now; this is now verified to correctly create a consensus without tripping the DoS filter starting cold on a test network. Still working on tweaking the filter parameters and best ways of testing the filter, and on unit tests.

comment:27 Changed 3 years ago by dgoulet

Status: needs_reviewneeds_revision
  • In dirdosfilter.c
    • dirdosfilter_bump_anon_dirport() has an unsused param: dst_port. Same for dirdosfilter_bump_direct(). Same goes for dst_addr.
    • dirdosfilter_bump_direct() also doesn't use dst_addr.
    • Most logging statement cast uint32_t to unsigned int which is fine in terms of size but we should either use a U32_PRINTF_ARG or PRIu32 from inttypes.h (that is already included).
    • dirdosfilter_bump_circuit_begindir(): Is it possible to _not_ have a circuit there? Maybe an assert() is a bit too agressive (do not know) but if we should have a circuit and we do not, I would put a BUG log since BEGIN_DIR without circuit seems weird. Same goes for no channel, since circ->p_chan->global_identifier is used, if we can't find the channel, I would be surprised...
    • dirdosfilter_set_time_constant(): Could be nothing but this is used by config.c to update the constant. Seems weird though to update all counters _before_ we change the dirdosfilter_lambda. Shouldn't we update lambda and then update all counters to fit it's new value? If not, a small comment on explaining that the effect of this function is to simply change the constant and "at some point in time" counters will be updated using the new value.
  • In main.c
    • time_t dirdosfilter_ht_compact is not initialized in time_to just below
    • Probably a typo, it should be COMPACT_DIRDOSFILTER_HT_INTERVAL and time_to.dirdosfilter_ht_compact instead:
          time_to.retry_dns_init = now + RETRY_DNS_INTERVAL;
      
  • I just want to discuss the choice of the torrc DirDosFilter values. There is nothing in the proposal about those, explaining why we chose "2.0" for DirDoSFilterMaxDirectConnRatePerIP or 32 for DirPort connection rate. Would be nice to have an idea on why we think 32 connections for instance is a good high limit or it's just "we do not know, let's try with this".
  • I see that you are working on tests, great! This directly impacts directory authority which is the heart and brain of the network so the more the merrier! :)

That's it for now.

comment:28 Changed 3 years ago by micah

Just wanted to bump this issue, it looks like there has been code review done a few months ago, what is the current status of this?

comment:29 Changed 3 years ago by teor

Keywords: TorCoreTeam201601 added; TorCoreTeam201509 removed

comment:30 Changed 3 years ago by nickm

Bulk-modify: It is February 2016, and no longer possible that anything else will get done in January 2016. Time's arrow and all that.

comment:31 Changed 3 years ago by nickm

Keywords: TorCoreTeam201602 added; TorCoreTeam201601 removed

comment:32 Changed 3 years ago by isis

Keywords: prop258 added; maybe-proposal removed

comment:33 Changed 3 years ago by nickm

Milestone: Tor: 0.2.8.x-finalTor: 0.2.9.x-final

These seem like features, or like other stuff unlikely to be possible this month. Bumping them to 0.2.9

comment:34 Changed 3 years ago by isabela

Sponsor: SponsorUSponsorU-can

comment:35 Changed 3 years ago by nickm

Keywords: TorCoreTeam201602 removed

comment:36 Changed 3 years ago by nickm

Keywords: tor-dos added

comment:37 Changed 3 years ago by isabela

Points: medium3

comment:38 Changed 3 years ago by nickm

Parent ID: #17280

comment:39 Changed 3 years ago by nickm

Parent ID: #17280#17293

comment:40 Changed 3 years ago by nickm

Keywords: nickm-deferred-20160905 added
Milestone: Tor: 0.2.9.x-finalTor: 0.2.???

Deferring many tickets that are in needs_revision with no progress noted for a while, where I think they could safely wait till 0.3.0 or later.

Please feel free to move these back to 0.2.9 if you finish the revisions soon.

comment:41 Changed 3 years ago by nickm

Parent ID: #17293

Unparenting these from #17293; holding for future work.

comment:42 Changed 2 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:43 Changed 2 years ago by nickm

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:44 Changed 22 months ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:45 Changed 22 months ago by nickm

Keywords: 027-triaged-in added

comment:46 Changed 22 months ago by nickm

Keywords: 027-triaged-in removed

comment:47 Changed 22 months ago by nickm

Keywords: 027-triaged-1-in removed

comment:48 Changed 22 months ago by nickm

Keywords: 028-triaged removed

comment:49 Changed 22 months ago by nickm

Keywords: nickm-deferred-20160905 removed

comment:50 Changed 22 months ago by dgoulet

Keywords: tor-dirauth added; tor-auth removed

Turns out that tor-auth is for directory authority so make it clearer with tor-dirauth

comment:51 Changed 21 months ago by nickm

Sponsor: SponsorU-canSponsorV-can

comment:52 Changed 13 months ago by dgoulet

Status: needs_revisionnew

comment:53 Changed 13 months ago by dgoulet

Owner: andrea deleted
Status: newassigned

comment:54 Changed 13 months ago by dgoulet

Status: assignedneeds_revision
Note: See TracTickets for help on using tickets.