Opened 17 months ago

Last modified 13 days ago

#22343 needs_revision defect

Save as... in the context menu results in using the catch-all circuit

Reported by: gk Owned by: arthuredelstein
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Major Keywords: tbb-linkability, tbb-usability, ff52-esr, tbb-7.0-must, tbb-7.0-issues, tbb-regression, tbb-7.0-frequent, TorBrowserTeam201810
Cc: brade, mcs, fufufu, dmr Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Loading a website and then trying to download it via the "Save as..."-option in the context menu results in the catch-all circuit being used. This is new with the switch to ESR52.

Child Tickets

TicketStatusOwnerSummaryComponent
#22649newtbb-teamSave Link As... in the context menu results in using the catch-all circuitApplications/Tor Browser
#26067closedtbb-teamDownloading of images through different circuits than the ones used to view them causes data corruption and incorrect filesApplications/Tor Browser

Change History (53)

comment:1 Changed 17 months ago by gk

Keywords: tbb-7.0-must added; tb-7.0-must removed

comment:2 Changed 17 months ago by arthuredelstein

I also see the catch-all circuit being used when I choose File > Save Page As.

Last edited 17 months ago by arthuredelstein (previous) (diff)

comment:3 Changed 17 months ago by arthuredelstein

Owner: changed from tbb-team to arthuredelstein
Status: newassigned

comment:4 Changed 17 months ago by mcs

Cc: brade mcs added

comment:5 Changed 17 months ago by arthuredelstein

Keywords: TorBrowserTeam201706R added; TorBrowserTeam201705 removed

comment:6 Changed 17 months ago by arthuredelstein

Status: assignedneeds_review

comment:7 Changed 17 months ago by gk

Keywords: TorBrowserTeam201706 added; TorBrowserTeam201706R removed
Status: needs_reviewneeds_revision

Loading torproject.org and doing a right-click and choosing "Save Page As..." gives me a ton of errors like:

[06-07 13:02:38] Torbutton NOTE: tor domain isolator error: channel.loadInfo is null
loadInfo is null for channel https://www.torproject.org/images/military.jpg
Handler function threw an exception: TypeError: channel.loadInfo is null
Stack: NetworkMonitor.prototype._createNetworkEvent@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/network-monitor.js:1077:9
NetworkMonitor.prototype._onRequestHeader@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/network-monitor.js:1154:5
NetworkMonitor.prototype.observeActivity<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/webconsole/network-monitor.js:1014:7
exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
Line: 1077, column: 9

and it does not download anything (while default 7.0 at least downloads the website) I built a custom build with the patch in comment:5 applied on top. But that was the only difference to our usual nightly builds.

comment:8 Changed 17 months ago by gk

Keywords: tbb-7.0-issues tbb-regression added

Regression from 6.X.

comment:9 Changed 16 months ago by arthuredelstein

Keywords: TorBrowserTeam201706R added; TorBrowserTeam201706 removed
Status: needs_revisionneeds_review

Sorry for these errors. I think I have fixed this problem and made some other improvements. Here's a new version for review, rebased on top of the latest tor-browser 7.5 branch:

https://github.com/arthuredelstein/tor-browser/commit/22343+1

comment:10 Changed 16 months ago by cypherpunks

There's something broken with the "Save Link As..." contextual action, as well:

  • It, also, ends up over the catch-all circuit.
  • Furthermore, "sometimes" Tor Browser's custom "Save external file type?" dialog is not shown, instead Firefox's "Save As" dialog is shown directly. When this happens, after confirming said "Save As" dialog, the download is not started ("nothing happens", as they say). I have not yet discovered a reliable way of reproducing this but it's not rare, it occurs after trying a few times with random links.

I have not (yet) seen these problems occur when following the link instead.

Tor Browser 7.0, security level set to High.

comment:11 in reply to:  10 Changed 16 months ago by gk

Replying to cypherpunks:

There's something broken with the "Save Link As..." contextual action, as well:

  • It, also, ends up over the catch-all circuit.

Yes. We might want to outsource that one to an own ticket as the external helper app dialog is involved and we had a bunch of trouble with that piece while transitioning to esr52. I created #22649.

  • Furthermore, "sometimes" Tor Browser's custom "Save external file type?" dialog is not shown, instead Firefox's "Save As" dialog is shown directly. When this happens, after confirming said "Save As" dialog, the download is not started ("nothing happens", as they say). I have not yet discovered a reliable way of reproducing this but it's not rare, it occurs after trying a few times with random links.

#22616 might be of interest to you.

Last edited 16 months ago by gk (previous) (diff)

comment:12 in reply to:  9 Changed 16 months ago by gk

Keywords: TorBrowserTeam201706 added; TorBrowserTeam201706R removed
Status: needs_reviewneeds_revision

Replying to arthuredelstein:

Sorry for these errors. I think I have fixed this problem and made some other improvements. Here's a new version for review, rebased on top of the latest tor-browser 7.5 branch:

https://github.com/arthuredelstein/tor-browser/commit/22343+1

This breaks the Save Image As... option. Currently, that one is at least working even though the catch-all circuit is getting used. But with your patch I get

Full message: TypeError: aInitiatingDocument is null
Full stack: continueSave@chrome://global/content/contentAreaUtils.js:467:1
internalSave/<@chrome://global/content/contentAreaUtils.js:446:7
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:932:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813:7
this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:11

. I guess it might be worth addressing the catch-all-circuit-issue for the image saving part while we are at it.

comment:13 Changed 16 months ago by gk

Keywords: TorBrowserTeam201707 added; TorBrowserTeam201706 removed

Moving Tickets to July 2017.

comment:14 Changed 15 months ago by gk

Keywords: tbb-7.0-frequent added

Tracking frequent 7.0 reports

comment:15 Changed 15 months ago by arthuredelstein

Keywords: TorBrowserTeam201707R added; TorBrowserTeam201707 removed
Status: needs_revisionneeds_review

Here's a new version that covers the Save As part. Please review :)

https://github.com/arthuredelstein/tor-browser/commit/22343+4

comment:16 Changed 15 months ago by gk

Keywords: TorBrowserTeam201707 added; TorBrowserTeam201707R removed
Status: needs_reviewneeds_revision

I still get exceptions:

[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebBrowserPersist.savePrivacyAwareURI]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://global/content/contentAreaUtils.js :: internalPersist :: line 579"  data: no]  (unknown)

STR:

1) Open a locally stored PDF file
2) Right-click on it and choose Save Page As...
3) Choose location and press Save
4) The download fails with the exception
5) It seems even that the file gets corrupted/vanishes

comment:17 Changed 15 months ago by gk

FWIW: This happens on Linux and with the patches for #22618 applied (although I think the later should not matter in this case).

comment:18 Changed 15 months ago by gk

Keywords: TorBrowserTeam201708 added; TorBrowserTeam201707 removed

Moving our Tickets to August.

comment:19 in reply to:  16 Changed 15 months ago by arthuredelstein

Keywords: TorBrowserTeam201708R added; TorBrowserTeam201708 removed
Status: needs_revisionneeds_review

Replying to gk:

I still get exceptions:

Good catch!

Here's a new version for review, with this issue fixed, and rebased to the latest TBB alpha branch:
https://github.com/arthuredelstein/tor-browser/commit/22343+5

comment:20 Changed 15 months ago by gk

mcs/brade: Could you have a look at v5 as well today? We should try to get it into the upcoming alpha is possible.

comment:21 Changed 15 months ago by mcs

Good work on a complicated fix! After reviewing the patch and testing it a little on OSX, Kathy and I have a couple of comments:

  • Consider changing the UUID for embedding/components/webbrowserpersist/nsIWebBrowserPersist.idl
  • Please add documentation for persistArgs.loadingPrincipal to the block comment before the implementation of internalPersist().
  • Code inside browser/base/content/pageinfo/pageInfo.js makes a call to internalSave() but you did not add the content principal parameter there. Is that intentional or is it an oversight? And is there another ticket about saving media from page info using the catch all circuit.
  • Save Image As is still not working for us. about:downloads shows "Failed" and Kathy and I see the following on the browser console:
    15:14:58.300 [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebBrowserPersist.savePrivacyAwareURI]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://global/content/contentAreaUtils.js :: internalPersist :: line 580"  data: no] 1 (unknown)
    	internalPersist chrome://global/content/contentAreaUtils.js:580:5
    	continueSave chrome://global/content/contentAreaUtils.js:489:5
    	internalSave/< chrome://global/content/contentAreaUtils.js:451:7
    	Handler.prototype.process resource://gre/modules/Promise-backend.js:932:23
    	this.PromiseWalker.walkerLoop resource://gre/modules/Promise-backend.js:813:7
    	bound  self-hosted:913:17
    	bound bound  self-hosted:913:17
    	this.PromiseWalker.scheduleWalkerLoop/< resource://gre/modules/Promise-backend.js:747:11
    

comment:22 Changed 15 months ago by gk

Keywords: TorBrowserTeam201708 added; TorBrowserTeam201708R removed
Status: needs_reviewneeds_revision

comment:23 in reply to:  21 ; Changed 15 months ago by arthuredelstein

Replying to mcs:

Good work on a complicated fix! After reviewing the patch and testing it a little on OSX, Kathy and I have a couple of comments:

Thank you for the review!

  • Consider changing the UUID for embedding/components/webbrowserpersist/nsIWebBrowserPersist.idl

Done.

  • Please add documentation for persistArgs.loadingPrincipal to the block comment before the implementation of internalPersist().

Done.

  • Code inside browser/base/content/pageinfo/pageInfo.js makes a call to internalSave() but you did not add the content principal parameter there. Is that intentional or is it an oversight? And is there another ticket about saving media from page info using the catch all circuit.

That was an oversight. :( Also I have now fixed the media saving in this patch as well, so no need to open a ticket for that.

  • Save Image As is still not working for us. about:downloads shows "Failed" and Kathy and I see the following on the browser console:
    15:14:58.300 [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebBrowserPersist.savePrivacyAwareURI]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://global/content/contentAreaUtils.js :: internalPersist :: line 580"  data: no] 1 (unknown)
    	internalPersist chrome://global/content/contentAreaUtils.js:580:5
    	continueSave chrome://global/content/contentAreaUtils.js:489:5
    	internalSave/< chrome://global/content/contentAreaUtils.js:451:7
    	Handler.prototype.process resource://gre/modules/Promise-backend.js:932:23
    	this.PromiseWalker.walkerLoop resource://gre/modules/Promise-backend.js:813:7
    	bound  self-hosted:913:17
    	bound bound  self-hosted:913:17
    	this.PromiseWalker.scheduleWalkerLoop/< resource://gre/modules/Promise-backend.js:747:11
    

I don't seem to be able to reproduce this exception in 22343+5 or 22343+6. Is there a specific image URL and/or procedure I should try?

Here is my revised branch:
https://github.com/arthuredelstein/tor-browser/commit/22343+6

comment:24 Changed 15 months ago by arthuredelstein

Keywords: TorBrowserTeam201708R added; TorBrowserTeam201708 removed
Status: needs_revisionneeds_review

comment:25 in reply to:  23 ; Changed 14 months ago by mcs

Replying to arthuredelstein:

Replying to mcs:

  • Save Image As is still not working for us. about:downloads shows "Failed" and Kathy and I see the following on the browser console:
    15:14:58.300 [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebBrowserPersist.savePrivacyAwareURI]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://global/content/contentAreaUtils.js :: internalPersist :: line 580"  data: no] 1 (unknown)
    	internalPersist chrome://global/content/contentAreaUtils.js:580:5
    	continueSave chrome://global/content/contentAreaUtils.js:489:5
    	internalSave/< chrome://global/content/contentAreaUtils.js:451:7
    	Handler.prototype.process resource://gre/modules/Promise-backend.js:932:23
    	this.PromiseWalker.walkerLoop resource://gre/modules/Promise-backend.js:813:7
    	bound  self-hosted:913:17
    	bound bound  self-hosted:913:17
    	this.PromiseWalker.scheduleWalkerLoop/< resource://gre/modules/Promise-backend.js:747:11
    

I don't seem to be able to reproduce this exception in 22343+5 or 22343+6. Is there a specific image URL and/or procedure I should try?

If I remember correctly, I loaded https://pearlcrescent.com/tor/19273.html, placed the mouse over the w3schools.com image, and then I chose "Save Image As…" from the context menu.

comment:26 in reply to:  25 Changed 14 months ago by cypherpunks

Replying to mcs:

50MB.zip (speedtest.tele2.net FTP download; no access via Tor)

ftp://ftp.adobe.com/pub/adobe/reader/win/11.x/11.0.20/misc/AdbeRdrUpd11020.msp
access via Tor
but see how awful the Tor Network works: every exit node without FTP allowed ends up with REASON=TIMEOUT, and you need a lot of attempts to get to FTP, ignoring 425 errors.

comment:27 in reply to:  25 Changed 14 months ago by arthuredelstein

Keywords: TorBrowserTeam201708 added; TorBrowserTeam201708R removed
Status: needs_reviewneeds_revision

Replying to mcs:

Replying to arthuredelstein:

Replying to mcs:

I don't seem to be able to reproduce this exception in 22343+5 or 22343+6. Is there a specific image URL and/or procedure I should try?

If I remember correctly, I loaded https://pearlcrescent.com/tor/19273.html, placed the mouse over the w3schools.com image, and then I chose "Save Image As…" from the context menu.

Thanks. Indeed this is failing. Right-click to Save As also fails if I load https://www.w3schools.com/html/img_logo.gif directly.

comment:28 Changed 14 months ago by gk

Keywords: TorBrowserTeam201709 added; TorBrowserTeam201708 removed

Items for September 2017.

comment:29 Changed 14 months ago by arthuredelstein

Keywords: TorBrowserTeam201709R added; TorBrowserTeam201709 removed
Status: needs_revisionneeds_review

Here's the patch revised to fix the problem found by mcs and brade for an image inside an iframe, discussed in comment:25:

https://github.com/arthuredelstein/tor-browser/commit/22343+7

comment:30 Changed 13 months ago by gk

Keywords: TorBrowserTeam201709 added; TorBrowserTeam201709R removed
Status: needs_reviewneeds_revision

The patch still breaks things. If you take https://developer.mozilla.org/samples/video/chroma-key/index.xhtml and right-click on the image choosing the "Save image as..." option you get a

A coding exception was thrown in a Promise resolution callback.
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

Full message: TypeError: this.principal is undefined
Full stack: nsContextMenu.prototype.saveMedia/<@chrome://browser/content/nsContextMenu.js:1415:1
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:932:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813:7
this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:11

If you want to download the video as well via right-click and choose "Save Video as..." I get

ReferenceError: sm is not defined nsContextMenu.js:1346:11

Both is not happening without the patch for me and the download works in this case (although it is going over the catch-all circuit).

comment:31 in reply to:  30 Changed 13 months ago by cypherpunks

Replying to gk:

If you take https://developer.mozilla.org/samples/video/chroma-key/index.xhtml

16:09:03.525 NS_BINDING_ABORTED: Component returned failure code: 0x804b0002 (NS_BINDING_ABORTED) [nsIStreamListener.onDataAvailable] 1 WebRequest.jsm:355

with Security Slider on Medium.

comment:32 Changed 13 months ago by gk

Keywords: TorBrowserTeam201710 added; TorBrowserTeam201709 removed

Items for October 2017

comment:33 Changed 11 months ago by gk

Keywords: TorBrowserTeam201711 added; TorBrowserTeam201710 removed

Moving tickets over to November.

comment:34 Changed 11 months ago by gk

Moving tickets to December 2017

comment:35 Changed 11 months ago by gk

Keywords: TorBrowserTeam201712 added; TorBrowserTeam201711 removed

Moving tickets to December 2017, for realz.

comment:36 Changed 9 months ago by arthuredelstein

Status: needs_revisionneeds_review

Here's a new version of the patch for review. I believe I have fixed the errors gk found in comment:30 and cleaned up some of the code.

https://github.com/arthuredelstein/tor-browser/commit/22343+8

comment:37 Changed 9 months ago by gk

Keywords: TorBrowserTeam201801R added; TorBrowserTeam201712 removed

comment:38 Changed 9 months ago by gk

Keywords: TorBrowserTeam201801 added; TorBrowserTeam201801R removed
Status: needs_reviewneeds_revision

Okay, here come some review notes:

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)

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....

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?

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)?

comment:39 Changed 8 months ago by gk

Keywords: TorBrowserTeam201802 added; TorBrowserTeam201801 removed

Moving tickets to Feb

comment:40 Changed 7 months ago by gk

Keywords: TorBrowserTeam201803 added; TorBrowserTeam201802 removed

Adding to our March plate.

comment:41 Changed 6 months ago by gk

Keywords: TorBrowserTeam201804 added; TorBrowserTeam201803 removed

Moving our tickets to April.

comment:42 in reply to:  38 Changed 6 months ago by arthuredelstein

Status: needs_revisionneeds_review

Replying to 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)

comment:43 Changed 6 months ago by arthuredelstein

Keywords: TorBrowserTeam201805R added; TorBrowserTeam201804 removed

comment:44 Changed 5 months ago by gk

Keywords: TorBrowserTeam201805 added; TorBrowserTeam201805R removed
Status: needs_reviewneeds_revision

According to our meeting yesterday marking this as needs_revision as we should base the patch on ESR60.

comment:45 Changed 5 months ago by gk

Priority: HighMedium

comment:46 Changed 5 months ago by gk

Cc: fufufu added

#26067 is a duplicate. This bug actually breaks functionality and is not "just" a privacy issue.

comment:47 Changed 4 months ago by gk

Keywords: TorBrowserTeam201806 added; TorBrowserTeam201805 removed

Moving our tickets to June 2018

comment:48 Changed 3 months ago by gk

Keywords: TorBrowserTeam201807 added; TorBrowserTeam201806 removed

More tickets for July.

comment:49 Changed 3 months ago by gk

Keywords: TorBrowserTeam201808 added; TorBrowserTeam201807 removed

Move our tickets to August.

comment:50 Changed 2 months ago by dmr

Cc: dmr added

comment:51 Changed 2 months ago by dmr

Keywords: tbb-usability added

This can have usability implications when switching a circuit for saving would prevent access to the resource.
For instance:

  • Cloudflare-/captcha-gated sites
  • scenarios where the resource is only available after login
  • other scenarios that require some sort of cookie / client attestation / etc.

comment:52 Changed 6 weeks ago by gk

Keywords: TorBrowserTeam201809 added; TorBrowserTeam201808 removed

Moving our tickets to September 2018

comment:53 Changed 13 days ago by gk

Keywords: TorBrowserTeam201810 added; TorBrowserTeam201809 removed

Moving tickets to October

Note: See TracTickets for help on using tickets.