1506693 - PDFJS range-based requests violate FPI

Status: RESOLVED FIXED

(Firefox :: PDF Viewer, defect)

Description

[richard (Tor Project)]

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.

Comment 1

[richard (Tor Project)]

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.

Comment 2

[richard (Tor Project)]

Attachment: 1506693.patch

Updated esr60 patch to latest in gecko-dev/master

Comment 3

[Tim Huang[:timhuang]]

Hi, bdahl

Would you be able to help on reviewing Richard's patch?

Comment 4

[Brendan Dahl [:bdahl]]

Is there known case where this was causing issues?

Comment 5

[richard (Tor Project)]

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.

Comment 6

[Brendan Dahl [:bdahl]]

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

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

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?

Comment 8

[richard (Tor Project)]

(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

Comment 9

[Ethan Tseng [:ethan]]

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

Comment 10

[richard (Tor Project)]

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a64efd0aa9c1
PDFJS range-based requests violate FPI. r=bdahl

Comment 12

[Daniel Varga [:dvarga]]

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

Comment 13

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

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)

3. Get reviews
4. Mark as checkin-needed

Then if you want to land something in esr60:

5. (Can be done immediately after #4 you don't have to wait) Attach another patch for esr60 (SKIP if the same patch applies cleanly)
6. Click on the 'Details' link of the patch and mark the esr60 patch as 'approval‑mozilla‑esr60 -> ?'
7. 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)
8. 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.

Comment 14

[richard (Tor Project)]

Attachment: 1506693_01.patch

Fixed whitespace issues in previous patch

Comment 15

[richard (Tor Project)]

Attachment: Reposted patch for 1506693 : PDFJS range-based requests violate FPI

Comment 16

[richard (Tor Project)]

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

Comment 17

[Andreea Pavel [:apavel]]

Landing is blocked.

Comment 18

[richard (Tor Project)]

Andreea,

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

thanks,
-Richard

Comment 19

[Andreea Pavel [:apavel]]

Hi Richard, please see https://irccloud.mozilla.com/file/jtsuByZB/landing_blocked.png

Comment 20

[richard (Tor Project)]

Attachment: Updated patch with author info and change description

Comment 21

[richard (Tor Project)]

Attachment: 0001-Bug-1506693-PDFJS-range-based-requests-violate-FPI.patch

uploaded wrong patch

Comment 22

[richard (Tor Project)]

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.

Comment 23

[Brendan Dahl [:bdahl]]

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

Comment 24

[richard (Tor Project)]

Attachment: Bug 1506693: PDFJS range-based requests violate FPI

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.

Comment 25

[richard (Tor Project)]

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

Comment 26

[Andreea Pavel [: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.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b1c78bd9fdc2
PDFJS range-based requests violate FPI r=bdahl

Comment 28

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/b1c78bd9fdc2