Opened 2 months ago

Closed 3 weeks ago

#26214 closed defect (implemented)

Check stream SENDME against max

Reported by: mikeperry Owned by: mikeperry
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-circuit, tor-cell
Cc: Actual Points:
Parent ID: Points:
Reviewer: dgoulet Sponsor: SponsorV-can

Description

In connection_edge_process_relay_cell() under the RELAY_COMMAND_SENDME handling, we check the circuit-level sendme against the window START_MAX, but we do not check the stream level SENDME against any max

This means that an attacker can send as many stream-level sendme's on a circuit as they like, inflating the stream window as large as they like. This might be a serious OOM bug, but the circuit level SENDME check should prevent that, I think.

Even so, it would be nice to fix this in 0.3.4, so that the vanguards script's detection of invalid/dropped circuit data is more accurate.

Child Tickets

Change History (9)

comment:1 Changed 2 months ago by mikeperry

Status: assignedneeds_review

comment:2 Changed 8 weeks ago by mikeperry

Status: needs_reviewneeds_revision

Eep. Test failure.

comment:3 Changed 8 weeks ago by mikeperry

Status: needs_revisionneeds_review

Ok fixed + added tests to cover this case. Same pull request.

comment:4 Changed 7 weeks ago by dgoulet

Reviewer: dgoulet

comment:5 Changed 6 weeks ago by dgoulet

Keywords: tor-circuit tor-cell added
Status: needs_reviewneeds_revision

Quick question in the review. If you think it is OK, I would be for an extra comment that says why it is OK since this is a bit confusing.

comment:6 Changed 5 weeks ago by mikeperry

Status: needs_revisionneeds_review

Ok I responded to your comment on https://github.com/torproject/tor/pull/123, and added a new commit for the comment explaining why this change won't trigger against well-behaved clients.

However, the branch now has conflicts, so I rebased it on top of maint-0.3.4 and created a new pull request: https://github.com/torproject/tor/pull/162 (The rust tests are failing but that is not because of me -- I only changed the comment and fixed a conflict... the other builds pass).

comment:7 Changed 5 weeks ago by dgoulet

Status: needs_reviewmerge_ready

lgtm;

comment:8 Changed 5 weeks ago by nickm

Squashed as mikeperry_bug26214-rebased_squashed. I'm merging this into master for now, to make sure it doesn't cause disaster on the network. If it doesn't, let's merge to 0.3.4 as well.

comment:9 Changed 3 weeks ago by nickm

Resolution: implemented
Status: merge_readyclosed

Backported to 0.3.4.

Note: See TracTickets for help on using tickets.