1450534 - Aborting load potentially exposes PDF Viewer APIs to webpages

Status: VERIFIED FIXED

(Firefox :: PDF Viewer, defect)

Description

[Wladimir Palant]

Attachment: Proof of concept page

This is a spin-off from bug 1449898, the discussion there made obvious that a second vulnerability exists in the way PdfStreamConverter attaches to the PDF Viewer. If loading is aborted, PdfStreamConverter might end up attaching to an about:blank document that the page has access to. This won't merely give the page to access the PDF file contents, it also gives it full access to the ChromeActions API exposed by PdfStreamConverter. This means for example reading or changing arbitrary preferences in the "pdfjs." namespace.

Steps to reproduce:

1. Open the attached proof of concept page.
2. Enter the URL of a PDF file into the text field and click "Download"
3. Wait a bit

When testing with PDF files that support ranged loading (such as https://www.blackhat.com/docs/eu-15/materials/eu-15-Vigo-Even-The-Lastpass-Will-Be-Stolen-deal-with-it.pdf), this page succeeds within a few seconds for me and displays the first 160 bytes of the PDF file. It does that by loading the PDF file in a hidden frame and aborting the load after a random interval. Eventually this will result in an about:blank document loaded in this frame with PdfStreamConverter attached to it - meaning that the page can send pdf.js.message events that PdfStreamConverter will act upon.

Tested in Firefox 59.0.2 and Firefox 61.0a1 (2018-03-31) nightly on Linux.

Comment 1

[:Gijs (he/him)]

This needs some investigation. We shouldn't be mixing up principals like this. Brendan, do you think you could poke at this more? If not, please could you ask someone who knows our network/principal stuff a bit, like :jkt or :ckerschb or :mayhemer or :bz to have a look?

Comment 2

[:Gijs (he/him)]

(For more context for other folks I just CC'd, see bug 1449898 comment 5 through 15)

Comment 3

[Brendan Dahl [:bdahl]]

(In reply to :Gijs (he/him) from comment #1)
> This needs some investigation. We shouldn't be mixing up principals like
> this. Brendan, do you think you could poke at this more? If not, please
> could you ask someone who knows our network/principal stuff a bit, like :jkt
> or :ckerschb or :mayhemer or :bz to have a look?

I'm not sure I follow what you want me to look into. The changes in bug 1449898 should address this issue too. In that patch we should now only attach the event listener to a window if the nodePrincipal matches pdf.js.

Comment 4

[:Gijs (he/him)]

(In reply to Brendan Dahl [:bdahl] from comment #3)
> (In reply to :Gijs (he/him) from comment #1)
> > This needs some investigation. We shouldn't be mixing up principals like
> > this. Brendan, do you think you could poke at this more? If not, please
> > could you ask someone who knows our network/principal stuff a bit, like :jkt
> > or :ckerschb or :mayhemer or :bz to have a look?
> 
> I'm not sure I follow what you want me to look into. The changes in bug
> 1449898 should address this issue too. In that patch we should now only
> attach the event listener to a window if the nodePrincipal matches pdf.js.

Oh. Because this was filed as a separate bug, I assumed that the patch was scoped down in order to spend more time looking at it. Having just re-read the patch and the comments again, I guess we just make this a dep and resolve when we fix the other bug.

Comment 5

[Brendan Dahl [:bdahl]]

Can someone confirm what happens in the following scenario:

1) we attach event listener to the pdf.js window
2) the document somehow changes/navigates
3) the event listener will NOT be listening to events from new document

I was told this was true, but want to make sure.

Comment 6

[:Gijs (he/him)]

(In reply to Brendan Dahl [:bdahl] from comment #5)
> Can someone confirm what happens in the following scenario:
> 
> 1) we attach event listener to the pdf.js window
> 2) the document somehow changes/navigates
> 3) the event listener will NOT be listening to events from new document
> 
> I was told this was true, but want to make sure.

I think it depends.

If you're content-privileged JS, that's true. Otherwise obviously we'd have issues with events being passed willy-nilly between origins that loaded in the same window.

If you're chrome-privileged JS... it's more complicated. If you're actually attaching an event listener to the *window* rather than the *document* (or any nodes in it), then the answer is "maybe". See e.g. https://dxr.mozilla.org/mozilla-central/rev/00bdc9451be6557ccce1492b9b966d4435615380/browser/base/content/browser.js#1169 and https://bugzilla.mozilla.org/show_bug.cgi?id=1348623

Comment 7

[Brendan Dahl [:bdahl]]

Hmm...so we should be using document here instead? https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/browser/extensions/pdfjs/content/PdfStreamConverter.jsm#955

Comment 8

[:Gijs (he/him)]

(In reply to Brendan Dahl [:bdahl] from comment #7)
> Hmm...so we should be using document here instead?
> https://searchfox.org/mozilla-central/rev/
> a0665934fa05158a5a943d4c8b277465910c029c/browser/extensions/pdfjs/content/
> PdfStreamConverter.jsm#955

I think it would be more explicit, but equally I am not an expert, so maybe you're OK anyway.  I just tried to see if I could get anything untoward to happen by adding a listener for a custom event from the devtools console in about:preferences and then loading about:mozilla (which is unprivileged) and dispatching an event in there, but I couldn't get my listener to fire.

So I'm still not sure under what circumstances the window listener sticks around when the document changes (per earlier references where it does do that, and then apparently for my testing above it doesn't...). :bz would probably know for sure.

Comment 9

[Boris Zbarsky [:bzbarsky]]

The only case when a window listener sticks around when the document changes is if the window itself sticks around.

And that only happens on navigation from an initial about:blank to some other page that is same-origin with that initial about:blank.

Does that help?

Comment 10

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #9)
> The only case when a window listener sticks around when the document changes
> is if the window itself sticks around.
> 
> And that only happens on navigation from an initial about:blank to some
> other page that is same-origin with that initial about:blank.
> 
> Does that help?

Yes, I think based on that the changes in bug 1449898 are sufficient to avoid any issues with the listener, though I'd also be happy to r+ moving it to the document if Brendan wants to do that.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

IIUC, this should now be fixed across all supported branches (Fx60+ & ESR 52.8.0).

Comment 12

[Daniel Veditz [:dveditz]]

Thank you for the clarity of the separate PoC, but we think for bounty purposes this is covered by the bounty in bug 1449898 since Brendan noted the abort issue as part of his debugging (bug 1449898 comment 13).

Comment 13

[Wladimir Palant]

(In reply to Daniel Veditz [:dveditz] from comment #12)
> but we think for bounty
> purposes this is covered by the bounty in bug 1449898 since Brendan noted
> the abort issue as part of his debugging (bug 1449898 comment 13).

Absolutely, that's what I said as well.

Comment 14

[Catalin Sasca, Desktop Test Engineering [:csasca]]

I successfully reproduced the issue on Firefox 59.0.2 RC, under Ubuntu 16.04 (x64) using STR from Comment 0.
The issue is no longer reproducible on Firefox 52.8.0esr, Firefox 60.0.2 RC and latest Nightly 61.0a1 (2018-05-04). Tests were performed under Windows 10 (x64), Ubuntu 16.04 (x64) and macOS 10.12.