Opened 7 weeks ago

Closed 6 weeks ago

#26259 closed defect (fixed)

Don't count 0-length RELAY_DATA cell as valid

Reported by: mikeperry Owned by: mikeperry
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-relay-layer, tor-metrics, tor-circuit
Cc: Actual Points:
Parent ID: Points: 1
Reviewer: isis Sponsor: Sponsor8-can

Description

Pointed out by Roger: The CIRC_BW OVERHEAD field currently counts a DATA cell on a valid stream but with 0 length as valid but 100% overhead. Since this can be injected arbitrarily by an end point, and should not happen in normal operation, it should not be counted as valid circuit data.

Child Tickets

Change History (8)

comment:1 Changed 7 weeks ago by mikeperry

Status: assignedneeds_review

comment:2 Changed 7 weeks ago by arma

Should we go farther and disallow 0-length payloads completely? Like, "you're violating the protocol if you try to send them"?

(I thought we already had a ticket for this concept? We came up with the issue in Wilmington.)

comment:3 Changed 7 weeks ago by mikeperry

Possibly. Do we feel comfortable forbidding these cells completely in 0.3.4.x? I would prefer to have this branch merged into 0.3.4, since it is just a control port field accounting fix.

If we're nervous about actually closing the circuit right away in these instances, then we should make a different ticket + different patch for that change.

comment:4 Changed 6 weeks ago by dgoulet

Reviewer: isis

comment:5 in reply to:  3 ; Changed 6 weeks ago by isis

Replying to mikeperry:

Possibly. Do we feel comfortable forbidding these cells completely in 0.3.4.x? I would prefer to have this branch merged into 0.3.4, since it is just a control port field accounting fix.

If we're nervous about actually closing the circuit right away in these instances, then we should make a different ticket + different patch for that change.

Different ticket and patch please! This one is clearly a bug; disallowing 0-length data cells entirely seems more contentious, possible requires a spec change, and has a higher possibility of breaking things.

comment:6 Changed 6 weeks ago by isis

Keywords: tor-relay-layer tor-metrics tor-circuit added
Points: 1
Sponsor: Sponsor8-can
Status: needs_reviewmerge_ready

LGTM!

comment:7 in reply to:  5 Changed 6 weeks ago by nickm

Replying to isis:

Replying to mikeperry:

Possibly. Do we feel comfortable forbidding these cells completely in 0.3.4.x? I would prefer to have this branch merged into 0.3.4, since it is just a control port field accounting fix.

If we're nervous about actually closing the circuit right away in these instances, then we should make a different ticket + different patch for that change.

Different ticket and patch please! This one is clearly a bug; disallowing 0-length data cells entirely seems more contentious, possible requires a spec change, and has a higher possibility of breaking things.

+1 on a different ticket and patch; and if we do it, we need to figure out how we're handling the fingerprinting issue.

comment:8 Changed 6 weeks ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged!

Note: See TracTickets for help on using tickets.