1450443 - PDF Viewer should use a different principal (defense in depth)

Status: NEW

(Firefox :: PDF Viewer, enhancement, P3)

Description

[Wladimir Palant]

Right now, PdfStreamConverter.jsm will set the principal for the PDF Viewer to resource://pdf.js/. IMHO this is suboptimal because compared to a regular page from resource://pdf.js/ the PDF Viewer has special powers. Normally, an XSS in some page on resource:// wouldn't be a security issue - an attacker doesn't gain anything with it. If a webpage could inject JavaScript code into some page from resource://pdf.js/ however, it would be able to create a PDF Viewer frame which would be considered same-origin - meaning that it would gain access to PDF Viewer's special powers.

While this is not an issue right now (the only HTML page under resource://pdf.js/ is viewer.html, and an XSS vulnerability there would mean an XSS vulnerability in the PDF Viewer itself), using a different principal would eliminate that threat completely. For example, one could use resource://pdf.js.internal/ for the principal - there are no files under resource://pdf.js.internal/, so no issues to be exploited.

Comment 1

[:Gijs (he/him)]

(In reply to Wladimir Palant (for Adblock Plus info Cc [e-mail]) from comment #0)
> While this is not an issue right now (the only HTML page under
> resource://pdf.js/ is viewer.html, and an XSS vulnerability there would mean
> an XSS vulnerability in the PDF Viewer itself), using a different principal
> would eliminate that threat completely. For example, one could use
> resource://pdf.js.internal/ for the principal - there are no files under
> resource://pdf.js.internal/, so no issues to be exploited.

I don't think that in the current architecture it's easily possible to assign an arbitrary codebase principal to a document that doesn't match the document URI for the resulting page -- other than a null principal (which we use in a few places). I could be wrong, though. For a null principal, I'm not sure what you'd use for the origin string for postMessage, but I would be a little surprised if we could make that work. Part of the "point" of a null principal is that it's completely isolated from any other non-subsuming, non-system principal. Boris, do you have thoughts here?

FWIW, I'm also a bit confused what this suggestion is trying to protect against. My understanding of comment 0 is that it outlines:

1) we use resource://foo/, which means issues happen if anything (else) at resource://foo/ has XSS vulnerabilities, because it could x-origin access any pdf viewer it could get its hands on
2) there is only 1 document in resource://foo/ that would allow this to happen
3) that document is the pdf viewer, so if that had xss flaws we'd have an issue no matter what principal we gave the document.


As a principle (ba-dum-tss - I live in Britain now, so have to make at least 1 bad pun a day), it would be good to use a null principal. But the issue seems pretty theoretical. In terms of bang-for-our-buck, I'd be much more inclined, for instance, to try to lock down CSP on the pdf viewer if we haven't already... Brendan, do you know what the state of things is in that department? (that might want to be a separate bug if we do do something)

Comment 2

[Boris Zbarsky [:bzbarsky]]

It's not that hard to end up with a principal that doesn't match the document URI if you have access to the channel early enough.  That said, the mechanism one would use for that (the "owner") is something we are trying to remove.

In general, having a principal not matching the URI is not a great idea because not everything does security checks on principals...  So it's too easy to give yourself a false sense of security.

Comment 3

[Wladimir Palant]

I made this suggestion because the principal is being set explicitly by PdfStreamConverter.jsm in this case, it already doesn't match the document URI (the latter being the URI of the PDF file). Since the principal is chosen freely, it can be set to something that is more secure. But it is up to you of course to decide whether this is worth doing.

Comment 4

[Wladimir Palant]

For reference, a null principal would work in principle - it would make it rather hard to verify that messages from PdfStreamConverter.jsm are sent to the right document however (both postMessage() and custom events).

Comment 5

[Brendan Dahl [:bdahl]]

Clearing needinfo's that I'm unlikely to work on in the near future.

Comment 6

[Rob Wu [:robwu]]

When this issue was filed, the concept of "precursor" did not exist. It does exist now, so we can feasibly consider using a null principal for PDF viewers.

A potential way to resolve this issue is to force all PDF viewers to have an opaque origin:

1. E.g. by rewriting the CSP in PdfStreamConverter.sys.mjs and (and something else for non-http channels).
   • alternatively: mint a null principal before setting aRequest.owner, with the precursor set to the PDF viewer (and/or the original URL).
2. With the above change, the viewer document is now a null principal instead of a content principal. There are several internal checks that confirm that the principal is resource://pdf.js. These would have to change, to check that the principal is a null principal with the precursor resource://pdf.js.
3. Besides these internal checks, the stream converter itself also needs to be updated: it currently uses postMessage to send data to the PDF.js origin. An alternative mechanism would be to use CustomEvent to send data from system privileged code into the viewer. CustomEvent does not support transferables (an optimization to reduce memory overhead). An alternative to that would be to construct a MessageChannel and send a port to the viewer. Then the viewer can use the same logic that it had before to send data via the MessagePort.
   • using CustomEvent would also fix bug 1685092. Side note, in that bug CustomEvent was also proposed and accepted as an alternative to postMessage.

Comment 7

[Rob Wu [:robwu]]

(I meant to mark this bug as a blocker of bug 1454760 - before we can ever consider allowing extensions to run content script in the PDF viewer, we need to make sure that the viewer cannot script unrelated origins)

Comment 8

[Jonas Jenwald [:Snuffleupagus]]

(In reply to Rob Wu [:robwu] from comment #6)

[...] An alternative mechanism would be to use CustomEvent to send data from system privileged code into the viewer. CustomEvent does not support transferables (an optimization to reduce memory overhead).

A solution that increases memory usage does not seem like a good idea, at least as far as I'm concerned.

Comment 9

[Tom Ritter [:tjr]]

There are several internal checks that confirm that the principal is resource://pdf.js

Many of them are broken, AFAICT. There's some discussion in Bug 1918257 about this as well (It might be considered a dupe of this). IT talks about another user case we have for identifying PDF.js

Comment 10

[Rob Wu [:robwu]]

(In reply to Jonas Jenwald [:Snuffleupagus] from comment #8)

(In reply to Rob Wu [:robwu] from comment #6)

[...] An alternative mechanism would be to use CustomEvent to send data from system privileged code into the viewer. CustomEvent does not support transferables (an optimization to reduce memory overhead).

A solution that increases memory usage does not seem like a good idea, at least as far as I'm concerned.

The next sentence after the quote that you clipped mentions MessageChannel (and its pair of MessagePort), which do support transferables, to minimize impact on memory usage. In conjunction with CustomEvent it would look like this:

window.addEventListener("eventName, e => {
  let port = event.detail;
  port.onmessage = e => {
    console.log("Received", e.data);
  };
});

var { port1, port2 } = new MessageChannel();
window.dispatchEvent(new CustomEvent("eventName", { detail: port1 }));

var data = new Uint8Array([1, 2, 3]);
port2.postMessage(data, [ data.buffer ]);
console.log("Sent", data); // Shows an empty array - the data has been transferred elsewhere.

Comment 11

[Calixte Denizet (:calixte)]

:robwu, can the port be "transfered" from privileged code to the viewer ?
Usually we've to clone the details in the window before sending them in a CustomEvent. I don't think a port is cloneable.

Comment 12

[Frederik Braun [:freddy]]

There's a resource principal as well as the PDFJS code using a content-principal of the target URL to fetch the pdf file with a contentPolicyType of SEC_OTHER
I think there's value in finding a pdfjs-specific solution rather than making up stuff that sort-of works on the fly.

We could mint a new contentPolicyType "TYPE_PDFJS_DOWNLOAD" to treat this differently (That won't help with the document principal though)
We could also use an ExtendedPrincipal (an array of [foo.example/test.pdf, resource://pdfjs-url]) and check the principal in nsContentUtils code ( browserglue).

Comment 13

[:Gijs (he/him)]

Last I checked we can't use expanded principals for documents, however. :-(