Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug28780 #630

Closed
wants to merge 19 commits into from
Closed

Bug28780 #630

wants to merge 19 commits into from

Conversation

asn-d6
Copy link
Member

@asn-d6 asn-d6 commented Jan 4, 2019

No description provided.

Mike Perry and others added 19 commits January 2, 2019 15:09
We need this for padding negotiation so that we can have later machine
revisions supercede earlier ones.

Co-authored-by: George Kadianakis <desnacked@riseup.net>
This is a good code review start point, to get an overview of the interfaces
and types used in circuit padding.

Co-authored-by: George Kadianakis <desnacked@riseup.net>
Co-authored-by: George Kadianakis <desnacked@riseup.net>
Co-authored-by: George Kadianakis <desnacked@riseup.net>
This helps us to determine if a middle node can pad to us or not.

Co-authored-by: George Kadianakis <desnacked@riseup.net>
Co-authored-by: George Kadianakis <desnacked@riseup.net>
These event callbacks allow circuit padding to decide when to attempt to
launch and negotiate new padding machines, and when to tear old ones down.

Co-authored-by: George Kadianakis <desnacked@riseup.net>
These callbacks allow the padding state machines to react to various types of
sent and received relay cells.

Co-authored-by: George Kadianakis <desnacked@riseup.net>
This implements all of the event handling, state machines, and padding
decisions for circuit padding.

I recommend reviewing this after you look at the call-in points into it from
the rest of Tor.

Co-authored-by: George Kadianakis <desnacked@riseup.net>
Co-authored-by: George Kadianakis <desnacked@riseup.net>
Co-authored-by: George Kadianakis <desnacked@riseup.net>
Hope is this will make it easier to test on the live tor network.

Does not need to be merged if we don't want to, but will come in handy
for researchers.

Co-authored-by: George Kadianakis <desnacked@riseup.net>
This project introduces the prob_distr.c subsystem which implements all the
probability distributions that WTF-PAD needs. It also adds unittests for all of
them.

Code and tests courtesy of Riastradh.

Co-authored-by: Taylor R Campbell <campbell+tor@mumble.net>
Co-authored-by: Mike Perry <mikeperry-git@torproject.org>
Co-authored-by: Mike Perry <mikeperry-git@torproject.org>
Co-authored-by: Taylor R Campbell <campbell+tor@mumble.net>
Co-authored-by: Mike Perry <mikeperry-git@torproject.org>
Co-authored-by: Mike Perry <mikeperry-git@torproject.org>
Note to self/others: don't merge this.

Co-authored-by: George Kadianakis <desnacked@riseup.net>
/* Check whether the circuitpadding subsystem wants to block this close */
if (!circpad_circuit_should_be_marked_for_close(circ)) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be marking the circuit dirty and/or setting other flags that say "don't attach new streams here"? We don't want to change behavior and cause circuits that really should be not used to get used (for just one example among many: if vanguards decided to close the circuit because of a dropped-cell side channel attack).

In other code, situations like this have caused me to decide to introduce a new circuit purpose (like CIRCUIT_PURPOSE_C_CIRCPAD in this case) to ensure that the circuit cannot be used for anything else that might otherwise grab it. Not sure if that is best in this case, but it's what I would do to really be sure here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related: We should check the reason flag to see if this is a remote reason or some other condition that indicates the other side closed the circuit on us, or it is unusable. (This may require some grepping to verify ciircuit_mark_for_close() calls).

circuit_t *on_circ = mi->on_circ;
if (mi->circuit_was_asked_to_be_closed) {
circuit_mark_for_close(on_circ, END_CIRC_REASON_NONE);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The XXX says "We ignore should_negotiate_end in this case" but we don't return here. I kinda think we should return here and not negotiate the end. Otherwise, circuits torn down due to circpad can have an end cell sent before they are closed, which seems like a (minor) information leak.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ye I should return here. That's a bug.

@ghost ghost closed this May 25, 2021
@ghost ghost deleted the branch torproject:master May 25, 2021 12:55
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants