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

Bug28634 draft #868

Closed
wants to merge 6 commits into from

Conversation

@asn-d6
Copy link
Member

commented Mar 29, 2019

No description provided.

@coveralls

This comment has been minimized.

Copy link

commented Mar 29, 2019

Pull Request Test Coverage Report for Build 4470

  • 132 of 143 (92.31%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.05%) to 62.312%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/core/or/circuitpadding.c 32 43 74.42%
Totals Coverage Status
Change from base Build 4445: 0.05%
Covered Lines: 46287
Relevant Lines: 74283

💛 - Coveralls
@mikeperry-tor
Copy link
Contributor

left a comment

We might need to be a bit more careful here. I think we might be creating the situation where these padded circuits are still unique. I have some thoughts in-line on this.

Stepping back it might also be useful to have padding machines get attached to general circuits to make them look like the intro pattern (or the rend pattern, by spoofing an extra hop). But that might also be overkill for 0.4.1.

bool is_client)
{
/* Token removal strategy for BURST state */
burst_state->token_removal = CIRCPAD_TOKEN_REMOVAL_CLOSEST;

This comment has been minimized.

Copy link
@mikeperry-tor

mikeperry-tor Apr 8, 2019

Contributor

Let's use CIRCPAD_TOKEN_REMOVAL_NONE here, and only rely on the length sampling. That way we avoid a bunch of extra allocations for the histogram that we don't need. Same thing for the rend machine.

This comment has been minimized.

Copy link
@asn-d6

asn-d6 Apr 9, 2019

Author Member

Agreed! BTW, there is a fun bug that needs to be resolved before we can do this. In particular, this block of code triggers

if (!state || state->token_removal == CIRCPAD_TOKEN_REMOVAL_NONE) {

and destroys the histogram right when we transition from START to BURST.

Any ideas on how this should be fixed? Also I remember you mentioned that a simplification was possible in that function, but we never really did it. Perhaps this is a good time along with the bugfix?

This comment has been minimized.

Copy link
@mikeperry-tor

mikeperry-tor Apr 9, 2019

Contributor

No, this only destroys the mutable copy of the histogram that is in the machine runtime from any previous state (in this case there was no histogram in START, so it should already be NULL). It also prevents us from allocating another mutable copy from the BURST state. If the machine runtime has no histrogram copy, the code still uses the readonly copy in the machine_spec_t. (This is part of the reason why I wanted to differentiate between mutable info and readonly info in the code/naming.. some stuff does not need to be in the runtime for it to run.. it only has to be there if it is mutable).

histogram = state->histogram;

and

case CIRCPAD_TOKEN_REMOVAL_NONE:

This comment has been minimized.

Copy link
@asn-d6

asn-d6 Apr 17, 2019

Author Member

ACK thanks for the clarification. Also added some comments in 7dc7c02.

src/core/or/circuitpadding_machines.c Show resolved Hide resolved
{
/* Token removal strategy for BURST state */
burst_state->token_removal =
CIRCPAD_TOKEN_REMOVAL_CLOSEST;

This comment has been minimized.

Copy link
@mikeperry-tor

mikeperry-tor Apr 8, 2019

Contributor

Again, using CIRCPAD_TOKEN_REMOVAL_NONE will avoid lots of needless mallocs.

This comment has been minimized.

Copy link
@asn-d6

asn-d6 Apr 17, 2019

Author Member

Done in 7dc7c02.


/* XXX Keep the circuit alive even after the introduction has been finished,
* otherwise the short-term lifetime of the circuit will blow our cover */
// client_machine->manage_circ_lifetime = 1;

This comment has been minimized.

Copy link
@mikeperry-tor

mikeperry-tor Apr 8, 2019

Contributor

Note that we also want to set client_machine->should_negotiate_end = 1 on the client side here, since the relay side will not be managing lifetime (as you say below).

This comment has been minimized.

Copy link
@asn-d6

asn-d6 Apr 17, 2019

Author Member

Done in 7dc7c02.

*
* We want a machine that will send a bunch of fake cells over a reasonable
* time frame, to obfuscate the fact that introduction circuits always send 4
* cells both ways */

This comment has been minimized.

Copy link
@mikeperry-tor

mikeperry-tor Apr 8, 2019

Contributor

This comment seems to be cut+paste from the introduction machine? Needs update.

This comment has been minimized.

Copy link
@asn-d6

asn-d6 Apr 17, 2019

Author Member

Fixed in 7dc7c02.

burst_state->histogram_len = 2;
burst_state->histogram_edges[0] = 30000;
burst_state->histogram_edges[1] = 200000;

This comment has been minimized.

Copy link
@mikeperry-tor

mikeperry-tor Apr 8, 2019

Contributor

I think that doing the HTTP get thing here might help also.

And again we have to be careful about creating a thing that is now obviously unique. If we start padding before at least 3 hops are built, for example, then only padded intro/rend circs will be the ones with mismatched EXTEND2/EXTENDED2 request/response pattern. We can enforce the "don't pad until three hops" situation with the min_hops machine condition.

This comment has been minimized.

Copy link
@asn-d6

asn-d6 Apr 10, 2019

Author Member

Yes, that's a good idea! If we let intro circs build the circuit up to the intro point before padding, then the extra EXTEND->EXTENDED roundtrip (bold is outgoing) will blend in with the BEGIN->CONNECTED roundtrip that most normal circuits have (if you look at the backgammon board). We can start padding after that and then we will blend much better since then the NEGOTIATE_PADDING->INTRO1 will also blend with the DATA->DATA that most normal circuits have.

Similarly for rend circuits, we should wait until the rendezvous circuit is established so that the ESTABLISH_REND->REND_ESTABLISHED dance, blends in with the BEGIN->CONNECTED dance of normal circuits.

This comment has been minimized.

Copy link
@asn-d6

asn-d6 Apr 11, 2019

Author Member

I have some good results by following this lead. I basically managed to hide the initial cell sequence of intro circs inside general circs by manipulating the allowed purposes: 90a81fc#diff-c1ce03b78513b52ceac68cfe10c5f991R203

I tried to do the same for rendezvous circuits in that commit but it didn't work as well. It still stands out as pretty unique.

I'm attaching a backgammon board for the commit above when used on the real network. See how intro circuits are blending in with general circuits very well, but rendezvous circuits still stand out. I think I might be able to tweak the state machine transitions and perhaps get to where we want...

Figure_1

tor_free(m);
}

/** Free all memory allocated by the circuitpadding subsystem. */

This comment has been minimized.

Copy link
@mikeperry-tor

mikeperry-tor Apr 8, 2019

Contributor

We should comment somewhere that circpad machine_specs must all be malloced now, if this function is used.

I think our unittests actually add some non-malloc'd machines from global static allocation to the smartlist..

This comment has been minimized.

Copy link
@asn-d6

asn-d6 Apr 17, 2019

Author Member

Agreed. Added comments in 7dc7c02.

@mikeperry-tor
Copy link
Contributor

left a comment

Hrmm not sure if I also need to request changes here to flag this -- but circpad_free_all() is dangerous when the padding machine lists contain static/global scope circpad_machine_spec_t objects.

@asn-d6

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

PR overseeded in favor of #635

@asn-d6 asn-d6 closed this Apr 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.