Opened 5 years ago

Closed 5 years ago

#13671 closed defect (fixed)

Circuit display is broken if bridges are being used.

Reported by: yawning Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Keywords: TorBrowserTeam201412R, TorBrowserTeam201412
Cc: arthuredelstein, gk, mikeperry, isis, mcs Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Note: This may be a good thing, I'm not sure about what the correct behavior should be here yet (As in, there's both a implementation problem, and a design problem here).

The "implementation" problem:

[11-05 09:46:55] Torbutton WARN: Error: 552 Unrecognized key "ns/id/$A09D536DD1752D542E1FBB3C9CE4449D51298239"

This won't work. desc/id/ is what you want.

The "design" problem:

Please think Really Hard about if displaying the IP address of a user's Bridge (or Guard under normal use for that matter) is an ok thing to do. In the Bridge use case, I'm currently in the "If it does display the Bridge IP, I'm going to bother them till they hide it" camp, mainly because making it easy for the user to inadvertently spread the IP address of the bridge they are using is a bad thing.

I'm open to being convinced otherwise on this, and I'm not sure if "the user inadvertently makes the guard they are using public via a screenshot" is a privacy concern or not. This is "less" of an issue since that doesn't have the potential to ruin the Guard for other users, unlike Bridges.

Child Tickets

Attachments (4)

Change History (27)

comment:1 Changed 5 years ago by isis

Cc: isis added

comment:2 Changed 5 years ago by gk

Keywords: tbb-4.5-alpha removed

comment:3 Changed 5 years ago by mcs

Cc: mcs added

comment:4 in reply to:  description Changed 5 years ago by asn

Replying to yawning:

The "design" problem:

Please think Really Hard about if displaying the IP address of a user's Bridge (or Guard under normal use for that matter) is an ok thing to do. In the Bridge use case, I'm currently in the "If it does display the Bridge IP, I'm going to bother them till they hide it" camp, mainly because making it easy for the user to inadvertently spread the IP address of the bridge they are using is a bad thing.

I'm open to being convinced otherwise on this, and I'm not sure if "the user inadvertently makes the guard they are using public via a screenshot" is a privacy concern or not. This is "less" of an issue since that doesn't have the potential to ruin the Guard for other users, unlike Bridges.

I can definitely see why displaying bridges IPs is not a very smart thing to do.

That said, in my experience many bridge users understand the scarcity of bridge IPs and they don't just post them on the Internet/pastebin/etc. So I hope that this behavior will never actually cause the burn of many bridges ( > 100 or so).

However, it would probably be a good idea to fix this. One of the problems here is that if you scrub bridge IPs from the logs, users won't know which bridge got blocked so that they can replace it or report it to the Tor devs. It would probably require some way for people to unscrub the logs for such occasions.

comment:5 Changed 5 years ago by arthuredelstein

Status: newneeds_review

I've attached a patch for "the implementation problem" part of this ticket. I agree we should continue thinking Really Hard about what parts of the circuit we want to make visible in the final version.

Changed 5 years ago by arthuredelstein

comment:6 Changed 5 years ago by arthuredelstein

I've now rewritten both patches so that #13672 can be applied before #13671.

comment:7 Changed 5 years ago by isis

Status: needs_reviewneeds_revision

Review notes for arthuredelstein's patch:

  1. For the readBridgeIPs() function, I think you might run into issues with "vanilla" bridges, which are non-PT bridges, and so they lack a "type":
let readBridgeIPs = function (controller) { 
  controller.getInfo("config-text", result => { 
    let bridgeEntries = result.Bridge; 
    if (bridgeEntries) { 
      bridgeEntries.map(entry => { 
        let [type, IPplusPort, ID] = entry.split(/\s+/), 
            // Drop port number of token[1] to get IP address. 
            IP = IPplusPort.split(":")[0]; 
        bridgeIDtoIPmap[ID] = IP; 
      }); 
    } 
  }); 
}; 

Meaning a bridge line for a "vanilla" bridge, which looks like:

Bridge 1.1.1.1:1111 0123456789abcdef0123456789abcdef01234567

Would give you

IP = "0123456789abcdef0123456789abcdef01234567"
  1. The indexing done above in the bridgeIDtoIPmap appears to be case sensitive. BridgeDB hands out lowercase bridge fingerprints, but there is no guarantee as to case since tor handles either.

comment:8 Changed 5 years ago by gk

Given the design problem should we take a patch for the implementation problem for 4.5-alpha-1?

comment:9 in reply to:  8 Changed 5 years ago by arthuredelstein

Replying to gk:

Given the design problem should we take a patch for the implementation problem for 4.5-alpha-1?

I can think of two alternative patches:

  1. Fix the problems Isis noted above, and display bridge IP addresses
  2. Hide the Bridge IP/country from the circuit diagram, but provide a patch that avoids the "unrecognized key" error currently seen with a bridge.

I'll be glad to hear any opinions.

Last edited 5 years ago by arthuredelstein (previous) (diff)

comment:10 Changed 5 years ago by mikeperry

I think a version of option 2 is the best. I think if bridges are configured, it should say the country of that bridge's IP followed by (Bridge) or (Private bridge). People will probably still want to know the country their bridge is in.

comment:11 Changed 5 years ago by arthuredelstein

Status: needs_revisionneeds_review

In the new attachment, I've implemented Mike's suggestion regarding showing only the bridge's country. And the patch should now cope with vanilla bridges as well.

comment:12 Changed 5 years ago by arthuredelstein

(The latest attachment needs the patch for #13672 applied first)

comment:13 Changed 5 years ago by mikeperry

Keywords: TorBrowserTeam2014R added

comment:14 Changed 5 years ago by mikeperry

Keywords: TorBrowserTeam201411R added; TorBrowserTeam2014R removed

comment:15 Changed 5 years ago by gk

Status: needs_reviewneeds_revision

Two comments:
1) just a nit: s/IDS/IDs/

2) The patch is broken if I select bridges via Torbutton onion -> Open Network Settings... -> My Internet Service Provider (ISP) blocks connections to the Tor Network

In this case I see:

[11-25 15:39:12] Torbutton WARN: Error: 552 Unrecognized key "ns/id/FINGEPRINT"
[11-25 15:39:12] Torbutton WARN: Disabling tor display circuit because of an error.

[11-25 15:44:33] Torbutton WARN: Error: 552 Unrecognized key "ns/id/6FINGERPRINT"
[11-25 15:44:33] Torbutton WARN: Disabling tor display circuit because of an error.
[11-25 15:44:33] Torbutton WARN: TypeError: gBrowser.tabContainer is undefined
[11-25 15:44:33] Torbutton WARN: Disabling tor display circuit because of an error.
gBrowser.tabContainer is undefined

Especially the tabContainer thing is nasty as even using a New Identity and unchecking bridge usage is not giving me the circuit display back. Only shutting down and restarting the browser helps.

comment:16 in reply to:  15 Changed 5 years ago by arthuredelstein

Replying to gk:

Two comments:
1) just a nit: s/IDS/IDs/

Thanks!

2) The patch is broken if I select bridges via Torbutton onion -> Open Network Settings... -> My Internet Service Provider (ISP) blocks connections to the Tor Network

In this case I see:

[11-25 15:39:12] Torbutton WARN: Error: 552 Unrecognized key "ns/id/FINGEPRINT"
[11-25 15:39:12] Torbutton WARN: Disabling tor display circuit because of an error.

[11-25 15:44:33] Torbutton WARN: Error: 552 Unrecognized key "ns/id/6FINGERPRINT"
[11-25 15:44:33] Torbutton WARN: Disabling tor display circuit because of an error.
[11-25 15:44:33] Torbutton WARN: TypeError: gBrowser.tabContainer is undefined
[11-25 15:44:33] Torbutton WARN: Disabling tor display circuit because of an error.
gBrowser.tabContainer is undefined

Especially the tabContainer thing is nasty as even using a New Identity and unchecking bridge usage is not giving me the circuit display back. Only shutting down and restarting the browser helps.

Most of the time bridges are working for me, but it does look like I do need to more gracefully handle the situation where an unrecognized key gets through. I'll need to try to improve the error handling of the tor control port before resubmitting this patch.

comment:17 in reply to:  15 Changed 5 years ago by arthuredelstein

Replying to gk:

Two comments:
1) just a nit: s/IDS/IDs/

Fixed.

2) The patch is broken if I select bridges via Torbutton onion -> Open Network Settings... -> My Internet Service Provider (ISP) blocks connections to the Tor Network

In this case I see:

[11-25 15:39:12] Torbutton WARN: Error: 552 Unrecognized key "ns/id/FINGEPRINT"
[11-25 15:39:12] Torbutton WARN: Disabling tor display circuit because of an error.

I've modified the patch to improve error handling. In the case of an unrecognized key, it should gracefully handle the error and will no longer disable the tor circuit display. (The API functions sendCommand and getInfo in tor-control-port.js have been changed to return Promises, and tor-circuit-display.js now uses Firefox's built-in Task.jsm module to handle async requests to the control port.)

[11-25 15:44:33] Torbutton WARN: Error: 552 Unrecognized key "ns/id/6FINGERPRINT"
[11-25 15:44:33] Torbutton WARN: Disabling tor display circuit because of an error.
[11-25 15:44:33] Torbutton WARN: TypeError: gBrowser.tabContainer is undefined
[11-25 15:44:33] Torbutton WARN: Disabling tor display circuit because of an error.
gBrowser.tabContainer is undefined

I now check for the presence of gBrowser.tabContainer so this TypeError shouldn't happen any more.

comment:18 Changed 5 years ago by gk

Status: needs_revisionneeds_review

comment:19 Changed 5 years ago by mikeperry

Keywords: TorBrowserTeam201412R added; TorBrowserTeam201411R removed

comment:20 Changed 5 years ago by mikeperry

Keywords: TorBrowserTeam201412 added; TorBrowserTeam201411 removed

comment:21 Changed 5 years ago by mcs

Kathy and I reviewed 0001-13671-fix-circuit-display-when-bridges-are-used.3.patch. It was interesting to learn about Task.jsm; it seems like a good fit for this code and it makes error handling much cleaner. Here are our comments, all of which can probably be deferred (maybe file a new ticket?):

1) Nit: s/nodeData/nodeDataForID/ in the Example comment here:

// nodeDataForID(controller, id)__.
// Returns the type, IP and country code of a node with given ID.
// Example: `nodeData(controller, "20BC91DC525C3DC9974B29FBEAB51230DE024C44")`
// => `{ type : "default" , ip : "12.23.34.45" , countryCode : "fr" }`
let nodeDataForID = function* (controller, id) {
  let result = {}; // ip, type, countryCode;

2) Nit: Remove spaces before commas in the code above and within info.routerStatusParser inside tor-control-port.js.

3) Nit: In the code above, change the "let result = {};" line to:

let result = {}; // type, ip, countryCode;

(so that the properties mentioned in the comment are listed in the same order as in the preceding comments).

4) Inside the nodeLines function, "Unknown country" and other nearby strings should be localized, someday.

5) Nit: In this code (now switching to tor-control-port.js), remove the "and " before dispatcher.removeCallback (there are now two "and"s in one sentence).

// __io.callbackDispatcher()__.
// Returns dispatcher object with three member functions:
// dispatcher.addCallback(regex, callback), and dispatcher.removeCallback(callback),
// and dispatcher.pushMessage(message).

6) Why do you use "getinfo config-text" instead of "getconf bridge"? I am not sure how large torrc is for some people, but it would seem easier and more efficient to just get the value of the config variable that you need.

7) We saw an "addTabsProgressListener is not defined" exception on the Error Console at this line within syncDisplayWithSelectedTab():

gBrowser.addTabsProgressListener(listener2);

But we were not able to reproduce the problem. Our best guess is that some combination of multiple tabs and New Identity triggered the problem. Looking at the code, it seems like the only way to get there would be if start() within setupDisplay() failed. If gBrowser is missing functions then the window it is part of must be in the process of closing or already closed.

comment:22 in reply to:  21 Changed 5 years ago by arthuredelstein

Replying to mcs:

Kathy and I reviewed 0001-13671-fix-circuit-display-when-bridges-are-used.3.patch. It was interesting to learn about Task.jsm; it seems like a good fit for this code and it makes error handling much cleaner. Here are our comments, all of which can probably be deferred (maybe file a new ticket?):

Thanks for the review!

1) Nit: s/nodeData/nodeDataForID/ in the Example comment here:

// nodeDataForID(controller, id)__.
// Returns the type, IP and country code of a node with given ID.
// Example: `nodeData(controller, "20BC91DC525C3DC9974B29FBEAB51230DE024C44")`
// => `{ type : "default" , ip : "12.23.34.45" , countryCode : "fr" }`
let nodeDataForID = function* (controller, id) {
  let result = {}; // ip, type, countryCode;

Fixed.

2) Nit: Remove spaces before commas in the code above and within info.routerStatusParser inside tor-control-port.js.

Fixed.

3) Nit: In the code above, change the "let result = {};" line to:

let result = {}; // type, ip, countryCode;

(so that the properties mentioned in the comment are listed in the same order as in the preceding comments).

Fixed.

4) Inside the nodeLines function, "Unknown country" and other nearby strings should be localized, someday.

Good point. New ticket: #13881.

5) Nit: In this code (now switching to tor-control-port.js), remove the "and " before dispatcher.removeCallback (there are now two "and"s in one sentence).

// __io.callbackDispatcher()__.
// Returns dispatcher object with three member functions:
// dispatcher.addCallback(regex, callback), and dispatcher.removeCallback(callback),
// and dispatcher.pushMessage(message).

Fixed.

6) Why do you use "getinfo config-text" instead of "getconf bridge"?

Oops -- I have no idea why. New ticket: #13882.

7) We saw an "addTabsProgressListener is not defined" exception on the Error Console at this line within syncDisplayWithSelectedTab():

gBrowser.addTabsProgressListener(listener2);

But we were not able to reproduce the problem. Our best guess is that some combination of multiple tabs and New Identity triggered the problem. Looking at the code, it seems like the only way to get there would be if start() within setupDisplay() failed. If gBrowser is missing functions then the window it is part of must be in the process of closing or already closed.

I tried a little but wasn't able to get that exception to happen. We could check for the presence of gBrowser.addTabsProgressListener at runtime, but maybe it's better to try to understand the problem first. It seems like this bug might happen in a new window if there were s a race condition between constructing the gBrowser object and running createTorCircuitDisplay().

(The new patch version is ...4.patch.)

Last edited 5 years ago by arthuredelstein (previous) (diff)

comment:23 Changed 5 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Ok, I pushed the fixups (commit 25921b57878c273ea4f499eb88ac2590563d9869) but I don't think we will rebundle for it. Thanks.

Note: See TracTickets for help on using tickets.