Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#7889 closed defect (fixed)

Relays should drop/destroy begin cells with streamid 0

Reported by: arma Owned by:
Priority: High Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay 023-backport spec
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Properly behaving clients can't generate a begin cell with streamid 0:

  if (test_stream_id == 0)
    goto again;

but if such a begin cell does arrive at an exit, it will still launch the stream. And since relay_lookup_conn() returns NULL if streamid is 0, so there's no way to address another cell (e.g. a relay end cell) to this stream. It opens but can never be closed.

This is an issue for RELAY_COMMAND_BEGIN, RELAY_COMMAND_BEGIN_DIR, and RELAY_COMMAND_RESOLVE in particular. But we should solve it for all non-control relay cells:

          1 -- RELAY_BEGIN     [forward]
          2 -- RELAY_DATA      [forward or backward]   
          3 -- RELAY_END       [forward or backward]   
          4 -- RELAY_CONNECTED [backward]
         11 -- RELAY_RESOLVE   [forward]
         12 -- RELAY_RESOLVED  [backward]
         13 -- RELAY_BEGIN_DIR [forward]

I think the resolution could be to kill the circuit for breaking protocol?

Bug reported by oftc_must_be_destroyed on oftc.

Child Tickets

Change History (14)

comment:1 Changed 7 years ago by nickm

This is fairly easy to fix. Roger, do you think it wants an 0.2.3 backport?

comment:2 in reply to:  1 Changed 7 years ago by arma

Replying to nickm:

Roger, do you think it wants an 0.2.3 backport?

I don't currently see a reason why it should. It doesn't imo pass the "weasel, this is a real and serious bug that your users need a fix for asap" test.

comment:3 Changed 7 years ago by arma

(not because it's not a real DoS vulnerability -- but because the fix doesn't make enough variations of the vulnerability go away.)

comment:4 Changed 7 years ago by nickm

Keywords: 023-backport added
Status: newneeds_review

I've fixed this, and the circID case too, in branch "bug7889_023". It's against 0.2.3 but merges into master too.

ISTR that there was a thought that this might not only enabled a zombie stream or circuit, but also might create a problem with bandwidth checks; we might seriously want to consider a backport.

comment:5 Changed 7 years ago by nickm

Priority: normalmajor

comment:6 in reply to:  4 Changed 7 years ago by cypherpunks

and the circID case too

Then please clarify torspec too:

The CircID field determines which circuit, if any, the cell is associated with.

The CircID for a CREATE cell is an arbitrarily chosen 2-byte integer

It need clarify that no zero for circuit allowed, only for "control" cells (from handshake).

comment:7 Changed 7 years ago by nickm

Keywords: spec added

agreed.

comment:8 Changed 7 years ago by cypherpunks

I'm worrying about dropping RELAY_COMMAND_END cell even if zero stream_id and client never lookup such stream to any connection. Why not to destroy whole circuit?

comment:9 Changed 7 years ago by cypherpunks

Why not to destroy whole circuit?

No. It's better not to destroy circuit for zero id case. Any worrying resolves by careful reading code if needs.

comment:10 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

After review by arma, merging to 0.2.3 and forward.

comment:11 Changed 7 years ago by nickm

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

comment:12 Changed 7 years ago by andrea

Your branch looks fine to me, nickm. Merge away.

comment:13 Changed 7 years ago by nickm

great; thanks everybody!

comment:14 Changed 7 years ago by cypherpunks

Drops of cell instead of destroying circuit can't prevent distinguishing of clients. Relay can send 100 RELAY_DATA cells with zero stream_id, and detect absent of RELAY_SENDME cell from patched client.

Note: See TracTickets for help on using tickets.