#20410 closed defect (fixed)
Tor master breaks bridge clients
Reported by: | teor | Owned by: | |
---|---|---|---|
Priority: | High | Milestone: | Tor: 0.3.0.x-final |
Component: | Core Tor/Tor | Version: | Tor: 0.3.0.0-alpha-dev |
Severity: | Major | Keywords: | crash bridge-client TorCoreTeam201610 |
Cc: | chelseakomlo | Actual Points: | .2 |
Parent ID: | Points: | 0.5 | |
Reviewer: | Sponsor: |
Description
When I run 'make test-network-all' on master (0.3.0-alpha-dev), both bridge tests are broken, apparently because the bridge client crashes:
$ make test-network-all $CHUTNEY_PATH was not set. Assuming test-network.sh will find ./../chutney mkdir -p ./test_network_log ping6 ::1 succeeded, running IPv6 flavors: bridges+ipv6-min ipv6-exit-min hs-ipv6 single-onion-ipv6. tor-stable not found, skipping mixed flavors: mixed. SKIP: mixed PASS: basic-min FAIL: bridges-min PASS: hs-min PASS: single-onion FAIL: bridges+ipv6-min PASS: ipv6-exit-min PASS: hs-ipv6 PASS: single-onion-ipv6 Log and result files are available in ./test_network_log. make: *** [test-network-all] Error 1
This is consistent across Linux 64-bit, macOS 64-bit, and macOS 32-bit.
0.2.8.9 and 0.2.9.4-alpha work fine, so it's probably #20389 or #20077 or #19858 or #13827 or #6769.
Child Tickets
Change History (12)
comment:1 Changed 2 years ago by
Cc: | chelseakomlo added |
---|
comment:2 Changed 2 years ago by
It's also possible I broke things with c87d9b13a4e56237e22df776c47e5520e0d37103 , but I don't think so.
comment:3 Changed 2 years ago by
Keywords: | crash bridge-client added |
---|---|
Priority: | Medium → High |
I think I understand why this is happening based on 195ccce in #20077:
- launch_direct_bridge_descriptor_fetch calls directory_initiate_command with DIR_PURPOSE_FETCH_SERVERDESC, ROUTER_PURPOSE_BRIDGE, and DIRIND_ONEHOP
- directory_initiate_command calls directory_initiate_command_rend
- directory_initiate_command_rend asserts because purpose_needs_anonymity believes all ROUTER_PURPOSE_BRIDGE requests must be anonymous (3-hop), but this clearly isn't true for bridge descriptor fetches straight from the bridge itself.
I think the correct fix for this is to modify purpose_needs_anonymity to require anonymity for ROUTER_PURPOSE_BRIDGE, except for DIR_PURPOSE_FETCH_SERVERDESC. (This might be problematic because it allows fetch_bridge_descriptors() to fetch directly from the bridge authority. Does this matter?)
Also, the other replacement of is_sensitive_dir_purpose with purpose_needs_anonymity is incomplete. We should upcast the linked connection to a dir_connection_t, then check the router_purpose field.
comment:4 follow-up: 5 Changed 2 years ago by
(This might be problematic because it allows fetch_bridge_descriptors() to fetch directly from the bridge authority. Does this matter?)
IMO yes. If we need another complexifying argument or flag in purpose_needs_anonymity() to tell the "fetch from source" case from "fetch from bridge authority, that's better than running into trouble down the road if fetch_bridge_descriptors() does something foolish.
comment:5 Changed 2 years ago by
Replying to nickm:
(This might be problematic because it allows fetch_bridge_descriptors() to fetch directly from the bridge authority. Does this matter?)
IMO yes. If we need another complexifying argument or flag in purpose_needs_anonymity() to tell the "fetch from source" case from "fetch from bridge authority, that's better than running into trouble down the road if fetch_bridge_descriptors() does something foolish.
I agree, the challenge will be to pass this flag down to where it's used (or derive it from the dir_connection_t / function arguments). Actually, it should be pretty easy - just check if the bridge is a configured bridge, and if it is, allow us to fetch bridge server descriptors from it.
comment:6 Changed 2 years ago by
I've done a complete kludge to see if we're on the right track, as branch bug20410_kludge
. Is that about what you had in mind? Does is work for you?
comment:7 Changed 2 years ago by
(that's in my public repository at https://git.torproject.org/nickm/tor.git )
comment:8 Changed 2 years ago by
That's not a kludge, that's almost exactly what I would have done.
We need to look up the dir_conn's router purpose here, instead of passing zero:
- if (purpose_needs_anonymity(linked_dir_conn_base->purpose, 0)) { + if (purpose_needs_anonymity(linked_dir_conn_base->purpose, 0, + TO_DIR_CONN(linked_dir_conn_base)->requested_resource)) {
Then I think we're good to run make test-network-all etc., then merge.
comment:9 Changed 2 years ago by
I also see some whitespace issues, are any of these yours (or from #20077)?
DoubleNL:./src/test/test_dir.c:3290 Wide:./src/test/test_dir.c:3314 Wide:./src/test/test_dir.c:3315 Wide:./src/test/test_dir.c:3318 Wide:./src/test/test_dir.c:3319 Wide:./src/test/test_dir.c:3320 Wide:./src/test/test_dir.c:3321 Wide:./src/test/test_dir.c:3322
comment:10 Changed 2 years ago by
Status: | new → needs_revision |
---|
make test-network-all passes everything, with the following patch on top of nickm's branch:
diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index faf658c..1ee0c0f 100644 --- a/src/or/connection_edge.c +++ b/src/or/connection_edge.c @@ -2434,7 +2434,8 @@ connection_ap_handshake_send_begin(entry_connection_t *ap_conn) * Otherwise, directory connections are typically one-hop. * This matches the earlier check for directory connection path anonymity * in directory_initiate_command_rend(). */ - if (purpose_needs_anonymity(linked_dir_conn_base->purpose, 0, + if (purpose_needs_anonymity(linked_dir_conn_base->purpose, + TO_DIR_CONN(linked_dir_conn_base)->router_purpose, TO_DIR_CONN(linked_dir_conn_base)->requested_resource)) { assert_circ_anonymity_ok(circ, options); }
I'd like to see this merged soon, we're already getting duplicate bug reports.
comment:11 Changed 2 years ago by
Resolution: | → fixed |
---|---|
Status: | needs_revision → closed |
Applied; thanks, all!
comment:12 Changed 2 years ago by
Actual Points: | → .2 |
---|---|
Keywords: | TorCoreTeam201610 added |
Milestone: | → Tor: 0.3.0.x-final |
Looks like the refactor in #20077 changed tor's behaviour: