1449898 - (CVE-2018-5157) Race condition in PDF Viewer allows circumventing same-origin policy for PDF files

Status: VERIFIED FIXED

(Firefox :: PDF Viewer, defect, P1)

Description

[Wladimir Palant]

Attachment: Proof of concept page

When a PDF file opens in the built-in PDF Viewer, the stream converter will retrieve the file data and post it as a message to the viewer (see https://dxr.mozilla.org/mozilla-central/rev/a456475502b80a1264642d9eaee9394a8fad8315/browser/extensions/pdfjs/content/PdfStreamConverter.jsm#605). When doing this, it specifies "*" as the recipient's origin. So if the PDF Viewer got replaced by another document in the meantime, that other document will receive the message.

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 https://www.microsoft.com/en-us/research/wp-content/uploads/2006/11/www2007.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 in a hidden frame first to make sure that it is cached. After that it will 
repeatedly load the PDF file in a frame, then replace the frame contents by about:blank after a random time interval.


I am not entirely sure why it doesn't work reliably with other PDF files for me, but with some tweaks to the timing it should be possible. So a malicious website should be able to retrieve PDF files in the background that only the website visitor can access. That might be documents on the intranet for example, or private documents on some storage service.

I tested with Firefox 59.0.2 and Firefox 61.0a1 (2018-03-28) nightly on Linux.

Comment 1

[Wladimir Palant]

Actually, figured out why this isn't reliable for other PDF files - it's reliable if pdfjs.disableRange and pdfjs.disableStream preferences are set to true. Meaning that StandardChromeActions class is the one more susceptible to this attack. I did see this succeed against RangedChromeActions class as well however, merely harder to reproduce.

Comment 2

[:Gijs (he/him)]

Yury / Brendan, can you take a look please?

Comment 3

[Brendan Dahl [:bdahl]]

I'm able to reproduce. Looking into fixes now.

Comment 4

[Brendan Dahl [:bdahl]]

Attachment: Ensure chrome PDF.js messages are only sent to the PDF viewer. r=mossop

MozReview-Commit-ID: 6l8ZJKjierS

Comment 5

[Brendan Dahl [:bdahl]]

The patch provides two ways to avoid sending messages to non-pdfjs windows (either works by itself, but I added both for safety):

1) Don't attach to windows in the first place if they don't match the pdf.js principal.
2) Only send message to the pdf.js origin.

Comment 6

[Wladimir Palant]

(In reply to Brendan Dahl [:bdahl] from comment #5)
> 1) Don't attach to windows in the first place if they don't match the pdf.js
> principal.

I don't really see how this could possibly happen in onStopRequest - it's a channel where you've set the principal explicitly. The issue here isn't that the wrong document loaded initially, it's rather the window's document getting replaced by another at some point later. So this change might make sense just to be safe, but I don't think that it would stop the attack here.

Comment 7

[Brendan Dahl [:bdahl]]

You're right #1 shouldn't fully prevent the issue. I do however occasionally get failures during onStopRequest with the added principal check where:

win.document.nodePrincipal.URI.spec = http://localhost/sandbox/pdf_retriever.html?url=https%3A%2F%2Fwww.microsoft.com%2Fen-us%2Fresearch%2Fwp-content%2Fuploads%2F2006%2F11%2Fwww2007.pdf

I'm not very familiar with how the channels code should work. Should that ever be possible or do we have another problem?

Comment 8

[Brendan Dahl [:bdahl]]

Comment on attachment 8963831 [details] [diff] [review]
Ensure chrome PDF.js messages are only sent to the PDF viewer. r=mossop

Dave is PTO can you take a look or recommend someone else? 

(also see comment 7)

Comment 9

[:Gijs (he/him)]

(In reply to Brendan Dahl [:bdahl] from comment #7)
> You're right #1 shouldn't fully prevent the issue. I do however occasionally
> get failures during onStopRequest with the added principal check where:
> 
> win.document.nodePrincipal.URI.spec =
> http://localhost/sandbox/pdf_retriever.html?url=https%3A%2F%2Fwww.microsoft.
> com%2Fen-us%2Fresearch%2Fwp-content%2Fuploads%2F2006%2F11%2Fwww2007.pdf
> 
> I'm not very familiar with how the channels code should work. Should that
> ever be possible or do we have another problem?

I'm not sure, and I think this needs sorting out. Can you find out when that happens? Does the listener end up attached to requests it shouldn't be attached to, or is something else going on?

Comment 10

[:Gijs (he/him)]

Comment on attachment 8963831 [details] [diff] [review]
Ensure chrome PDF.js messages are only sent to the PDF viewer. r=mossop

Review of attachment 8963831 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the origin changes for postMessage, but f+ for now to work out what's going on with the request stopping.

Note that today + Monday are public holidays here, so I might not be around much before Tuesday. If you need help with the networking/onStopRequest stuff, try asking :mayhemer or someone like :mcmanus or :dragana . If you need more reviews, you could also ask :jaws or :MattN .

Comment 11

[:Gijs (he/him)]

I assume we'll want to track this for 60. Not sure we'll get a fix for 59 at this point, depends a bit on how hard it turns out to be to work out the details.

Comment 12

[Brendan Dahl [:bdahl]]

Attachment: Ensure chrome PDF.js messages are only sent to the PDF viewer. r=

MozReview-Commit-ID: 6l8ZJKjierS

Comment 13

[Brendan Dahl [:bdahl]]

I think I see what's happening when the channel's nodePrincipal is not what I'd expect. It happens when the request is aborted and I there's an abort statusCode in onStopRequest.

Comment 14

[Wladimir Palant]

IMHO, the principal used here is suboptimal - this is an unrelated issue however and currently not exploitable. I filed a public bug 1450443 on that.

Comment 15

[Wladimir Palant]

(In reply to Brendan Dahl [:bdahl] from comment #13)
> I think I see what's happening when the channel's nodePrincipal is not what
> I'd expect. It happens when the request is aborted and I there's an abort
> statusCode in onStopRequest.

You are correct. There is a second vulnerability here, I filed bug 1450534 on it.

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8964082 [details] [diff] [review]
Ensure chrome PDF.js messages are only sent to the PDF viewer. r=

Review of attachment 8964082 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM without much context on this code. I'm not sure how good the automated text coverage is so we may want manual verification.

::: browser/extensions/pdfjs/content/PdfStreamConverter.jsm
@@ +19,5 @@
>  
>  const PDFJS_EVENT_ID = "pdf.js.message";
>  const PREF_PREFIX = "pdfjs";
> +const PDF_VIEWER_ORIGIN = "resource://pdf.js";
> +const PDF_VIEWER_WEB_PAGE =  `${PDF_VIEWER_ORIGIN}/web/viewer.html`;

Nit: There is a double-space after the `=`

Comment 17

[Daniel Veditz [:dveditz]]

Please request sec-approval before landing. We may want to reconsider the comments in the patch, or perhaps land near the very end of the cycle since they really point to the problem.

Does this affect ESR-52? I assume so, but perhaps not.

Comment 18

[Brendan Dahl [:bdahl]]

Attachment: Ensure chrome PDF.js messages are only sent to the PDF viewer.

MozReview-Commit-ID: 6l8ZJKjierS

Comment 19

[Brendan Dahl [:bdahl]]

Comment on attachment 8964367 [details] [diff] [review]
Ensure chrome PDF.js messages are only sent to the PDF viewer.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
With the comments it be easier to reason it out, but still not trivial.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Yes.

Which older supported branches are affected by this flaw?
All supported branches are.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I haven't made any yet, but should be pretty easy. This code hasn't changed much in a while.

How likely is this patch to cause regressions; how much testing does it need?
Small chance there's some edge case this could break some PDF loading flow, but pdf.js has automated testing to cover the common cases.

Comment 20

[Brendan Dahl [:bdahl]]

Another option for landing this patch is rolling it into a PDF.js update.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

(In reply to Brendan Dahl [:bdahl] from comment #20)
> Another option for landing this patch is rolling it into a PDF.js update.

That would work on trunk, but we'd still have to uplift a targeted fix to the branches.

Comment 22

[Al Billings [:abillings - ex-MoCo]]

(In reply to Brendan Dahl [:bdahl] from comment #20)
> Another option for landing this patch is rolling it into a PDF.js update.

I'd prefer that over a bulleye patch!

(In reply to Ryan VanderMeulen [:RyanVM] from comment #21)
 
> That would work on trunk, but we'd still have to uplift a targeted fix to
> the branches.

Sure. We could delay that a week or two though.

Comment 23

[Brendan Dahl [:bdahl]]

Attachment: Ensure chrome PDF.js messages are only sent to the PDF viewer.

MozReview-Commit-ID: 6l8ZJKjierS

Comment 24

[Brendan Dahl [:bdahl]]

Comment on attachment 8965424 [details] [diff] [review]
Ensure chrome PDF.js messages are only sent to the PDF viewer.

See comment 19

Comment 25

[:Gijs (he/him)]

Comment on attachment 8965424 [details] [diff] [review]
Ensure chrome PDF.js messages are only sent to the PDF viewer.

Review of attachment 8965424 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

Comment 26

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8965424 [details] [diff] [review]
Ensure chrome PDF.js messages are only sent to the PDF viewer.

sec-approval+ for trunk.

We need to have Beta and ESR52 patches made and nominated as soon as this lands as well so we can fix this everywhere. (This does affect ESR52, right?)

Comment 27

[Brendan Dahl [:bdahl]]

(In reply to Al Billings [:abillings] from comment #26)
> Comment on attachment 8965424 [details] [diff] [review]
> Ensure chrome PDF.js messages are only sent to the PDF viewer.
> 
> sec-approval+ for trunk.
> 
> We need to have Beta and ESR52 patches made and nominated as soon as this
> lands as well so we can fix this everywhere. (This does affect ESR52, right?)

Yes, affects ESR52.


How should rolling this out work? Do you want me to land this with a pdf.js update, then update the other branches? Or?

Comment 28

[Al Billings [:abillings - ex-MoCo]]

Land the update on mozilla-central. Once it is green and looks good, we'll land on branches if we have approved patches for them.

Comment 29

[Brendan Dahl [:bdahl]]

Attachment: esr-52 - Ensure chrome PDF.js messages are only sent to the PDF viewer. r=mattn

MozReview-Commit-ID: 6l8ZJKjierS

# Conflicts:
#	browser/extensions/pdfjs/content/PdfStreamConverter.jsm

Comment 30

[Brendan Dahl [:bdahl]]

Attachment: Ensure chrome PDF.js messages are only sent to the PDF viewer.

MozReview-Commit-ID: 6l8ZJKjierS

Comment 31

[Brendan Dahl [:bdahl]]

Attachment: ESR - Ensure chrome PDF.js messages are only sent to the PDF viewer.

MozReview-Commit-ID: 6l8ZJKjierS

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Trunk patch landed on inbound as part of bug 1453838.
https://hg.mozilla.org/integration/mozilla-inbound/rev/676c6c0096afa3e77d32ace4cfd4b21f65b53d15

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/676c6c0096af
https://hg.mozilla.org/releases/mozilla-beta/rev/ad91b3bde301

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/608e76ec5ba2

Comment 35

[Catalin Sasca, Desktop QA [:csasca]]

Hi Wladimir. It seems that I can't manage to reproduce this issue by using the "proof of concept page". After attaching the link for the .pdf from Comment 0 and clicking on the "Download" button, it seems that the https://bug1449898.bmoattachments.org/attachment.cgi?url= link is redirected. 
Am I missing something? Also I see the same behavior happening for Bug 1450534.

Comment 36

[Wladimir Palant]

Yes, the proof of concept doesn't work when opened via Bugzilla - this page adds URL parameters without preserving the existing ones. You have to download the page and open it locally (file system will do).

Comment 37

[Catalin Sasca, Desktop QA [:csasca]]

Thank You for the answer Wladimir. I successfully reproduced the issue on Firefox 59.0.2 RC, under Ubuntu 16.04 (x64) using STR from Comment 0 and Comment 36.
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.