Opened 6 years ago

Last modified 2 years ago

#9925 needs_revision defect

Directory Authorities can crash client/relay by scrambling microdesc assignments

Reported by: sysrqb Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-client nickm-patch, SponsorU-deferred, needs-testing
Cc: teor2345@… Actual Points:
Parent ID: Points: medium
Reviewer: Sponsor:

Description

A malicious/misbehaving set of directory authorities can cause a client to fail an assertion if they create a consensus that swaps descriptor digests between router entries and a client already has the descriptors for those routers.

in update_consensus_router_descriptor_downloads()

  SMARTLIST_FOREACH_BEGIN(consensus->routerstatus_list, void *, rsp) {
      routerstatus_t *rs =
        is_vote ? &(((vote_routerstatus_t *)rsp)->status) : rsp;
      signed_descriptor_t *sd;
      if ((sd = router_get_by_descriptor_digest(rs->descriptor_digest))) {
        const routerinfo_t *ri;
        ++n_have;
        if (!(ri = router_get_by_id_digest(rs->identity_digest)) ||
            tor_memneq(ri->cache_info.signed_descriptor_digest,
                   sd->signed_descriptor_digest, DIGEST_LEN)) {

If rs && sd && ri && the descriptor digests are not equal, then

in routerlist_remove_old()

  tor_assert(0 <= idx && idx < smartlist_len(rl->old_routers));
  /* XXXX edmanm's bridge relay triggered the following assert while
   * running 0.2.0.12-alpha.  If anybody triggers this again, see if we
   * can get a backtrace. */
  tor_assert(smartlist_get(rl->old_routers, idx) == sd);

Both assertions are triggerable because sd is assumed to be in old_routers. If the consensus specifies a valid but wrong descriptor digest for a router (i.e. they swap two of them), then the client will compare that new digest to the one it already has in the routerinfo. They will be different, so the client will assume it already has a new descriptor and that it previously moved the old descriptor into rl->old_routers (despite the fact that clients don't cache them and old_routers is empty). When we try to retrieve the cached descriptor, we assert.

If we're a relay, then we probably have descriptors in old_routers but not the one we're looking for. Therefore, these assumption are false, because we're comparing two different routers, thus resulting in the crash.

Brought to you by your friendly neighborhood cat.

Child Tickets

Change History (42)

comment:1 Changed 6 years ago by sysrqb

I have a quick, untested, and likely flawed patch in my public repo in bug9925. In it, if the descriptor digests don't match then we compare the identity digests to verify that they're for the same router.

comment:2 Changed 6 years ago by nickm

Status: newneeds_revision

Reviewing just the code: Well, you can't compare digest with strncmp. memcmp would be tradtional, but in this case tor_memeq and tor_memneq are what you should use. Also, prefer uint8_t[] to char[] for bytes that aren't going to a human. Also, it looks as though you neer actually set need_to_fetch to 0.

Also, this can't be right:

+        if (!(ri = router_get_by_id_digest(rs->identity_digest))) {
+          if (tor_memneq(ri->cache_info.signed_descriptor_digest,

The second "if" will only be executed if ri is NULL. But if ri is NULL, it will crash.

comment:3 Changed 6 years ago by nickm

Status: needs_revisionneeds_review

There's a rather different looking fix in "bug9925_nm" in my public repository. It may not be quite right; more thought is needed. I also included a second patch, to disallow routerstatuses with the same descriptor digest and different identities in the same consensus.

comment:4 Changed 6 years ago by sysrqb

1d9ccb1a99a7590f8269d7dfeabf976836e9891f:

  • The warning message says "Dropping old descriptor to fetch another, but this probably won't work." but we don't actually fetch it. Should we? Or is this a "we'll get it next time"? Or am I misinterpreting "fetch another" to mean "refetch the descriptor" but it really means "skip this one and fetch the next routers' descriptor"?

016c4dcbcddc391515e2fdd0f28832679ab2fae5:

  • Is there a reason to pass seen_descriptor_digests as a parameter? It seems that it is always local to the calling function and never used (only freed) on return.
  • In the commit message s/and/can/ second paragraph.

Otherwise I think this does what it says.

comment:5 Changed 6 years ago by nickm

Or is this a "we'll get it next time"?

It's a "we'll get it next time." But really, it won't work, since if we get a descriptor with the right digest, then -- barring sha1 collisions -- we'll get the same descriptor we already had: the one with the wrong identity key.

Another option might be to mark the descriptor as "just don't download this." Possibly that's a better idea.

Is there a reason to pass seen_descriptor_digests as a parameter?

It needs to stay around between invocations of routerstatus_parse_entry_from_string, since each call to routerstatus_parse_entry_from_string will parse only a single entry, but we want to check (as we parse each entry) whether we have seen *any* duplicate entries before.

comment:6 Changed 6 years ago by arma

Well, if we solve this one right it should also get rid of #1776. :)

comment:7 in reply to:  5 Changed 6 years ago by sysrqb

Replying to nickm:

Another option might be to mark the descriptor as "just don't download this." Possibly that's a better idea.

The latter idea seems better. I was looking at the available routerstatus_t fields, but it seemed abusive to use any of them. For this, I think it will be smarter just to disqualify the router for the current consensus period somehow.

Is there a reason to pass seen_descriptor_digests as a parameter?

It needs to stay around between invocations of routerstatus_parse_entry_from_string, since each call to routerstatus_parse_entry_from_string will parse only a single entry, but we want to check (as we parse each entry) whether we have seen *any* duplicate entries before.

Ah, I missed that key fact, thanks.

comment:8 Changed 5 years ago by nickm

Keywords: 025-triaged added

comment:9 Changed 5 years ago by andrea

Keywords: andrea-review-0255 added

comment:10 Changed 5 years ago by nickm

still looks plausible to me, but it's complex enough that it could use another review IMO.

comment:11 Changed 5 years ago by cypherpunks

About defense-in-depth by 016c4dcbcddc391515e2fdd0f28832679ab2fae5
What happen if someday two different sd for different relays will have the same digest? Hash collision, non-zero chance?
What about digest for md, digestmap functions about DIGEST_LEN while descriptor_digest for md about DIGEST256_LEN. And descriptor_digest is under control of attacker actually, they no need to proof they had onion keys. Can attacker to drop (by client) every md-consensus such way?

And related to this thoughts but another problem perhaps.
What about two rs with the same digest for md-consensus, in general? Can code handle it? (it's another problem perhaps) Attacker could to generate descriptor with onion and ntor keys from victim relay, and choose self id so every new client (without cached documents) will stuck without chance to get victim's keys (md will be assigned to attacker's rs by nodelist_add_microdesc) till next consensus update.

comment:12 Changed 5 years ago by cypherpunks

What happen if someday two different sd for different relays will have the same digest? Hash collision, non-zero chance?

It should be filtered by router_add_to_routerlist, it seems. OK, 1 question resolved.

comment:13 Changed 5 years ago by cypherpunks

What about digest for md, digestmap functions about DIGEST_LEN while descriptor_digest for md about DIGEST256_LEN. And descriptor_digest is under control of attacker actually, they no need to proof they had onion keys. Can attacker to drop (by client) every md-consensus such way?

016c4dcbcddc391515e2fdd0f28832679ab2fae5 counts digest for FLAV_NS only, so attacker can't to trigger this code by fake onion keys. Second question resolved. Remains question is not about this ticket, it seems.

comment:14 Changed 5 years ago by nickm

Keywords: andrea-review-0255 removed
Milestone: Tor: 0.2.5.x-finalTor: 0.2.6.x-final

comment:15 Changed 5 years ago by nickm

Status: needs_reviewneeds_revision

This is complex enough that I should take another pass over the code before we seriously consider merge for 0.2.6

comment:16 Changed 5 years ago by nickm

Keywords: 026-triaged-1 added; 025-triaged removed

comment:17 Changed 5 years ago by nickm

Keywords: nickm-patch added

Add nickm-patch keyword to needs_revision tickets in 0.2.6 where (I think) I wrote the patch, so I know which ones are my responsibility to revise.

comment:18 Changed 5 years ago by teor

Cc: teor2345@… added

comment:19 Changed 5 years ago by nickm

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

comment:20 Changed 4 years ago by nickm

Keywords: 027-triaged-1-out added

Marking triaged-out items from first round of 0.2.7 triage.

comment:21 Changed 4 years ago by nickm

Keywords: PostFreeze027 added

If we wind up with a nice patch for any of these in the appropriate window, we should sure merge it.

comment:22 Changed 4 years ago by nickm

Keywords: PostFreeze027 removed
Milestone: Tor: 0.2.7.x-finalTor: 0.2.8.x-final

comment:23 Changed 4 years ago by nickm

Keywords: 028-triaged added

comment:24 Changed 4 years ago by nickm

Points: medium

comment:25 Changed 4 years ago by nickm

Keywords: pre028-patch added

comment:26 Changed 4 years ago by nickm

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

It is impossible that we will fix all 188 currently open 028 tickets before 028 releases. Time to move some out. This is my second pass through the "needs_revision" tickets, looking for things to move to 0.2.9.

Note that if the requested revisions happen in advance of the 0.2.8 freeze, they can get considered for 0.2.8.

comment:27 Changed 3 years ago by isabela

Severity: Normal
Sponsor: SponsorU-can

comment:28 Changed 3 years ago by isabela

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

tickets market to be removed from milestone 029

comment:29 Changed 3 years ago by nickm

Parent ID: #17293

comment:30 Changed 3 years ago by nickm

Keywords: SponsorU-deferred added
Sponsor: SponsorU-can

Remove the SponsorU status from these items, which we already decided to defer from 0.2.9. add the SponsorU-deferred tag instead in case we ever want to remember which ones these were.

comment:31 Changed 3 years ago by nickm

Parent ID: #17293

comment:32 Changed 3 years ago by teor

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

Milestone renamed

comment:33 Changed 3 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:34 Changed 2 years ago by arma

Is there some more general refactoring we could/should do on the sd and related subsystem, so a fix here would be more manageable?

comment:35 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:36 Changed 2 years ago by nickm

Keywords: 027-triaged-in added

comment:37 Changed 2 years ago by nickm

Keywords: 027-triaged-in removed

comment:38 Changed 2 years ago by nickm

Keywords: 027-triaged-1-out removed

comment:39 Changed 2 years ago by nickm

Keywords: 026-triaged-1 removed

comment:40 Changed 2 years ago by nickm

Keywords: 028-triaged removed

comment:41 Changed 2 years ago by nickm

Keywords: pre028-patch removed

comment:42 Changed 2 years ago by nickm

Keywords: tor-client needs-testing added
Summary: Directory Authorities can crash client/relayDirectory Authorities can crash client/relay by scrambling microdesc assignments
Note: See TracTickets for help on using tickets.