Closed Bug 1449898 (CVE-2018-5157) Opened 2 years ago Closed 2 years ago

Race condition in PDF Viewer allows circumventing same-origin policy for PDF files

Categories

(Firefox :: PDF Viewer, defect, P1, critical)

defect

Tracking

()

VERIFIED FIXED
Firefox 61
Tracking Status
firefox-esr52 60+ verified
firefox59 --- wontfix
firefox60 + verified
firefox61 + verified

People

(Reporter: gaubugzilla, Assigned: bdahl)

References

Details

(Keywords: csectype-sop, sec-high, Whiteboard: [adv-main60+][adv-esr52.8+][post-critsmash-triage])

Attachments

(3 files, 5 obsolete files)

Attached file 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.
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.
Yury / Brendan, can you take a look please?
Flags: needinfo?(ydelendik)
Flags: needinfo?(bdahl)
I'm able to reproduce. Looking into fixes now.
Flags: needinfo?(bdahl)
MozReview-Commit-ID: 6l8ZJKjierS
Assignee: nobody → bdahl
Flags: needinfo?(ydelendik)
Attachment #8963831 - Flags: review?(dtownsend)
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.
(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.
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 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)
Attachment #8963831 - Flags: review?(dtownsend) → review?(gijskruitbosch+bugs)
(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?
Flags: needinfo?(bdahl)
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 .
Attachment #8963831 - Flags: review?(gijskruitbosch+bugs) → feedback+
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.
MozReview-Commit-ID: 6l8ZJKjierS
Attachment #8964082 - Flags: review?(MattN+bmo)
Attachment #8963831 - Attachment is obsolete: true
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.
Flags: needinfo?(bdahl)
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.
(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.
See Also: → 1450534
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 `=`
Attachment #8964082 - Flags: review?(MattN+bmo) → review+
Blocks: 1450534
Flags: sec-bounty?
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.
Flags: sec-bounty?
Flags: sec-bounty+
Flags: needinfo?(bdahl)
MozReview-Commit-ID: 6l8ZJKjierS
Attachment #8964082 - Attachment is obsolete: true
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.
Flags: needinfo?(bdahl)
Attachment #8964367 - Flags: sec-approval?
Another option for landing this patch is rolling it into a PDF.js update.
(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.
(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.
See Also: 1450534
Severity: normal → critical
Priority: -- → P1
MozReview-Commit-ID: 6l8ZJKjierS
Attachment #8965424 - Flags: review?(gijskruitbosch+bugs)
Attachment #8964367 - Attachment is obsolete: true
Attachment #8964367 - Flags: sec-approval?
Comment on attachment 8965424 [details] [diff] [review]
Ensure chrome PDF.js messages are only sent to the PDF viewer.

See comment 19
Attachment #8965424 - Flags: sec-approval?
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.
Attachment #8965424 - Flags: review?(gijskruitbosch+bugs) → review+
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?)
Flags: needinfo?(bdahl)
Attachment #8965424 - Flags: sec-approval? → sec-approval+
(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?
Flags: needinfo?(bdahl) → needinfo?(abillings)
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.
Flags: needinfo?(abillings)
MozReview-Commit-ID: 6l8ZJKjierS

# Conflicts:
#	browser/extensions/pdfjs/content/PdfStreamConverter.jsm
Attachment #8966404 - Attachment description: Ensure chrome PDF.js messages are only sent to the PDF viewer. r=mattn → esr-52 - Ensure chrome PDF.js messages are only sent to the PDF viewer. r=mattn
MozReview-Commit-ID: 6l8ZJKjierS
Attachment #8965424 - Attachment is obsolete: true
MozReview-Commit-ID: 6l8ZJKjierS
Attachment #8966404 - Attachment is obsolete: true
Attachment #8967636 - Attachment description: Ensure chrome PDF.js messages are only sent to the PDF viewer. → ESR - Ensure chrome PDF.js messages are only sent to the PDF viewer.
https://hg.mozilla.org/mozilla-central/rev/676c6c0096af
https://hg.mozilla.org/releases/mozilla-beta/rev/ad91b3bde301
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Group: firefox-core-security → core-security-release
Whiteboard: [adv-main60+][adv-esr52.8+]
Alias: CVE-2018-5157
Flags: qe-verify+
Whiteboard: [adv-main60+][adv-esr52.8+] → [adv-main60+][adv-esr52.8+][post-critsmash-triage]
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.
Flags: needinfo?(gaubugzilla)
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).
Flags: needinfo?(gaubugzilla)
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.