Opened 7 years ago

Closed 6 years ago

#7351 closed enhancement (implemented)

Implement proposal 214: 4-byte circuid IDs

Reported by: nickm Owned by:
Priority: High Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay
Cc: andrea Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I've got a semi-tested implementation of proposal 214 (for 4-byte circuit IDs) in branch "wide_circ_ids" in my public repository.

Some relays are running out of circuit IDs, so we should probably consider this for 0.2.4.x.

Child Tickets

Change History (14)

comment:1 Changed 7 years ago by nickm

Status: newneeds_review

It's fun to write a proposal, and then say "oh hey, I wonder if I can implement that" and have apparently working code about 2 hours later.

It's still only apparently working, though. Caveat haxxor.

comment:2 Changed 7 years ago by nickm

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

Moving into 0.2.5; this doesn't actually seem to be a problem people are hitting yet, though I really think we should be prepared.

comment:3 Changed 7 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: unspecified

On #7921, cypherpunks has pointed out that we should be thinking not only of current average behavior, but also of deliberate circID exhaustion DOS.

comment:4 Changed 7 years ago by nickm

Milestone: Tor: unspecifiedTor: 0.2.4.x-final

leaving this in "unspecified" risks my not seeing it as I scan stuff for 0.2.4.

comment:5 in reply to:  3 ; Changed 6 years ago by arma

Replying to nickm:

On #7921, cypherpunks has pointed out that we should be thinking not only of current average behavior, but also of deliberate circID exhaustion DOS.

I assume you don't mean #7921?

comment:6 in reply to:  5 Changed 6 years ago by nickm

Replying to arma:

Replying to nickm:

On #7921, cypherpunks has pointed out that we should be thinking not only of current average behavior, but also of deliberate circID exhaustion DOS.

I assume you don't mean #7921?

I don't, but I don't know what I *did* mean. Still, the point's fairly clear: we should think about circID-exhaustion DOS.

comment:7 Changed 6 years ago by arma

I don't think this issue happens in practice. So I am not super-excited to make Tor flows more bloaty to address a potential denial-of-service issue -- at least, not while leaving all the other such issues unaddressed.

But I recognize that it is one of those things that one day we'll wish we'd done. So I am torn.

comment:8 Changed 6 years ago by andrea

Status: needs_reviewneeds_revision

If there's a possible DoS I think we need to do this, but there are too many magic constants in this code for me as-is. For example, in fetch_var_cell_from_buf():

const unsigned header_len = wide_circ_ids ? VAR_CELL_MAX_HEADER_SIZE : VAR_CELL_MAX_HEADER_SIZE - 2;

There are also similar constructions in fetch_var_cell_from_evbuffer(), channel_tls_write_packed_cell_method(), connection_bucket_read_limit(), connection_bucket_write_limit(), var_cell_pack_header(), connection_or_flushed_some(), or_handshake_state_record_cell(), connection_or_write_cell_to_buf() and connection_or_process_cells_from_inbuf(). I think we should have a set of inline functions or macros of the form get_cell_size(linkproto), get_max_var_cell_size(linkproto), etc. instead. Maybe also get_circid_len_for_channel() instead of things like "const int wide_circ_ids = linkproto >= MIN_LINK_PROTO_FOR_WIDE_CIRC_IDS; const int circ_id_len = wide_circ_ids ? 4 : 2;"

comment:9 Changed 6 years ago by andrea

I think I'm cool with this branch other than my previous comment.

comment:10 Changed 6 years ago by nickm

Priority: normalmajor

comment:11 Changed 6 years ago by nickm

Status: needs_revisionneeds_review

Okay; I pushed a fix for the magic constants thing to wide_circ_ids in my public repository.

comment:12 Changed 6 years ago by nickm

Cc: andrea added

Testing this with chutney now; it seems to be working. Andrea, did you like my magic numbers fix?

comment:13 Changed 6 years ago by nickm

(It still works in my tests)

comment:14 Changed 6 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Andrea likes. Merging.

Note: See TracTickets for help on using tickets.