Closed Bug 1506693 Opened Last year Closed 6 months ago

PDFJS range-based requests violate FPI

Categories

(Firefox :: PDF Viewer, defect)

Desktop
All
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 68
Tracking Status
firefox68 --- fixed

People

(Reporter: richard, Assigned: richard)

References

(Blocks 1 open bug)

Details

(Whiteboard: [tor 26540])

Attachments

(3 files, 3 obsolete files)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:63.0) Gecko/20100101 Firefox/63.0

Steps to reproduce:

Range-based XMLHttpRequests in PDFJS are made in the background from a System/Chrome context, so their underlying channels do not have the correct OriginAttributes of the owning window.
OS: Unspecified → All
Hardware: Unspecified → Desktop
Fix made in ESR60-based tor-browser: https://gitweb.torproject.org/user/richard/tor-browser.git/commit/?h=bug_26540_v4&id=37b0bbc8bd0d69ef0b8f01a6d02cf9b809606634

Currently verifying if this issue persists in latest versions.
Attached patch 1506693.patch (obsolete) — Splinter Review
Updated esr60 patch to latest in gecko-dev/master
Component: Untriaged → PDF Viewer
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Hi, bdahl

Would you be able to help on reviewing Richard's patch?
Flags: needinfo?(bdahl)
Flags: needinfo?(bdahl)
Attachment #9024597 - Flags: review?(bdahl)
Whiteboard: [tor 26540]
Is there known case where this was causing issues?
Flags: needinfo?(richard)
Tor Browser uses the firstPartyDomain on a channel's originAttributes to determine which tor circuit to route traffic over ( see https://gitweb.torproject.org/user/richard/torbutton.git/tree/src/components/domain-isolator.js?h=bug_25702_v2#n134 ). Without this patch, the XMLHttpRequests used to download pdf chunks do not have *any* firstPartyDomain set, so Tor Browser must send said traffic over a default catchall circuit, while the initial HTTP request does have the firstPartyDomain set.  As a result, an adversary can determine that traffic coming from the 'catchall' circuit and from the correctly isolated circuit are the same user.
Flags: needinfo?(richard)
Attachment #9024597 - Flags: review?(bdahl) → review+

Hi Richard,
Are you planning to check this into mozilla central?

Flags: needinfo?(richard)

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:richard, could you have a look please?

Flags: needinfo?(richard)

(In reply to Brendan Dahl [:bdahl] Away until Mar 29th from comment #6)

Hi Richard,
Are you planning to check this into mozilla central?

Hi Brendan,

I don't have commit access (to my knowledge at least) but it would be great if this patch could get landed.

best,
-Richard

Flags: needinfo?(richard)

(In reply to Richard Pospesel (Tor Browser Dev) from comment #8)

I don't have commit access (to my knowledge at least) but it would be great if this patch could get landed.

Hey Richard, you don't need commit access. Once the patch is r+'d, you can set "checkin-needed" in the Keywords field, and people who have the access will land the patch for you.

Since the patch has been sitting idle for 4 months, it's better that you verify the patch is still valid. If you need to rebase it, please ask for review again.

Flags: needinfo?(richard)

Ah my apologies. Verified the posted patch still applies to esr60 branch of gecko-dev and added required keyword.

Flags: needinfo?(richard)
Keywords: checkin-needed

Pushed by dvarga@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a64efd0aa9c1
PDFJS range-based requests violate FPI. r=bdahl

Keywords: checkin-needed

Backed out changeset a64efd0aa9c1 (Bug 1506693) for linting failure at /builds/worker/checkouts/gecko/browser/extensions/pdfjs/content/PdfStreamConverter.jsm:224:3. on a CLOSED TREE

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=a64efd0aa9c1f1ed87c80108ea0e395374637570

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=236221383&repo=mozilla-inbound&lineNumber=268

Backout link: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=8ce913be1b325fddaaf1299a42a5f648c3128614

[task 2019-03-26T22:15:45.854Z] New python executable in /builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/init/bin/python2.7
[task 2019-03-26T22:15:45.854Z] Also creating executable in /builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/init/bin/python
[task 2019-03-26T22:15:47.514Z] Installing setuptools, pip, wheel...done.
[task 2019-03-26T22:15:48.562Z] running build_ext
[task 2019-03-26T22:15:48.562Z] building 'psutil._psutil_linux' extension
[task 2019-03-26T22:15:48.562Z] creating build
[task 2019-03-26T22:15:48.562Z] creating build/temp.linux-x86_64-2.7
[task 2019-03-26T22:15:48.562Z] creating build/temp.linux-x86_64-2.7/psutil
[task 2019-03-26T22:15:48.562Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_common.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_common.o
[task 2019-03-26T22:15:48.562Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_posix.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o
[task 2019-03-26T22:15:48.562Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_linux.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_linux.o
[task 2019-03-26T22:15:48.562Z] creating build/lib.linux-x86_64-2.7
[task 2019-03-26T22:15:48.562Z] creating build/lib.linux-x86_64-2.7/psutil
[task 2019-03-26T22:15:48.562Z] x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-Bsymbolic-functions -Wl,-z,relro -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/psutil/_psutil_common.o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o build/temp.linux-x86_64-2.7/psutil/_psutil_linux.o -o build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so
[task 2019-03-26T22:15:48.562Z] building 'psutil._psutil_posix' extension
[task 2019-03-26T22:15:48.562Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_common.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_common.o
[task 2019-03-26T22:15:48.562Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_posix.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o
[task 2019-03-26T22:15:48.562Z] x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-Bsymbolic-functions -Wl,-z,relro -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/psutil/_psutil_common.o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o -o build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so
[task 2019-03-26T22:15:48.562Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so -> psutil
[task 2019-03-26T22:15:48.562Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so -> psutil
[task 2019-03-26T22:15:48.562Z]
[task 2019-03-26T22:15:48.562Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2019-03-26T22:21:36.749Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/extensions/pdfjs/content/PdfStreamConverter.jsm:224:3 | Opening curly brace does not appear on the same line as controlling statement. (brace-style)
[task 2019-03-26T22:21:36.749Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/extensions/pdfjs/content/PdfStreamConverter.jsm:227:7 | Expected space(s) after "catch". (keyword-spacing)
[taskcluster 2019-03-26 22:21:37.255Z] === Task Finished ===

Flags: needinfo?(richard)

I noticed this patch is for esr60, which will cause a bit of a disconnect: checkin-needed is only for checkins to -central. The right process to follow is:

  1. Attach a patch that can apply to -central (which this does, by luck alone I think)
  2. (Ensure linting and tests pass - this one failed with

TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/extensions/pdfjs/content/PdfStreamConverter.jsm:224:3 | Opening curly brace does not appear on the same line as controlling statement. (brace-style)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/extensions/pdfjs/content/PdfStreamConverter.jsm:227:7 | Expected space(s) after "catch". (keyword-spacing)

  1. Get reviews
  2. Mark as checkin-needed

Then if you want to land something in esr60:

  1. (Can be done immediately after #4 you don't have to wait) Attach another patch for esr60 (SKIP if the same patch applies cleanly)
  2. Click on the 'Details' link of the patch and mark the esr60 patch as 'approval‑mozilla‑esr60 -> ?'
  3. Fill out the form (Here's a random example https://bugzilla.mozilla.org/show_bug.cgi?id=1520310#c14 but you'll need to fill out 'Risk' yourself)
  4. Some number of days later a Release Manager will review the esr60 application and grant and land the patch, or follow up with questions

If you only want to land something in esr60 - which should be done rarely - You skip #1 and #4.

Attached patch 1506693_01.patchSplinter Review

Fixed whitespace issues in previous patch

Attachment #9024597 - Attachment is obsolete: true
Flags: needinfo?(richard)
Attachment #9054640 - Flags: review+
Version: 60 Branch → Trunk

Reposted patch as a phabricator diff:

https://phabricator.services.mozilla.com/D29727

If possible we'd like to get this into ESR68. I added bdahl as reviewer since they did the first review.

best,
-Richard

Flags: needinfo?(bdahl)
Keywords: checkin-needed

Landing is blocked.

Flags: needinfo?(richard)
Keywords: checkin-needed

Andreea,

What info is needed? Phabricator says 'This revision is now accepted and ready to land" as of May 7th.

thanks,
-Richard

Flags: needinfo?(richard)
Flags: needinfo?(apavel)
Flags: needinfo?(apavel) → needinfo?(richard)
Flags: needinfo?(richard)

uploaded wrong patch

Attachment #9064113 - Attachment is obsolete: true

It looks like phabricator stripped out my author info and commit info when I uploaded it, so here it is again. moz-phab doesn't seem to like git, so I'll try applying the patch to the mercurial version of gecko-dev and see how that goes.

Strange, locally I use moz-phab with git(cinnabar) and things are fine. Though I started from https://github.com/glandium/git-cinnabar/wiki/Mozilla:-A-git-workflow-for-Gecko-development

Flags: needinfo?(bdahl)

Large pdf files download in parts via range-based requests so that
users can begin reading before the entire file has finished
downloading. This is implemented using XMLHttpRequests. However, since
these requests are created in the chrome, they are given the System
Principal and lack the correct firstPartyDomain associated with the
parent window.

This patch manually sets the XMLHttpRequest's originAttributes to the
one provided by the real owning window cached in the
RangedChromeActions object. This is done via the chrome-only
setOriginAttributes method.

The method is called in the xhr_onreadystatechanged() callback rather
than directly after construction in getXhr() because the
setOriginAttributes implementation requires the internal nsIChannel
object to have been created but not used. Fortunately, the
XMLHttpRequest object fires the readStateChangedEvent precisely after
the channel has been created in the XmlHttpRequest's Open() method.

The nsIChannel's nsILoadInfo's OriginAttributes are now overwritten
with the known OriginAttributes of the parent window before anything
else has had a chance to use it.

Ok applied the patch to the hg mozilla-central and now moz-phab seemed happy with it, here's the phabricator link:

https://phabricator.services.mozilla.com/D30689

Flags: needinfo?(apavel)

Hi Richard, for the moment this cannot land because it does not have review. Will leave the ni for me so I can check on in and land it after it has the review+. Thanks for looking into it.

Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b1c78bd9fdc2
PDFJS range-based requests violate FPI r=bdahl
Flags: needinfo?(apavel)
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
Attachment #9062305 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.