Opened 6 months ago

Closed 5 months ago

#30115 closed defect (fixed)

NoScript's XSS popup breaks circuit display in some cases

Reported by: gk Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-torbutton, tbb-circuit-display, TorBrowserTeam201904R
Cc: arma Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Go to https://getharvest.com and sign in. After entering your credentials and sending them you'll get a NoScript XSS popup which is a false positive in this case. More importantly for this bug, though: if you go back to your window with the Harvest URL in the URL bar, clicking on the identity box does not show your circuit anymore. The whole panel is gone. This does not change either if I select the "block your request" option. The result is, one is stuck and can't change/look at the circuit even.

Found by arma.

Child Tickets

Change History (8)

comment:1 Changed 6 months ago by gk

Cc: arma added

comment:2 Changed 6 months ago by gk

The problem might be related to NoScript creating a new window for that. However, there are a ton of other sites where this does not result in this bug.

comment:3 Changed 5 months ago by acat

patch: https://github.com/acatarineu/torbutton/commit/30115

The problem seems to be common to #27749 and #25145, these should also be fixed with the patch. Currently we keep a mapping of gBrowser.selectedBrowser -> socks credentials for the circuit display, populated when there is a request. This fails for cases when the browser "moves" to a different location but there is no request, which I think is the case here and in the other bugs above. In this case, upon successful login there are several HTTP redirects, one of which triggers the XSS popup and is blocked.

The suggested fix keeps a mapping of domain -> socks credentials instead. I have seen #16936, which I think aims for a different approach, but not sure why.

comment:4 Changed 5 months ago by gk

Keywords: TorBrowserTeam201904R added; TorBrowserTeam201904 removed
Status: newneeds_review

comment:5 Changed 5 months ago by gk

Keywords: TorBrowserTeam201904 added; TorBrowserTeam201904R removed
Status: needs_reviewneeds_revision

The code changes look mostly good.

let urlOrigin = new URL(origin); can throw, though, which we don't catch anymore (given let origin = browser.contentPrincipal.origin || '';). I think we should catch that nevertheless, even if we omit log output now.

That said, consider the following scenario: in tab 1 you open https://torproject.org and in tab 2 you open https://blog.torproject.org. Checking the circuit display on both tabs should show you the same circuit (assume there are no circuit errors like timeouts etc. for the whole example). Now, select tab 2 and change the circuit for the blog by requesting a new one for that site. What should the circuit display show for each tab? With your patch it shows for *both* tabs the new circuit used for https://blog.torproject.org. Without your patch only tab 2 shows the new circuit while tab 1 still shows the one that got originally used to fetch its contents. I think #16936 was concerned with corner cases like that where you have a circuit in one tab and a different one for the same domain in a new tab. And, yes, I think the current behavior we have (not the one coming with your patch) regarding my example above is the one we want. Thus, this patch needs revision.

comment:6 Changed 5 months ago by acat

I think https://github.com/acatarineu/torbutton/commit/30115+1 should preserve current behaviour and still fix this and mentioned bugs (also addressed other review comments).

So, the intended behaviour for circuit display in a given tab and firstPartyDomain is to show the circuit of the last request proxied for that domain in that tab, if I understand it correctly.

comment:7 Changed 5 months ago by gk

Keywords: TorBrowserTeam201904R added; TorBrowserTeam201904 removed
Status: needs_revisionneeds_review

comment:8 in reply to:  6 Changed 5 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Replying to acat:

I think https://github.com/acatarineu/torbutton/commit/30115+1 should preserve current behaviour and still fix this and mentioned bugs (also addressed other review comments).

Looks good to me. Merged to master (commit e44be49b35060f6ae17230bbc63bd617126843ee).

So, the intended behaviour for circuit display in a given tab and firstPartyDomain is to show the circuit of the last request proxied for that domain in that tab, if I understand it correctly.

Yes, that's correct.

Note: See TracTickets for help on using tickets.