[tor-bugs] #22343 [Applications/Tor Browser]: Save as... in the context menu results in using the catch-all circuit

[Tor Bug Tracker & Wiki]

#22343: Save as... in the context menu results in using the catch-all circuit
-------------------------------------------------+-------------------------
 Reporter:  gk                                   |          Owner:
                                                 |  arthuredelstein
     Type:  defect                               |         Status:
                                                 |  needs_review
 Priority:  High                                 |      Milestone:
Component:  Applications/Tor Browser             |        Version:
 Severity:  Major                                |     Resolution:
 Keywords:  tbb-linkability, ff52-esr,           |  Actual Points:
  tbb-7.0-must, tbb-7.0-issues, tbb-regression,  |
  tbb-7.0-frequent, TorBrowserTeam201804         |
Parent ID:                                       |         Points:
 Reviewer:                                       |        Sponsor:
-------------------------------------------------+-------------------------
Changes (by arthuredelstein):

 * status:  needs_revision => needs_review


Comment:

 Replying to [comment:38 gk]:
 > Okay, here come some review notes:

 Thanks for the review.

 > 1) You added a `doc.nodePrincipal` to the `saveURL` call in
 `browser.js`. But it seems to me that principal gets passed as
 `aIsContentWindowPrivate` in `contentAreaUtils.js`. The function
 definition is
 > {{{
 > function saveURL(aURL, aFileName, aFilePickerTitleKey,
 aShouldBypassCache,
 >                  aSkipPrompt, aReferrer, aSourceDocument,
 aIsContentWindowPrivate,
 >                  aContentPrincipal)
 > }}}

 Good catch -- fixed.

 > 2) Could you elaborate a bit on why just adding the
 `isContentWindowPrivate` parameter in the `saveURL` call in
 `nsConetextMenu.js` is enough skipping the loading prinicpal one? It seems
 to me that's wrong. Looking at the code I tried to test my hypothesis by
 setting `browser.download.saveLinkAsFilenameTimeout` to `0`. Then I get
 indeed a notice in the Torbutton log that the download seems to happen
 over the catch-all circuit if I try to download a link via `Save Link
 as...`.

 Yes, this was also wrong. Fixed.

 > 3) There is
 > {{{
 >  * @param aContentPrincipal [optional]
 >  *        The principal to be used for loading and saving the target
 URL.
 > }}}
 > added to the comment regarding `internalSave` in `contentAreaUtils.js`.
 Maybe at it to the comment for `saveImageURL` as well?

 Added.

 > 4) I stumbled over
 > {{{
 > +  if (persistArgs.sourceURI.scheme !== "file") {
 > +    persist.loadingPrincipal = persistArgs.loadingPrincipal;
 > +  }
 > }}}
 > wondering why `file://` is special here. What about `view-source:`? I am
 not sure if it is related to just that file scheme check but when I went
 to download the view-source result of torproject.org I get
 > {{{
 > Security Error: Content at view-source:https://www.torproject.org/ may
 not load or link to resource://gre-resources/viewsource.css.
 > }}}
 > in my browser console which is not happening without your patch. So, do
 you want to check for a non-content loading principal or is it really all
 principals as long as the scheme is not `file://`. If so a comment might
 help (too)?

 Now, instead of disallowing file:// URLs, instead I whitelist http, https
 and ftp URLs. Other protocols will use the default principal.

 New version:
 https://github.com/arthuredelstein/tor-browser/commit/22343+10
 (516cbf20b2b722b3f9522b4b9e5f5b6c25c34322)

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/22343#comment:42>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online