Opened 19 months ago

Closed 4 weeks ago

Last modified 3 weeks ago

#22343 closed defect (fixed)

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, TorBrowserTeam201811R
Cc: brade, mcs, fufufu, dmr, dwatson@…, igt0, sysrqb 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
#22649closedtbb-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 (68)

comment:1 Changed 19 months ago by gk

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

comment:2 Changed 19 months ago by arthuredelstein

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

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

comment:3 Changed 19 months ago by arthuredelstein

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

comment:4 Changed 19 months ago by mcs

Cc: brade mcs added

comment:5 Changed 19 months ago by arthuredelstein

Keywords: TorBrowserTeam201706R added; TorBrowserTeam201705 removed

comment:6 Changed 19 months ago by arthuredelstein

Status: assignedneeds_review

comment:7 Changed 18 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 18 months ago by gk

Keywords: tbb-7.0-issues tbb-regression added

Regression from 6.X.

comment:9 Changed 18 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 18 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 18 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 18 months ago by gk (previous) (diff)

comment:12 in reply to:  9 Changed 18 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 17 months ago by gk

Keywords: TorBrowserTeam201707 added; TorBrowserTeam201706 removed

Moving Tickets to July 2017.

comment:14 Changed 17 months ago by gk

Keywords: tbb-7.0-frequent added

Tracking frequent 7.0 reports

comment:15 Changed 17 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 17 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 17 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 17 months ago by gk

Keywords: TorBrowserTeam201708 added; TorBrowserTeam201707 removed

Moving our Tickets to August.

comment:19 in reply to:  16 Changed 17 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 17 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 17 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 17 months ago by gk

Keywords: TorBrowserTeam201708 added; TorBrowserTeam201708R removed
Status: needs_reviewneeds_revision

comment:23 in reply to:  21 ; Changed 16 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 16 months ago by arthuredelstein

Keywords: TorBrowserTeam201708R added; TorBrowserTeam201708 removed
Status: needs_revisionneeds_review

comment:25 in reply to:  23 ; Changed 16 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 16 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 16 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 15 months ago by gk

Keywords: TorBrowserTeam201709 added; TorBrowserTeam201708 removed

Items for September 2017.

comment:29 Changed 15 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 15 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 15 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 15 months ago by gk

Keywords: TorBrowserTeam201710 added; TorBrowserTeam201709 removed

Items for October 2017

comment:33 Changed 13 months ago by gk

Keywords: TorBrowserTeam201711 added; TorBrowserTeam201710 removed

Moving tickets over to November.

comment:34 Changed 12 months ago by gk

Moving tickets to December 2017

comment:35 Changed 12 months ago by gk

Keywords: TorBrowserTeam201712 added; TorBrowserTeam201711 removed

Moving tickets to December 2017, for realz.

comment:36 Changed 11 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 11 months ago by gk

Keywords: TorBrowserTeam201801R added; TorBrowserTeam201712 removed

comment:38 Changed 11 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 10 months ago by gk

Keywords: TorBrowserTeam201802 added; TorBrowserTeam201801 removed

Moving tickets to Feb

comment:40 Changed 9 months ago by gk

Keywords: TorBrowserTeam201803 added; TorBrowserTeam201802 removed

Adding to our March plate.

comment:41 Changed 8 months ago by gk

Keywords: TorBrowserTeam201804 added; TorBrowserTeam201803 removed

Moving our tickets to April.

comment:42 in reply to:  38 Changed 7 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 7 months ago by arthuredelstein

Keywords: TorBrowserTeam201805R added; TorBrowserTeam201804 removed

comment:44 Changed 7 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 7 months ago by gk

Priority: HighMedium

comment:46 Changed 7 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 6 months ago by gk

Keywords: TorBrowserTeam201806 added; TorBrowserTeam201805 removed

Moving our tickets to June 2018

comment:48 Changed 5 months ago by gk

Keywords: TorBrowserTeam201807 added; TorBrowserTeam201806 removed

More tickets for July.

comment:49 Changed 4 months ago by gk

Keywords: TorBrowserTeam201808 added; TorBrowserTeam201807 removed

Move our tickets to August.

comment:50 Changed 4 months ago by dmr

Cc: dmr added

comment:51 Changed 4 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 3 months ago by gk

Keywords: TorBrowserTeam201809 added; TorBrowserTeam201808 removed

Moving our tickets to September 2018

comment:53 Changed 2 months ago by gk

Keywords: TorBrowserTeam201810 added; TorBrowserTeam201809 removed

Moving tickets to October

comment:54 Changed 7 weeks ago by arthuredelstein

Status: needs_revisionneeds_review

Here's my patch from comment:42 rebased on top of tor-browser-60.3.0esr-8.5-1:
https://github.com/arthuredelstein/tor-browser/commit/22343+10 (4833bfbd82ac4fb6b8836b326a84a11af8971044)

I manually tested all "save as" functionalities and confirmed they were isolated to first party.

comment:55 Changed 7 weeks ago by gk

Cc: dwatson@… added
Keywords: TorBrowserTeam201810R added; TorBrowserTeam201810 removed

Resolving #22649 as duplicate as that issue will be solved in Arthur's patch/this bug.

comment:56 in reply to:  54 ; Changed 6 weeks ago by gk

Cc: igt0 sysrqb added
Keywords: TorBrowserTeam201810 added; TorBrowserTeam201810R removed
Status: needs_reviewneeds_revision

Replying to arthuredelstein:

Here's my patch from comment:42 rebased on top of tor-browser-60.3.0esr-8.5-1:
https://github.com/arthuredelstein/tor-browser/commit/22343+10 (4833bfbd82ac4fb6b8836b326a84a11af8971044)

Alright, I think that's 22343+11? At least that's the one I used. :) (the commit I looked at is the same).

I think we are close, nice work! Some questions and nits below:

1) We have

+        const principal = Services.scriptSecurityManager.createCodebasePrincipal(
+          makeURI(blobURL), thisPrincipal.originAttributes);
         saveImageURL(blobURL, "canvas.png", "SaveImageTitle",
                      true, false, referrerURI, null, null, null,
-                     isPrivate);
+                     isPrivate, principal);
       }, Cu.reportError);
     } else if (this.onImage) {
-      urlSecurityCheck(this.mediaURL, this.principal);
+      const principal = Services.scriptSecurityManager.createCodebasePrincipal(
+        makeURI(this.mediaURL), thisPrincipal.originAttributes);
+      urlSecurityCheck(this.mediaURL, principal);
       saveImageURL(this.mediaURL, null, "SaveImageTitle", false,
                    false, referrerURI, null, gContextMenuContentData.contentType,
-                   gContextMenuContentData.contentDisposition, isPrivate);
+                   gContextMenuContentData.contentDisposition, isContentWindowPrivate,
+                   principal);

in the patch. Why are we switching in the onImage case from isPrivate to isContentWindowPrivate but not in the former case (the latter onVideo or orAudio case already had that before)? If that's correct and not just an oversight, it might be worth commenting it.

2) From looking at the code in ContentClick.jsm It seems we might be able to trigger window.openLinkIn(json.href, where, params); which could lead to false FPI in the save case (see the: // Todo(903022): code for where == save) or is that just a leftover comment and we are actually good?

3) What about

            ContentAreaUtils.saveImageURL(aTarget.currentRequestFinalURI.spec, null, "SaveImageTitle",
                                          false, true, aTarget.ownerDocument.documentURIObject,
                                          aTarget.ownerDocument);

in mobile/android/chrome/content/browser.js? *If* we can hit it it should get amended as well for FPI reasons I think (maybe even without that just to be on the safe side). Maybe igt0/sysrqb have some insight here. I did not look closer during the review.

4) Where is the new Ci needed in contentAreaUtils.js (see:

+function saveDocument(aDocument, aSkipPrompt, aContentPrincipal) {
+  const Ci = Components.interfaces;
+

It seems to me it is some leftover as Ci is already available in that .js file.

5) loadingPrincipal : aContentPrincipal,: the two whitespaces before the : should go.

6) Maybe add some comment to

+  attribute nsIPrincipal loadingPrincipal;

in the .idl file given that everything else in that file does get a comment explaining what those items are about?

I guess mcs/brade could be good second reviewers given that they already looked at previous patch iterations. Let's hear as well what they think

Last edited 4 weeks ago by gk (previous) (diff)

comment:57 Changed 5 weeks ago by gk

Keywords: TorBrowserTeam201811 added; TorBrowserTeam201810 removed

Moving our tickets to November.

comment:58 in reply to:  56 Changed 5 weeks ago by mcs

Replying to gk:

...
I guess mcs/brade could be good second reviewers given that they already looked at previous patch iterations. Let's hear as well what they think

We agree with gk's comments. Otherwise, this looks good to us.

comment:59 in reply to:  56 ; Changed 4 weeks ago by arthuredelstein

Status: needs_revisionneeds_review

Replying to gk:

Alright, I think that's 22343+11? At least that's the one I used. :) (the commit I looked at is the same).

Thanks for the reviews, everyone! You found the right patch despite my typo.
Here's the new branch: https://github.com/arthuredelstein/tor-browser/commit/22343+12 (a0279161dfeb2857d0a3ba74d858a8be6c48f700)

I think we are close, nice work! Some questions and nits below:

1) We have

+        const principal = Services.scriptSecurityManager.createCodebasePrincipal(
+          makeURI(blobURL), thisPrincipal.originAttributes);
         saveImageURL(blobURL, "canvas.png", "SaveImageTitle",
                      true, false, referrerURI, null, null, null,
-                     isPrivate);
+                     isPrivate, principal);
       }, Cu.reportError);
     } else if (this.onImage) {
-      urlSecurityCheck(this.mediaURL, this.principal);
+      const principal = Services.scriptSecurityManager.createCodebasePrincipal(
+        makeURI(this.mediaURL), thisPrincipal.originAttributes);
+      urlSecurityCheck(this.mediaURL, principal);
       saveImageURL(this.mediaURL, null, "SaveImageTitle", false,
                    false, referrerURI, null, gContextMenuContentData.contentType,
-                   gContextMenuContentData.contentDisposition, isPrivate);
+                   gContextMenuContentData.contentDisposition, isContentWindowPrivate,
+                   principal);

in the patch. Why are we switching in the onImage case from isPrivate to isContentWindowPrivate but not in the former case (the latter onVideo or orAudio case already had that before)? If that's correct and not just an oversight, it might be worth commenting it.

That was an oversight, and I have corrected it.

2) From looking at he code in ContentClick.jsm It seems we might be able to trigger window.openLinkIn(json.href, where, params); which could lead to false FPI in the save case (see the: // Todo(903022): code for where == save) or is that just a leftover comment and we are actually good?

This was a good catch. I found I needed to patch the saveURL function in browser/base/content/utilityOverlay.js.

3) What about

            ContentAreaUtils.saveImageURL(aTarget.currentRequestFinalURI.spec, null, "SaveImageTitle",
                                          false, true, aTarget.ownerDocument.documentURIObject,
                                          aTarget.ownerDocument);

in mobile/android/chrome/content/browser.js? *If* we can hit it it should get amended as well for FPI reasons I think (maybe even without that just to be on the safe side). Maybe igt0/sysrqb have some insight here. I did not look closer during the review.

Another good find. I patched that file.

4) Where is the new Ci needed in contentAreaUtils.js (see:

+function saveDocument(aDocument, aSkipPrompt, aContentPrincipal) {
+  const Ci = Components.interfaces;
+

It seems to me it is some leftover as Ci is already available in that .js file.

True -- I removed that unnecessary line.

5) loadingPrincipal : aContentPrincipal,: the two whitespaces before the : should go.

Fixed.

6) Maybe add some comment to

+  attribute nsIPrincipal loadingPrincipal;

in the .idl file given that everything else in that file does get a comment explaining what those items are about?

Done, thanks.

Last edited 4 weeks ago by arthuredelstein (previous) (diff)

comment:60 Changed 4 weeks ago by gk

Keywords: TorBrowserTeam201811R added; TorBrowserTeam201811 removed

comment:61 in reply to:  59 ; Changed 4 weeks ago by gk

Looks good to me now, thanks!

Replying to arthuredelstein:

Replying to gk:

2) From looking at he code in ContentClick.jsm It seems we might be able to trigger window.openLinkIn(json.href, where, params); which could lead to false FPI in the save case (see the: // Todo(903022): code for where == save) or is that just a leftover comment and we are actually good?

This was a good catch. I found I needed to patch the saveURL function in browser/base/content/utilityOverlay.js.

Where you able to trigger this bug in a browsing sesssion? If so, how? I tried quite a bit to verify my suspicion after reading the code, but failed.

comment:62 in reply to:  61 Changed 4 weeks ago by arthuredelstein

Replying to gk:

Replying to arthuredelstein:

Replying to gk:

2) From looking at he code in ContentClick.jsm It seems we might be able to trigger window.openLinkIn(json.href, where, params); which could lead to false FPI in the save case (see the: // Todo(903022): code for where == save) or is that just a leftover comment and we are actually good?

This was a good catch. I found I needed to patch the saveURL function in browser/base/content/utilityOverlay.js.

Where you able to trigger this bug in a browsing sesssion? If so, how? I tried quite a bit to verify my suspicion after reading the code, but failed.

Actually, I wasn't. I can try further. Regardless, I think we should patch the file because the signature of saveURL has changed.

comment:63 Changed 4 weeks ago by mcs

r=brade, r=mcs if you add a closing ')' to the first saveURL() call inside openLinkIn(). It definitely would be good to exercise that code.

comment:64 Changed 4 weeks ago by gk

Keywords: TorBrowserTeam201811 added; TorBrowserTeam201811R removed
Status: needs_reviewneeds_revision

comment:65 in reply to:  63 Changed 4 weeks ago by arthuredelstein

Status: needs_revisionneeds_review

Replying to mcs:

r=brade, r=mcs if you add a closing ')' to the first saveURL() call inside openLinkIn(). It definitely would be good to exercise that code.

Phooey, thanks for catching that mistake. Here's the revised version:
https://github.com/arthuredelstein/tor-browser/commit/22343+13

The if (where == "save") block in that function seems likely to be dead code. But I am working on a script to exercise it and will report results here.

comment:66 Changed 4 weeks ago by arthuredelstein

Keywords: TorBrowserTeam201811R added; TorBrowserTeam201811 removed

Here's my manual test for confirming that the openLinkIn() function works:

First, I set extensions.torbutton.loglevel to 3. Then, I navigated to https://torpat.ch. Next, I entered the following code in the browser console:

params = {
  fromChrome: true,
  allowThirdPartyFixup: true,
  originPrincipal: gBrowser.selectedBrowser.contentPrincipal,
  isContentWindowPrivate: true
}

openLinkIn("https://torpat.ch", "save", params);

A "File Save" dialog opened. I chose a location to save the file, and then in the browser console I saw the following result:

undefined
[11-15 21:09:26] Torbutton INFO: tor SOCKS: https://arthuredelstein.net/ via
                       torpat.ch:a6ba48c98fcb2812f029759c19313d35

So it appears the function is working and correctly uses the principal passed to it.

comment:67 Changed 4 weeks ago by gk

Resolution: fixed
Status: needs_reviewclosed

Thanks. And thanks to mcs for catching my blindness. I cherry-picked the patch to tor-browser-60.3.0esr-8.5-1 (commit c2b7e89ecaf3ea6a14a650a0a71c6dc3ebb02c50).

Arthur: Could you file an uplift ticket, so Mozilla and we don't lose track of this patch?

comment:68 in reply to:  67 Changed 3 weeks ago by arthuredelstein

Replying to gk:

Thanks. And thanks to mcs for catching my blindness. I cherry-picked the patch to tor-browser-60.3.0esr-8.5-1 (commit c2b7e89ecaf3ea6a14a650a0a71c6dc3ebb02c50).

Thanks!

Arthur: Could you file an uplift ticket, so Mozilla and we don't lose track of this patch?

I opened https://bugzilla.mozilla.org/show_bug.cgi?id=1508355

Note: See TracTickets for help on using tickets.