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 #635

Closed
wants to merge 11 commits into from

Conversation

@asn-d6
Copy link
Member

commented Jan 7, 2019

No description provided.

@asn-d6 asn-d6 force-pushed the asn-d6:bug28634 branch from ae5e774 to c3d8bd4 Jan 7, 2019

@asn-d6 asn-d6 force-pushed the asn-d6:bug28634 branch from c3d8bd4 to 5fe7a7e Apr 17, 2019

@asn-d6 asn-d6 force-pushed the asn-d6:bug28634 branch from 5fe7a7e to 2a09694 Apr 17, 2019

@asn-d6 asn-d6 referenced this pull request Apr 17, 2019
@coveralls

This comment has been minimized.

Copy link

commented Apr 17, 2019

Pull Request Test Coverage Report for Build 4773

  • 162 of 174 (93.1%) changed or added relevant lines in 5 files are covered.
  • 12 unchanged lines in 2 files lost coverage.
  • Overall coverage remained the same at 62.328%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/core/or/circuituse.c 0 1 0.0%
src/core/or/circuitlist.c 1 3 33.33%
src/core/or/circuitpadding.c 51 60 85.0%
Files with Coverage Reduction New Missed Lines %
src/feature/dirauth/shared_random.c 3 85.64%
src/feature/dirauth/dirvote.c 9 64.74%
Totals Coverage Status
Change from base Build 4763: 0.0%
Covered Lines: 46334
Relevant Lines: 74339

💛 - Coveralls
Tune machines to obfuscate the circuit setup cell sequence.
I suggest you re-read the machines.c file instead of reading the diff here.

We abort the "look-like-nothing" strategy of the old machines, for these new
machines tha are carefully hand-tuned to obfuscate the circuit construction
sequence of intro/rend circuits.

We take this approach because it's easier to reason about the properties that
the machines offer, because they have minimal overhead, and also because they
are extensible by adding more states to them to do more advanced tricks

@asn-d6 asn-d6 force-pushed the asn-d6:bug28634 branch from 2a09694 to 3a146a8 Apr 17, 2019

@@ -539,6 +539,11 @@ circpad_machine_sample_delay(circpad_machine_runtime_t *mi)
histogram_total_tokens = state->histogram_total_tokens;
}

/* If we are out of tokens, don't schedule padding. */

This comment has been minimized.

Copy link
@mikeperry-tor

mikeperry-tor Apr 30, 2019

Contributor

What triggered this condition? It seems OK as a safety check, but shouldn't our machines transition to the END state instead of relying on this?

This comment has been minimized.

Copy link
@asn-d6

asn-d6 May 2, 2019

Author Member

Hmm, this triggers for the origin-side rendezvous machine right now, because of this transition:

  machine->states[CIRCPAD_STATE_BURST].
    next_state[CIRCPAD_EVENT_PADDING_SENT] = CIRCPAD_STATE_BURST;

and the fact that the code circpad_send_padding_cell_for_callback() first does the circpad_cell_event_padding_sent(circ); transition and attempts to send padding with zero tokens, and then calls check_machine_token_supply() to potentially bring the machine to END.

The BURST->BURST transition above is not necessary because the machine only has one token, but in previous iterations of the code it had more than one token and that's why I had that transition.

What do you think?

definition of these constants for rationale. */
/* Figure out the length of the BURST state so that it's randomized. We will
* set the histogram such that we don't send any padding from the
* origin-side, so just tune these parameteres for the relay-side. */
burst_state->length_dist.type = CIRCPAD_DIST_UNIFORM;

This comment has been minimized.

Copy link
@mikeperry-tor

mikeperry-tor Apr 30, 2019

Contributor

I do not see the code that prevents us from using the histogram on the origin-side. It looks like the origin side uses the same state machine event setup, and thus will send the same padding as relay side?

This comment has been minimized.

Copy link
@asn-d6

asn-d6 May 2, 2019

Author Member

It's because I do this for the origin-side:

    burst_state->histogram_edges[0] = CIRCPAD_DELAY_INFINITE-1;
    burst_state->histogram_edges[1] = CIRCPAD_DELAY_INFINITE;

which is a hacky way to never send padding.

It's hacky because it's not actually an infinity bin, more like a infinity-1 bin. That was necessary since we recently enforced strictly increasing bin orders and I could not have two infinity edges in a row.


/* Let the origin-side machine control the lifetime of the circuit. */
/* Let the client-side machine control the lifetime of the circuit.
* XXX This might act as a fingerprint at the end of the conn. */

This comment has been minimized.

Copy link
@mikeperry-tor

mikeperry-tor Apr 30, 2019

Contributor

Good point with the XXX. Now that we have manage_circ_lifetime, I don't think we should use should_negotiate_end.. The circuit will stay open until the padding machine shuts down, and then it will time out and get closed by circuit_expire_old_circuits_clientside(). So I would remove this block.

This comment has been minimized.

Copy link
@asn-d6

asn-d6 May 2, 2019

Author Member

Hm, sounds good, but then what would close the relay-side of the circuit? I guess just because the origin-side circuit will close (by expiring), the relay-side of the circuit will also close due to the other end missing? Let me know if this is right and I will remove the block.

This comment has been minimized.

Copy link
@mikeperry-tor

mikeperry-tor May 2, 2019

Contributor

Yes when the origin circuit is closed, it will cause a DESTROY to get sent towards that middle, which will then close and free its circuit.

@asn-d6 asn-d6 closed this May 16, 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.