Opened 18 months ago

Closed 4 months ago

#18913 closed defect (fixed)

about:tor should not have chrome privileges

Reported by: mcs Owned by: mcs
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: ff52-esr, TorBrowserTeam201706R
Cc: brade, gk Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We should ensure that about:tor runs with only content privileges.

Changing the getURIFlags() function in src/components/aboutTor.js to include Ci.nsIAboutModule.URI_SAFE_FOR_UNTRUSTED_CONTENT in the value returned should do the trick, but other things will need to be fixed as a result of that change.

Child Tickets

Change History (13)

comment:1 Changed 17 months ago by gk

Cc: gk added

comment:2 Changed 17 months ago by cypherpunks

Does this also need to be done for the changelog about page popup or is it the same page as about:tor?

comment:3 in reply to:  2 Changed 17 months ago by mcs

Replying to cypherpunks:

Does this also need to be done for the changelog about page popup or is it the same page as about:tor?

It is a separate page (about:tbupdate) but it already runs with only content privileges.

comment:4 Changed 6 months ago by cypherpunks

Component: Applications/TorbuttonApplications/Tor Browser
Keywords: ff52-esr added

Actual for e10s.

comment:5 in reply to:  4 Changed 6 months ago by mcs

Replying to cypherpunks:

Actual for e10s.

Do you mean that with e10s enabled, we *must* avoid requiring chrome privileges? I do not think that is true, so maybe you mean something else?

comment:6 Changed 4 months ago by mcs

Keywords: TorBrowserTeam201706 added

comment:7 Changed 4 months ago by mcs

Keywords: TorBrowserTeam201706R added; TorBrowserTeam201706 removed
Status: newneeds_review

Here is a patch for review:

https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug18913-01&id=984af558af58bb8715af72c4811acc7fc4253bc1

This change fixes #21948 and #22525 as well, so it would be great to include it in a Tor Browser release soon. While the patch is somewhat large, that is mainly because we had to move a lot of code out of torbutton.js into the new aboutTor-content.js content script (so it can run in the content process where the about:tor DOM is accessible).

comment:8 Changed 4 months ago by mcs

I forgot to mention that Kathy and I did some testing of this patch on OSX, Linux64, and Windows 10.

comment:9 in reply to:  7 ; Changed 4 months ago by gk

Keywords: TorBrowserTeam201706 added; TorBrowserTeam201706R removed
Status: needs_reviewneeds_revision

Replying to mcs:

Here is a patch for review:

https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug18913-01&id=984af558af58bb8715af72c4811acc7fc4253bc1

This change fixes #21948 and #22525 as well, so it would be great to include it in a Tor Browser release soon. While the patch is somewhat large, that is mainly because we had to move a lot of code out of torbutton.js into the new aboutTor-content.js content script (so it can run in the content process where the about:tor DOM is accessible).

Looks good to me, thanks! Just some nits:

+  // process that is only available here (in the chrome process). It is sent
+  // sent to the content process when an about:tor window is opened and in

just one "sent"

+   kAboutTorMessages: [ "AboutTor:ChromeData", "AboutTor:ToolbarData" ],
+
+   get isAboutTor() {
+    return content.document.documentURI.toLowerCase() == "about:tor";

Indentation

"the Tor Button item's x coordinate" -> "the x coordinate of Torbutton's toolbar item"

"torbutton toolbar item" -> "Torbutton toolbar item"

comment:10 in reply to:  9 Changed 4 months ago by mcs

Keywords: TorBrowserTeam201706R added; TorBrowserTeam201706 removed
Status: needs_revisionneeds_review

Replying to gk:

Looks good to me, thanks! Just some nits:
...

Thanks for your review! Here is a revised patch that fixes the things you found:
https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug18913-02

comment:11 Changed 4 months ago by gk

Keywords: TorBrowserTeam201706 added; TorBrowserTeam201706R removed
Status: needs_reviewneeds_revision

Sorry, I should have given more context. You have now

+var AboutTorListener = {
+   kAboutTorMessages: [ "AboutTor:ChromeData", "AboutTor:ToolbarData" ],
+
+   get isAboutTor() {
+     return content.document.documentURI.toLowerCase() == "about:tor";
+  },

but I'd like to have it like

+var AboutTorListener = {
+  kAboutTorMessages: [ "AboutTor:ChromeData", "AboutTor:ToolbarData" ],
+
+  get isAboutTor() {
+    return content.document.documentURI.toLowerCase() == "about:tor";
+  },

comment:12 Changed 4 months ago by mcs

Keywords: TorBrowserTeam201706R added; TorBrowserTeam201706 removed
Status: needs_revisionneeds_review

I am sorry we botched the indentation fix, and I am sorry that we then forgot to revise the patch for the past two days. Do you have a git tool to spot those kind of errors, or just "eagle eyes?

In any case, here is a revised patch:
https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug18913-03

comment:13 Changed 4 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Just "eagle eyes". :) Applied to master (commit 32f9cf56c89ccd2e24924975dc5515e4198d28c3).

Note: See TracTickets for help on using tickets.