Opened 6 months ago

Closed 4 weeks ago

Last modified 4 weeks ago

#26540 closed defect (fixed)

Enabling pdfjs disableRange option prevents pdfs from loading

Reported by: pospeselr Owned by: pospeselr
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: ff60-esr, TorBrowserTeam201811R, tbb-backport
Cc: tbb-team Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In ESR52, range-based requests in pdf.js (which allows pdfs to render-as-load) go out on the default circuit as the relevant code was running in the System security context. To 'fix' the issue, we simply disabled range-based request with the 'pdfjs.disableRange' option. However, in ESR60 based Tor Browser, enabling this option prevents PDFs from loading at all. Without the option enabled, PDFs load and render, but the range-based requests still go out on the default circuit.

Original bug: https://trac.torproject.org/projects/tor/ticket/15599

Child Tickets

Change History (26)

comment:1 Changed 6 months ago by gk

Status: assignedneeds_information

Well, flipping that preference has been an experiment. And I am not convinced yet that the usability impact was worth it. Thus, I wonder if we should just abandon this "fix" idea and try to look again in a new ticket at dealing with the original linkability concern? Maybe ESR60 makes it easier now to isolate those range requests properly? What do you think?

comment:2 Changed 6 months ago by cypherpunks

FPI FTW!

comment:3 in reply to:  1 Changed 6 months ago by pospeselr

Replying to gk:

Well, flipping that preference has been an experiment. And I am not convinced yet that the usability impact was worth it. Thus, I wonder if we should just abandon this "fix" idea and try to look again in a new ticket at dealing with the original linkability concern? Maybe ESR60 makes it easier now to isolate those range requests properly? What do you think?

Yeah I'm inclined to agree. For the time being, I'm investigating whether we can enable FPI for these range requests .

comment:4 Changed 5 months ago by gk

Keywords: TorBrowserTeam201807 added; TorBrowserTeam201806 removed

comment:5 Changed 5 months ago by pospeselr

Keywords: TorBrowserTeam201807R added; TorBrowserTeam201807 removed
Status: needs_informationneeds_review

Two patches, one for tor-browser and one for torbutton.

The patches take an approach of 'smuggling' the first party domain on the existing nsIPrivateBrowsingChannel used by XMLHttpRequest. Basically, the first-party domain is known when the range-based request is created, but since it's created from within chrome js code, it gets the System Principal which throws out all that info. So, in pdfjs we set the firstPartyDomain on the channel object which is then read by torbutton. If torbutton fails to find a firstPartyDomain in the usual way from the OriginAttributes, it will try to read it off of the channel directly.

With this smuggling hack in place, we should be able to fix any other 'XMLHttpRequest-created-in-System-Principal' first-party isolation issues we come across.

Currently doing an RBM build with the patches applied just to make sure it all works as expected without hacks.

EDIT: Looks like this patch only works when browser.tabs.remote.autostart is false (disables the separate content process). Will have to dig into this and find the other XMLHttpRequest constructors in pdfjs which shouldn't be to tricky.

Last edited 5 months ago by pospeselr (previous) (diff)

comment:6 Changed 5 months ago by pospeselr

So this updated patch shows the general approach here of smuggling the first-party domain on the channel object directly as a fallback in case the domain stored on the origin attributes is empty-string (as is the case for XMLHttpRequests made under the System Principal.

I think the final patch (assuming y'all are ok with this approach) would stow the smuggled first-party domain on the nsIHttpChannel, rather than the nsIPrivateBrowsingChannel (since almost everything implements the private browsing interface, but we only need it on the http channel).

comment:7 Changed 5 months ago by gk

Keywords: TorBrowserTeam201807 added; TorBrowserTeam201807R removed
Status: needs_reviewneeds_revision

Neat idea! I am fine doing this hack if that helps us. Note, we want to have domain isolation working even if the user is not in PBM as PBM is only used to take care of disk leaks. The pref governing the domain isolation is privacy.firstparty.isolate.

It might be worth as well to open a Mozilla bug, blocking the FirstPartyIsolation one (https://bugzilla.mozilla.org/show_bug.cgi?id=1299996) and asking Mozilla for feedback wrt the final approach you come up with.

Finally, it seems the patches you currently have do not work for me when trying to load https://www.amnestyusa.org/pdfs/sscistudy1.pdf

STR:

1) Take a vanilla 8.0a9 bundle on Linux
2) Override the bundle parts with the patched code
3) After start-up set extensions.torbutton.loglevel to 3 and extensions.torbutton.logmethod to 0
4) Try to load https://www.amnestyusa.org/pdfs/sscistudy1.pdf
5) During CAPTCHA completion you'll see the isolation to the Amnesty domain
6) Once you CAPTCH got completed and the .pdf is loading this seems to go over the catch-all curcuit.

comment:9 Changed 4 months ago by gk

Keywords: TorBrowserTeam201808 added; TorBrowserTeam201807 removed

Move our tickets to August.

comment:10 Changed 4 months ago by gk

FWIW: We got reports on the blog that disableRange is still enabled in the alpha series after updating from 8.0a8. I guess the pref we flipped is kept because it is essentially a user set pref. I think we should try to get that solved as part of this patch: on the alpha series we should try to (re)set the behavior we want again.

comment:11 Changed 3 months ago by gk

Keywords: TorBrowserTeam201809 added; TorBrowserTeam201808 removed

Moving our tickets to September 2018

comment:12 Changed 3 months ago by pospeselr

Keywords: TorBrowserTeam201809R added; TorBrowserTeam201809 removed
Status: needs_revisionneeds_review

Updated tor-browser patch: https://gitweb.torproject.org/user/richard/tor-browser.git/commit/?h=bug_26540_v2

Moved the smuggled first party domain over to the nsIHttpChannel

Updated torbutton patch: https://gitweb.torproject.org/user/richard/torbutton.git/commit/?h=bug_26540_v2

Now looking for an nsIHttpChannel

comment:13 Changed 2 months ago by gk

Keywords: TorBrowserTeam201810R added; TorBrowserTeam201809R removed

Moving review tickets to October

comment:14 Changed 2 months ago by arthuredelstein

Status: needs_reviewneeds_revision

I wonder if we could use Firefox's standard approach to passing around origin attributes inside the "principal". Currently, in the

download(data, sendResponse)

function definition in PdfStreamConverter.jsm, the channel is created as

    var netChannel = NetUtil.newChannel({
      uri: blobUri,
      loadUsingSystemPrincipal: true,
    });

but I think we could change this to:

    var netChannel = NetUtil.newChannel({
      uri: blobUri,
      loadingPrincipal: this.domWindow.document.nodePrincipal,
    });

I haven't tested this, but I expect it would result in

  channel.loadInfo.originAttributes.firstPartyDomain

returning the document's firstPartyDomain, as needed in torbutton domain isolator. With this approach (assuming it works) we wouldn't need to alter channel classes in Firefox or patch torbutton.

comment:15 Changed 8 weeks ago by gk

Keywords: TorBrowserTeam201810 added; TorBrowserTeam201810R removed

comment:16 Changed 6 weeks ago by pospeselr

Ok, so in PdfStreamConverter.jsm there are two places where channels are created: PdfStreamConverter::onStartRequest (which seems to load the pdf.js viewer itself: "resource://pdf.js/web/viewer.html") and ChromeActions::download which appears to be dead code; breakpoints placed there never get hit and no call to 'download' function seems to exist in the source, but it's JavaScript so who knows what fancy bs it's pulling.

The range-based requests occur via an XMLHttpRequest whose channel is created internally in one of the constructors/factory methods in the C++ source, so we don't seem to have access to it at construction and it can't be replaced with our own channel with the correct principal set.

HOWEVER!

It seems that while we cannot simply overwrite the channel's originAttribute's firstpartyDomain and have it stick, we can simply overwrite the channel's entire originAttributes. I've prototyped doing so in the debugger and it would seem that everything works as expected. I'll get a patch written and tested tomorrow and let y'all know how it goes.

comment:17 Changed 6 weeks ago by pospeselr

Keywords: TorBrowserTeam201810R added; TorBrowserTeam201810 removed
Status: needs_revisionneeds_review

Updated with much simpler patch, no change tor-button is necessary:

https://gitweb.torproject.org/user/richard/tor-browser.git/commit/?h=bug_26540_v3

comment:18 Changed 5 weeks ago by gk

Keywords: TorBrowserTeam201811R added; TorBrowserTeam201810R removed

Moving reviews to November.

comment:19 Changed 5 weeks ago by pospeselr

Some concerns arthuredelestein brought up on irc:

(11/01/2018 03:47:14 PM) arthuredelstein: hi! your new patch for #26540 looks good! I'm wondering though, about cache and cookies and things like that.
(11/01/2018 03:47:25 PM) arthuredelstein: Does setting the originAttributes on loadinfo take care of all of that?
(11/01/2018 03:47:41 PM) arthuredelstein: (I guess the range request pieces might not be cached, not sure.)
(11/01/2018 03:47:54 PM) arthuredelstein: But, for example, if there's a cookie header, does that get stored in the right place?
(11/01/2018 03:48:02 PM) arthuredelstein: I don't know how to easily test that.

comment:20 Changed 5 weeks ago by pospeselr

Better patch following suggestions from Arthur, now using the secret chrome-only SetOriginAttributes function:

https://gitweb.torproject.org/user/richard/tor-browser.git/commit/?h=bug_26540_v4

The origin attributes are set immediately after channel creation, FPI regarding cookies is now enforced correctly, and pdfjs's range-based requests now go out on the correct channel (the channel for the hosted pdf's domain if entered directly in the title bar, and the first party domain's when embedded via an iframe or embed node).

Verified on jsbin.com with the following html:

<!DOCTYPE html>
<html>
<body>
  <iframe src="http://cadwe.free.fr/cadr/DD4/Player%27s%20Handbook.pdf"></iframe>
</body>
</html>

EDIT: Updated patch with input from gk: https://gitweb.torproject.org/user/richard/tor-browser.git/commit/?h=bug_26540_v4&id=37b0bbc8bd0d69ef0b8f01a6d02cf9b809606634

Last edited 5 weeks ago by pospeselr (previous) (diff)

comment:21 Changed 4 weeks ago by gk

Resolution: fixed
Status: needs_reviewclosed

Okay, this looks promising. I cherry-picked the fixed up commit to tor-browser-60.3.0esr-8.5-1.

Could you file an upstream bug blocking (https://bugzilla.mozilla.org/show_bug.cgi?id=1299996) and feel free to just try to get it upstreamed right now. I hope that won't be that hard and delaying work on #3600.

Oh, and while you are at it: while testing your patch I got tons (i.e. hundreds) of

Attempting to post a message to window with url
"resource://pdf.js/web/viewer.html" and origin
"resource://pdf.js^firstPartyDomain=amnestyusa.org" from a system principal
scope with mismatched origin "[System Principal]"

messages in my browser console. Those are not related to your patch, but could you file a FPI bug blocking the above one as well (or maybe the FirstPartyIsolationQA bug, I am not sure)? I have not checked whether those messages are actually a sign of something that is deeper wrong or whether it's just console spam.

comment:22 Changed 4 weeks ago by gk

Keywords: tbb-backport added

comment:23 in reply to:  21 Changed 4 weeks ago by tom

Replying to gk:

Could you file an upstream bug blocking (https://bugzilla.mozilla.org/show_bug.cgi?id=1299996) and feel free to just try to get it upstreamed right now. I hope that won't be that hard and delaying work on #3600.

Please cc me!

Oh, and while you are at it: while testing your patch I got tons (i.e. hundreds) of

Attempting to post a message to window with url
"resource://pdf.js/web/viewer.html" and origin
"resource://pdf.js^firstPartyDomain=amnestyusa.org" from a system principal
scope with mismatched origin "[System Principal]"

messages in my browser console. Those are not related to your patch, but could you file a FPI bug blocking the above one as well (or maybe the FirstPartyIsolationQA bug, I am not sure)? I have not checked whether those messages are actually a sign of something that is deeper wrong or whether it's just console spam.

This is https://bugzilla.mozilla.org/show_bug.cgi?id=1495204 ! =)

comment:24 in reply to:  21 Changed 4 weeks ago by pospeselr

Replying to gk:

Could you file an upstream bug blocking (https://bugzilla.mozilla.org/show_bug.cgi?id=1299996) and feel free to just try to get it upstreamed right now.

Done: https://bugzilla.mozilla.org/show_bug.cgi?id=1506693

Verifying issue still exists in firefox latest.

Note: See TracTickets for help on using tickets.