Opened 4 months ago

Closed 4 months 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 4 months ago by mikeperry

Status: assignedneeds_review

comment:2 Changed 4 months 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 4 months 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 4 months ago by dgoulet

Reviewer: isis

comment:5 in reply to:  3 ; Changed 4 months 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 4 months 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 4 months 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 4 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged!

Note: See TracTickets for help on using tickets.