Implement proposal 214: 4-byte circuid IDs

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.

changed milestone to %Tor: 0.2.4.x-final

added component::core tor/tor milestone::Tor: 0.2.4.x-final priority::high resolution::implemented status::closed tor-relay type::enhancement labels

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.

Trac:
Status: new to needs_review

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.

Trac:
Milestone: Tor: 0.2.4.x-final to Tor: 0.2.5.x-final

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

Trac:
Milestone: Tor: 0.2.5.x-final to Tor: unspecified

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

Trac:
Milestone: Tor: unspecified to Tor: 0.2.4.x-final

Replying to nickm:

On #7921 (moved), 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 (moved)?

Replying to arma:

Replying to nickm:

On #7921 (moved), 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 (moved)?

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.

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.

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;"

Trac:
Status: needs_review to needs_revision

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

Trac:
Priority: normal to major

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

Trac:
Status: needs_revision to needs_review

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

Trac:
Cc: N/A to andrea

(It still works in my tests)

Andrea likes. Merging.

Trac:
Resolution: N/A to implemented
Status: needs_review to closed

closed

mentioned in issue #7912 (moved)

mentioned in issue #11553 (moved)

mentioned in issue #11555 (moved)

moved to tpo/core/tor#7351 (closed)

mentioned in issue tpo/core/tor#11555 (closed)