Opened 3 years ago

Closed 7 months ago

#15599 closed defect (fixed)

Range requests used by pdfjs are not isolated to URL bar domain

Reported by: gk Owned by: pospeselr
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-linkability, TorBrowserTeam201802R
Cc: arthuredelstein, mcs, brade Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by gk)

If a server sends the Accept-Ranges header + a (proper) Content-Length Tor Browser is starting range requests that are not isolated to the URL bar domain. You can test this e.g. with https://kpdyer.com/publications/usenix2014-fte.pdf. Works even in a third party context with https://people.torproject.org/~gk/misc/range-request-test.html (your security slider level needs to be below medium-high in this case).

Child Tickets

Attachments (1)

0001-Bug-15599-Range-requests-used-by-pdfjs-are-not-isola.patch (3.7 KB) - added by pospeselr 8 months ago.
updated description to be grammatical

Download all attachments as: .zip

Change History (23)

comment:1 Changed 3 years ago by gk

Description: modified (diff)

I think this is due to

getFirstPartyURI failed for https://kpdyer.com/publications/usenix2014-fte.pdf

I wonder if we could get that fixed last-minute for 4.5.

comment:2 Changed 3 years ago by gk

Summary: Range requests are not isolated to URL -bar domainRange requests are not isolated to URL bar domain

comment:3 Changed 3 years ago by mikeperry

Is this specific to PDFs, I wonder? It is possible that getFirstPartyURI is failing here not just because of the range requests but because pdf.js is fetching portions of the file in that case. It might not happen with all PDFs, depending on how pdf.js decides to do rendering.

comment:4 Changed 3 years ago by gk

I was wondering that, too. There are definitely larger pdfs that get rendered without range requests. Then there are large non-pdf downloads where the server is sending an Accept-Ranges header but no range requests are issued by the browser. So far, I don't have non-pdf range requests seen which would clarify if that is purely pdf related.

Last edited 3 years ago by gk (previous) (diff)

comment:5 Changed 3 years ago by gk

Summary: Range requests are not isolated to URL bar domainRange requests used by pdfjs are not isolated to URL bar domain

Retitling this bug as it seems only range requests issued by pdfjs have this problem. See, e.g. http://hpr.dogphilosophy.net/test/opus.opus or other tests on http://hpr.dogphilosophy.net/test/.

comment:6 Changed 3 years ago by gk

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

comment:7 Changed 3 years ago by gk

Keywords: tbb-4.5-alpha removed
Owner: changed from gk to tbb-team

We could disable range requests easily in pdfjs but that is affecting rendering speed with unknown impact. Thus, this is no great idea to do without testing at least in one alpha. Fixing the root cause of the problem, an extension issuing XHRs and thus having no window associated to the channel, is no option for an untested stable patch either. Therefore, removing this issue from the 4.5 radar. :(

comment:8 Changed 2 years ago by bugzilla

Severity: Normal

PDFs, which are not affected, are going through catchall circuit too when PDF Download button is pressed.
UPDATE: this is one of two issues of #17933.

Last edited 2 years ago by bugzilla (previous) (diff)

comment:9 in reply to:  description Changed 16 months ago by cypherpunks

And its OCSP requests too:

[05-29 18:16:40] Torbutton INFO: tor SOCKS: http://ocsp.usertrust.com/ via
                       --unknown--:acc796c227a065d5b876d251f00beb87

Replying to gk:

Works even in a third party context with https://people.torproject.org/~gk/misc/range-request-test.html (your security slider level needs to be below medium-high in this case).

Security Error: Content at https://kpdyer.com/publications/usenix2014-fte.pdf#disableRange=true may not load data from https://people.torproject.org/~gk/misc/range-request-test.html.
Load denied by X-Frame-Options: https://kpdyer.com/publications/usenix2014-fte.pdf#disableRange=true does not permit cross-origin framing.  (unknown)

Hrm, does PDF.js support Private Browsing Mode?

comment:10 Changed 9 months ago by gk

Keywords: TorBrowserTeam201712 added
Owner: changed from tbb-team to pospeselr

comment:11 Changed 9 months ago by gk

Keywords: TorBrowserTeam201801 added; TorBrowserTeam201712 removed

Moving tickets to 2018.

comment:12 Changed 8 months ago by pospeselr

Keywords: TorBrowserTeam201801R added; TorBrowserTeam201801 removed
Status: assignedneeds_review

Patch to disable range-based requests in pdf.js. Fixes the domain isolation issue, at the expense of usability. With this pref flipped, the entire pdf must be downloaded before being viewed and interacted with.

comment:13 Changed 8 months ago by gk

Keywords: TorBrowserTeam201802 added; TorBrowserTeam201801R removed
Status: needs_reviewneeds_revision

Okay, let's try that in the alpha series a bit. I admit, though, I am a bit skeptical whether the possible usability issues are worth it. We'll see.

But before that, two things:

1) s/cannot/can/ (it seems to me one negation is already enough :) )
2) extension-overrides.js is the wrong place for the patch. We don't treat pdf.js as en extension but rather as part of the browse core (after all it does not show up in the about:addons menu etc.). So, our usual prefs file, 000-tor-browser.js, would be the better place.

comment:14 in reply to:  13 ; Changed 8 months ago by pospeselr

Unfortunately, the user_pref setting doesn't seem to stick when placed in the usual 000-tor-browser.js, and it gets overwritten by pdfjs initialization code if specified in the usual fashion (verified with an rbm build).

Changed 8 months ago by pospeselr

updated description to be grammatical

comment:15 Changed 8 months ago by gk

Keywords: TorBrowserTeam201802R added; TorBrowserTeam201802 removed

comment:16 Changed 8 months ago by gk

Status: needs_revisionneeds_review

comment:17 in reply to:  14 ; Changed 7 months ago by mcs

Status: needs_reviewneeds_information

Replying to pospeselr:

Unfortunately, the user_pref setting doesn't seem to stick when placed in the usual 000-tor-browser.js, and it gets overwritten by pdfjs initialization code if specified in the usual fashion (verified with an rbm build).

I assume this is the code that is overriding the settings for pdfjs.disableRange when placed in 000-tor-browser.js:
https://dxr.mozilla.org/mozilla-esr52/source/browser/extensions/pdfjs/content/PdfJs.jsm#79

Maybe we should patch the above code instead and also add the the setting to 000-tor-browser.js as a reminder that we care about the value for pdfjs.disableRange. Probably gk should decide which approach we want to use.

How bad is performance when loading a large PDF with this change in place? I assume "time to first page display" increases significantly.

comment:18 in reply to:  17 Changed 7 months ago by pospeselr

Replying to mcs:

I assume this is the code that is overriding the settings for pdfjs.disableRange when placed in 000-tor-browser.js:
https://dxr.mozilla.org/mozilla-esr52/source/browser/extensions/pdfjs/content/PdfJs.jsm#79

Yep exactly, this code goes in and overwrites the preference unless it's been set by a user, so updating the default system preference does nothing in 000-tor-browser.js just gets blown away.

How bad is performance when loading a large PDF with this change in place? I assume "time to first page display" increases significantly.

Entirely dependent on how large the pdf is that you're trying to download, and how fast your circuit is. Fortunately there is a progress bar in the pdf UI indicating the load progress, so it doesn't look like the browser is just hanging.

comment:19 Changed 7 months ago by mcs

Status: needs_informationneeds_review

Back to needs_review so we don't lose track of this ticket (gk will hopefully give a final opinion when he is back at his keyboard).

comment:20 Changed 7 months ago by cypherpunks

Keywords: TorBrowserTeam201802 added; TorBrowserTeam201802R removed
Status: needs_reviewneeds_revision

2) extension-overrides.js is the wrong place for the patch.

@mcs too: see what you've done with e10srollout.

BTW, where do you see range requests now?

comment:21 Changed 7 months ago by gk

Keywords: TorBrowserTeam201802R added; TorBrowserTeam201802 removed
Status: needs_revisionneeds_review

comment:22 Changed 7 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Let's test the patch as-is in the alpha. I am skeptical that the benefit is worth the usability penalty. If we think it is okay then I think the patch idea brought up by mcs is the better one.

Applied to tor-browser-build's master as commit 7db15759a31a7381d0a43b1a40373cd9f970210a.

Note: See TracTickets for help on using tickets.