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

hs-v3: Don't assert if we don't have the intro point descriptor #362

Closed
wants to merge 3 commits into from

Conversation

Labels
None yet
Projects
None yet
4 participants
@dgoulet-tor
Copy link
Contributor

@dgoulet-tor dgoulet-tor commented Sep 20, 2018

When sending the INTRODUCE1 cell, we build the needed data for the cell and
there might be issues with the rendezvous point node_t that makes us not able
to do so. In that case, close the rendezvous circuit and return a transient
error meaning that Tor can recover by selecting a new rendezvous point.

If we are unable to build the INTRODUCE1 cell, we'll also close the rendezvous
circuit betting on the fact that some data wasn't encodable. It is practically
impossible in theory.

Fixes #27774

Signed-off-by: David Goulet dgoulet@torproject.org

When sending the INTRODUCE1 cell, we build the needed data for the cell and
there might be issues with the rendezvous point node_t that makes us not able
to do so. In that case, close the rendezvous circuit and return a transient
error meaning that Tor can recover by selecting a new rendezvous point.

If we are unable to build the INTRODUCE1 cell, we'll also close the rendezvous
circuit betting on the fact that some data wasn't encodable. It is practically
impossible in theory.

Fixes #27774

Signed-off-by: David Goulet <dgoulet@torproject.org>
@coveralls
Copy link

@coveralls coveralls commented Sep 20, 2018

Coverage Status

Coverage increased (+0.04%) to 61.936% when pulling 44ef2f7 on dgoulet-tor:ticket27774_035_02 into 1191596 on torproject:master.

* this is a permanent error. */
tor_assert_nonfatal(TO_CIRCUIT(intro_circ)->marked_for_close);
goto perm_err;
if (TO_CIRCUIT(intro_circ)->marked_for_close) {
Copy link
Member

@asn-d6 asn-d6 Sep 21, 2018

Why not do this in hs_circ_send_introduce1() instead of clobbering this function with this info, and trying to infer what hs_circ_send_introduce1() has done?

Copy link
Contributor Author

@dgoulet-tor dgoulet-tor Sep 21, 2018

Good question. The reason is that I wanted that to be a "client action" and not part of the "HS circuit" layer. The client there decides to note down the intro point client cache failure, not the circuit layer.

In other word, I wanted to not have hs_circuit to callback into hs_client.

* get reused. */
hs_cache_client_intro_state_note(service_identity_pk,
&intro_circ->hs_ident->intro_auth_pk,
INTRO_POINT_FAILURE_UNREACHABLE);
Copy link
Member

@asn-d6 asn-d6 Sep 21, 2018

UNREACHABLE or GENERIC?

Copy link
Contributor Author

@dgoulet-tor dgoulet-tor Sep 21, 2018

Good catch. Fixup commit to make it GENERIC: b5d938b


/* We should never select an invalid rendezvous point in theory but if we
* do, this function will fail to populate the introduce data. */
if (BUG(setup_introduce1_data(ip, exit_node, subcredential,
Copy link
Member

@asn-d6 asn-d6 Sep 21, 2018

Hm, why are we wrapping this into a BUG()? If it's a BUG, why do we introduce whole new behavior just for a buggy possibility? Instead we should fix the bug.

Or is this an edge-case that can happen normally? In this case we should not call BUG.

@nmathewson
Copy link
Contributor

@nmathewson nmathewson commented Sep 30, 2018

This has been squashed and merged to master.

@nmathewson nmathewson closed this Sep 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment