Enabling pdfjs disableRange option prevents pdfs from loading
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
Activity
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?
Trac:
Status: assigned to needs_information
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 .
-
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.
Trac:
Status: needs_information to needs_review
Keywords: TorBrowserTeam201807 deleted, TorBrowserTeam201807R added -
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).
-
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:
- Take a vanilla 8.0a9 bundle on Linux
- Override the bundle parts with the patched code
- After start-up set
extensions.torbutton.loglevelto3andextensions.torbutton.logmethodto0 - Try to load https://www.amnestyusa.org/pdfs/sscistudy1.pdf
- During CAPTCHA completion you'll see the isolation to the Amnesty domain
- Once you CAPTCH got completed and the .pdf is loading this seems to go over the catch-all curcuit.
Trac:
Status: needs_review to needs_revision
Keywords: TorBrowserTeam201807R deleted, TorBrowserTeam201807 added -
tor-browser changes:
torbutton changes:
-
FWIW: We got reports on the blog that
disableRangeis 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. -
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
Trac:
Keywords: TorBrowserTeam201809 deleted, TorBrowserTeam201809R added
Status: needs_revision to needs_review 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 asvar 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.firstPartyDomainreturning 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.
Trac:
Status: needs_review to needs_revision-
Ok, so in PdfStreamConverter.jsm there are two places where channels are created: PdfStreamConverter::onStartRequest (which seems to load the pdf.js viewer itself: "re/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.
-
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
Trac:
Status: needs_revision to needs_review
Keywords: TorBrowserTeam201810 deleted, TorBrowserTeam201810R added -
Some concerns arthuredelestein brought up on irc:
(11/01/2018 03:47:14 PM) arthuredelstein: hi! your new patch for #26540 (moved) 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.
-
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
-
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 (moved).
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 "re[/pdf.js/web/viewer.html"](/pdf.js/web/viewer.html") and origin "re[/pdf.js^firstPartyDomain=amnestyusa.org"](/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.
Trac:
Status: needs_review to closed
Resolution: N/A to fixed 
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 (moved).
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 "re/pdf.js/web/viewer.html" and origin "re/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 ! =)
-
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.