Closed Bug 1144988 (CVE-2015-0818) Opened 9 years ago Closed 9 years ago

Same-origin bypass via SVG hash navigation (ZDI-CAN-2825)

Categories

(Core :: DOM: Navigation, defect)

x86
All
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla39
Tracking Status
firefox36 + verified
firefox37 + verified
firefox38 + verified
firefox39 + verified
firefox-esr31 37+ verified
firefox-esr38 --- verified
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: dveditz, Assigned: smaug)

References

Details

(Keywords: sec-critical, Whiteboard: [filed bug 1145195 to change svgView() behavior][adv-main37-][adv-esr31.6-][b2g-adv-main2.2-])

Attachments

(13 files, 3 obsolete files)

849 bytes, text/html
Details
4.89 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.02 KB, patch
Details | Diff | Splinter Review
13.54 KB, patch
bzbarsky
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
15.68 KB, patch
Details | Diff | Splinter Review
15.72 KB, patch
Details | Diff | Splinter Review
3.20 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.63 KB, patch
Details | Diff | Splinter Review
3.55 KB, patch
Details | Diff | Splinter Review
137.81 KB, image/png
Details
154.08 KB, image/png
Details
903 bytes, text/html
Details
1.26 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Attached file moz-poc1.html
From winning Pwn2Own exploit by Mariusz Mlynski [1/2]:

-----
TL;DR
-----

It is possible to bypass the same-origin policy using a mutation event triggered in the middle of a SVG document's hash navigation. This can be exploited to execute script in the context of the internal pdf viewer, whose security principal is not restricted from loading privileged content, which allows web content to execute arbitrary code.


----------------------
1. CROSS-ORIGIN BYPASS
----------------------

In Firefox, short-circuited hash navigations are handled as follows:


>>>>>>>>>>>>>>>>>>>>>>>> /docshell/base/nsDocShell.cpp >>>>>>>>>>>>>>>>>>>>>>>>>

NS_IMETHODIMP
nsDocShell::InternalLoad(nsIURI * aURI, (...) )
{
(...)
        if (doShortCircuitedLoad) {
            // Save the position of the scrollers.
            nscoord cx = 0, cy = 0;
            GetCurScrollPos(ScrollOrientation_X, &cx);
            GetCurScrollPos(ScrollOrientation_Y, &cy);

            // ScrollToAnchor doesn't necessarily cause us to scroll the window;
            // the function decides whether a scroll is appropriate based on the
            // arguments it receives.  But even if we don't end up scrolling,
            // ScrollToAnchor performs other important tasks, such as informing
            // the presShell that we have a new hash.  See bug 680257.
            rv = ScrollToAnchor(curHash, newHash, aLoadType);
            NS_ENSURE_SUCCESS(rv, rv);

            (...)

            /* This is a anchor traversal with in the same page.
             * call OnNewURI() so that, this traversal will be 
             * recorded in session and global history.
             */
            nsCOMPtr<nsISupports> owner;
            if (mOSHE) {
                mOSHE->GetOwner(getter_AddRefs(owner));
            }
            // Pass true for aCloneSHChildren, since we're not
            // changing documents here, so all of our subframes are
            // still relevant to the new session history entry.
            //
            // It also makes OnNewURI(...) set LOCATION_CHANGE_SAME_DOCUMENT
            // flag on firing onLocationChange(...).
            // Anyway, aCloneSHChildren param is simply reflecting
            // doShortCircuitedLoad in this scope.
            OnNewURI(aURI, nullptr, owner, mLoadType, true, true, true);

            (...)

            return NS_OK;
        }
(...)
}

<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<


This code doesn't consider that |ScrollToAnchor| can run script if the following conditions are met:

1. A SVG document is being navigated;
2. A DOMAttrModified mutation listener is registered on the SVG document's documentElement;
3. The hash modifies a SVGViewAttribute through the use of a SVG ViewSpec, such as "svgView(viewBox(0,0,0,0))".

In such case, the following code path is taken to trigger a mutation event:

nsDocShell::InternalLoad
nsDocShell::ScrollToAnchor
PresShell::GoToAnchor
mozilla::SVGFragmentIdentifier::ProcessFragmentIdentifier
mozilla::SVGFragmentIdentifier::ProcessSVGViewSpec
nsSVGViewBox::SetBaseValueString
nsSVGElement::DidChangeViewBox
nsSVGElement::DidChangeValue
mozilla::dom::Element::SetAttrAndNotify

The event handler can perform a synchronous navigation to a cross-origin document, affecting the state of the docshell and its corresponding history entry stored in nsDocShell::mOSHE. When the handler returns, the security principal associated with the docshell's current history entry (which is cross-origin at this point) is fetched in the mOSHE->GetOwner call. Then, |OnNewURI| combines the URI of the SVG document and the cross-origin owner into a new history entry, which is added to the session history. This allows an attacker to load a principal-inheriting data:image/svg+xml,* URI with the hijacked principal during a subsequent navigation to the newly added history entry (|history.go(2)| in the exploit).
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bugs)
See Also: → CVE-2015-0816
(In reply to Daniel Veditz [:dveditz] from comment #0)
> This code doesn't consider that |ScrollToAnchor| can run script if the
> following conditions are met:
[...]
> 3. The hash modifies a SVGViewAttribute through the use of a SVG ViewSpec,
> such as "svgView(viewBox(0,0,0,0))".

For the record, we added support for #svgView(viewBox(...)) syntax in bug 512525, in 2012, for Firefox 15. The ScrollToAnchor call (guarded by if (doShortCircuitedLoad)) was added the year before, in this changeset for bug 640387: http://hg.mozilla.org/mozilla-central/rev/228d41c97600

So this ScrollToAnchor call predates this particular means for running script.
Version: unspecified → Trunk
(In reply to Daniel Holbert [:dholbert] from comment #1)
> (In reply to Daniel Veditz [:dveditz] from comment #0)
> > This code doesn't consider that |ScrollToAnchor| can run script if the
> > following conditions are met:
> [...]
> > 3. The hash modifies a SVGViewAttribute through the use of a SVG ViewSpec,
> > such as "svgView(viewBox(0,0,0,0))".
> 
> For the record, we added support for #svgView(viewBox(...)) syntax in bug
> 512525, in 2012, for Firefox 15. The ScrollToAnchor call (guarded by if
> (doShortCircuitedLoad)) was added the year before, in this changeset for bug
> 640387: http://hg.mozilla.org/mozilla-central/rev/228d41c97600
> 
> So this ScrollToAnchor call predates this particular means for running
> script.

Can we plug this in the history entry logic in ScrollToAnchor()? It seems like we'll need to still allow fragment processing in SVG.
Re-reading the svgView() / <view> spec-text, here...
  http://www.w3.org/TR/SVG11/linking.html#ViewElement
... it sounds to me like we shouldn't really be *setting* these attributes on the <svg> element. So for example, if we navigate to #svgView(viewBox(...)), that shouldn't actually modify the value returned by rootSVGElem.getAttribute("viewBox").

Specifically, I'm looking at the bulleted list at the end of http://www.w3.org/TR/SVG11/linking.html#SVGFragmentIdentifiers -- that list seems to imply that the viewBox attribute on the root SVG node is used for rendering, if we don't have a view specified. But otherwise, that attribute is simply not used for rendering. (Its value isn't overwritten -- it's just not used.)

I'm bringing this up because if we don't actually overwrite the value (and instead just store the svgView()-provided attributes in a special extra place that we check when rendering the document), then there wouldn't be any 'attribute changed' notifications, and hence no way to trigger script via svgView() navigation.
The mochitest for svgView() behavior shows that we do currently expect these svgView() fragment-identifiers to modify the value returned by getAttribute -- that's how we test that the svgView() navigation was effective, actually. [1]

(If we changed to not actually modify the (DOM/script-exposed) attribute values, then we'd have to replace that mochitest with a rendering test, I guess)

Robert: do you recall if there's a reason we're actually modifying the attributes here, instead of setting up secret under-the-hood overrides? (as my current reading of the spec suggests we should do) E.g. maybe there's some other spec text that calls for actual attribute modification on the root <svg> element?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/svg/test/test_fragments.html?rev=d245e31f8edc&force=1&mark=69-69,74-74,77-77,80-80#69
Flags: needinfo?(longsonr)
Chrome & Opera 12.16 (Presto) match my expectations here -- svgView() expressions do not actually modify attributes there. Whereas, they do in Firefox.

Test URL, to demonstrate (using just-attached SVG file, & requires you to be logged into bugzilla, since this bug's attachments are hidden for now)
https://bugzilla.mozilla.org/attachment.cgi?id=8579844#svgView(viewBox(50,50,100,100))

When I load that URL in Firefox, I get an alert saying:
> getAttribute(viewBox) returns: '50 50 100 100'
which is the svgView()-provided value.

In contrast, when I load it in Opera 12.16 or Chrome, I get an alert saying:
> getAttribute(viewBox) returns: '9999 9999 200 200'
which is the actual underlying hardcoded 'viewBox' attribute (which is being overridden for rendering, but is still the value exposed via the DOM & getAttribute)

So, unless there's some reason that we're right on this & they're wrong, we should probably change this behavior to match them, for interop & to match my understanding of the spec. (and that will fix this bug, as well)

Having said that, it's possible there are simpler (and more trivial/backport-friendly) ways to shut down this bug's means of attack, too, which we could pursue in parallel / in addition.
I think we should spin off a public (non-security-sensitive) bug to bring our svgView() attribute-overriding-behavior into line with Opera/Chrome's behavior described in comment 6. This public bug shouldn't leak the existence of this vulnerability -- on it's own, that's a pretty tame-seeming change, and a clear interop/spec-compliance issue, and our current behavior isn't really a secret or an accident -- our mochitest for this feature explicitly *depends* on it right now, as noted above in comment 4.

(The public bug probably shouldn't mention mutation listeners, to be on the safe side).

Meanwhile, we can discuss any further implications & alternative potentially-more-backport-friendly fixes back over here on this hidden bug.

Any objections? If not, I'll file the public bug tomorrow at some point.
If Chrome/Opera do it we should too. The specification is too vague to be any use. I did it the way I did so that changing the attribute values would automatically cause the appropriate refresh/reflow to take place.

We need to make sure http://hoffmann.bplaced.net/svgtest/index.php?in=attributes#tr_view and http://hoffmann.bplaced.net/svgtest/viewviewbox01.svg#view1 continue to animate but we've reftests that should ensure that. Worth doing a manual check on those too though, just in case.
Flags: needinfo?(longsonr)
Component: DOM → SVG
Whiteboard: [unless there are objections, spinning off #svgView()-impacting-attributes into public bug]
I think we want a docshell fix here as well, since PresShell::GoToAnchor may end up flushing, which
means scripts may run.

Daniel, could you file the SVG bug to fix svgView?
Component: SVG → Document Navigation
Flags: needinfo?(bugs)
Attached patch possible patchSplinter Review
I think we want this or something close to this - do the scary stuff
at the end of doShortCircuitedLoad
Assignee: nobody → bugs
Attachment #8580018 - Flags: review?(bzbarsky)
Comment on attachment 8580018 [details] [diff] [review]
possible patch

I think this makes sense, esp. with a comment about making sure our internal state is all updated before we start notifying people.  It's the right general fix on the docshell side, assuming a hostile-ish presshell.  r=me for trunk based on that.

I also think this change is moderately scary in terms of regression risk.

For the safe backportable fix, couldn't we make the ProcessFragmentIdentifier call in presshell async?  I see no reason it needs to happen synchronously. And even if that does cause some problem, the risk is way lower simply because it's used so much less than anchor scrolling in general.

And of course we should file a followup on having ProcessFragmentIdentifier not mutate the DOM...
Attachment #8580018 - Flags: review?(bzbarsky) → review+
Oh, I was going to say.  We could also disallow navigation of the docshell while we're inside doShortCircuitedLoad or parts of it, without reordering things.  That seems like it would also be safer and more backportable, in case doing the SVG thing async doesn't pan out for some reason.
Flags: needinfo?(bzbarsky)
(In reply to Not doing reviews right now from comment #11)
> I also think this change is moderately scary in terms of regression risk.
> 
I agree

> For the safe backportable fix, couldn't we make the
> ProcessFragmentIdentifier call in presshell async?  I see no reason it needs
> to happen synchronously. And even if that does cause some problem, the risk
> is way lower simply because it's used so much less than anchor scrolling in
> general.
very true, though it doesn't fix the actually bug. One may figure out a way to run scripts when
scrolling triggers layout flush.
(Flush happens when PresShell::GoToAnchor calls PresShell::ScrollContentIntoView)



dveditz, how soon do we need the fix?
Flags: needinfo?(dveditz)
Flags: needinfo?(dveditz)
> One may figure out a way to run scripts when scrolling triggers layout flush.

That's true.  Generally this takes some work (e.g. XBL), but the possibility _is_ there.  Hence the suggestion in comment 12 too.
Preparing a patch to follow the approach from comment 12.


(looks like resize event could be a way to trigger JS when flushing layout.)
(In reply to Olli Pettay [:smaug] from comment #13)
> dveditz, how soon do we need the fix?

The sooner the better. I don't know if we're going to chemspill to fix the Pwn2Own bugs (response time is visible and watched) or get it into the Fx37 release in 10 days -- either way not that much time. We still have one more potential Pwn2Own bug to come and no idea how hard that might be to find and fix, and there's no point in releasing a fix that doesn't fix all the bugs.
Attachment #8580099 - Flags: review?(bzbarsky)
(In reply to Olli Pettay [:smaug] from comment #9)
> Daniel, could you file the SVG bug to fix svgView?

Done -- filed bug 1145195.

(In reply to Not doing reviews right now from comment #11)
> For the safe backportable fix, couldn't we make the
> ProcessFragmentIdentifier call in presshell async?

Two problems with that:
 (1) Our mochitest, test_fragments.html, assumes that the attributes are set synchronously, and I think this would break that assumption.  (Maybe that's fine, though, given that we're going to break that assumption harder in bug 1145195. It just means that this "safe backportable fix" would need a test-rewrite and would potentially break content that makes similar assumptions.)

 (2) If someone loaded foo.svg#svgView(whatever), and we process the svgView() syntax asynchronously, then I'd think they might see a "flash" of the default foo.svg viewport (depending on event ordering). If that can happen (I'm not 100% sure it can but seems possible), that'd be undesirable, and it'd be annoying for use-cases like sprite-sheets.

Anyway, sounds like we're already going a different route here, though.
Whiteboard: [unless there are objections, spinning off #svgView()-impacting-attributes into public bug] → [filed bug 1145195 to chnage svgView() behavior]
Comment on attachment 8580099 [details] [diff] [review]
block-loading approach

r=me based on the diff -w
Attachment #8580099 - Flags: review?(bzbarsky) → review+
Docshell uses still the old coding style in beta so need a separate patch to
deal with 4 spaces indentation etc.
Comment on attachment 8580099 [details] [diff] [review]
block-loading approach

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I'd say not easily.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
commit message should be
"Bug 1144988, don't let other pages to load while doing scroll-to-anchor, r=bz"

Which older supported branches are affected by this flaw?
all

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
the beta patch is the same as the patch for aurora/trunk, just different indentation 

How likely is this patch to cause regressions; how much testing does it need?
Should be reasonable safe, but this is DocShell code, which in general tends to be rather regression prone.
Attachment #8580099 - Flags: sec-approval?
Comment on attachment 8580099 [details] [diff] [review]
block-loading approach

We'll want beta, aurora, and ESR31 patches too, for completeness.
Attachment #8580099 - Flags: sec-approval? → sec-approval+
Whiteboard: [filed bug 1145195 to chnage svgView() behavior] → [filed bug 1145195 to change svgView() behavior]
Keywords: sec-high
Does the patch apply to ESR31?
Flags: needinfo?(bugs)
Let me clone esr31 and check.
(In reply to Al Billings [:abillings] from comment #24)
> I assume this should be rated sec-critical?

The whole exploit chain is definitely sec-critical. By itself we normally count SOP violations as sec-high, it's the combination with the other bug that makes it critical. But the other bug by itself isn't so bad (moderate?) so maybe we should go ahead and count this as the critical one.
Keywords: sec-highsec-critical
Flags: needinfo?(bugs)
Comment on attachment 8580099 [details] [diff] [review]
block-loading approach

Approval Request Comment
[Feature/regressing bug #]: Ancient stuff
[User impact if declined]: See the bug summary
[Describe test coverage new/current, TreeHerder]: landed to m-i
[Risks and why]: Should be reasonable safe, but docshell stuff is always a bit regression risky.
[String/UUID change made/needed]: NA
Attachment #8580099 - Flags: approval-mozilla-aurora?
Comment on attachment 8580119 [details] [diff] [review]
block-loading approach (for beta)

Approval Request Comment
See comment 30
Attachment #8580119 - Flags: approval-mozilla-beta?
Comment on attachment 8580301 [details] [diff] [review]
block-loading approach (for esr31)

[Approval Request Comment]
See comment 30
Attachment #8580301 - Flags: approval-mozilla-esr31?
Attachment #8580099 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8580119 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8580301 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
Comment on attachment 8580119 [details] [diff] [review]
block-loading approach (for beta)

This one seems to apply to release branch too.

Approval Request Comment
See comment 30
Attachment #8580119 - Flags: approval-mozilla-release?
Attachment #8580119 - Flags: approval-mozilla-release? → approval-mozilla-release+
Alias: CVE-2015-0818
Test failures. Session restore is doing something a bit unexpected. looking.
Attachment #8580382 - Flags: review?(bzbarsky)
(Tree is closed, so can't push atm. I'd use commit message "Bug 1144988, don't let other pages to load while doing scroll-to-anchor, r=bz")
(In reply to Olli Pettay [:smaug] from comment #39)
> (Tree is closed, so can't push atm. I'd use commit message "Bug 1144988,
> don't let other pages to load while doing scroll-to-anchor, r=bz")

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/ec28e56febf1
Can we get the branch patches formatted as patches and not raw diffs?
Flags: needinfo?(bugs)
Comment on attachment 8580382 [details] [diff] [review]
limit the blocking only to around ScrollToAnchor (trunk, aurora)

r=me, but when you're awake again, could you please add a comment about why a broader scope there is not ok?  Because I'd _really_ like to understand that.
https://hg.mozilla.org/mozilla-central/rev/ec28e56febf1
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Reproduced with 36.0.1 build 2 (20150305021524)  on OS X 10.9.5 by loading https://bugzilla.mozilla.org/attachment.cgi?id=8579844#svgView(viewBox(50,50,100,100)) - same value is displayed in the alert window.

Verified as fixed on Firefox 36.0.3, Firefox 37.0b7, latest Developer Edition 38.0a2 (2015-03-20) and latest Nightly 39.0a1 (2015-03-20) - the correct value is displayed at 1st load; but when loaded in a new tab, the issue is still reproducible [1]. I suspect this is expected because I encountered the same behavior Chrome.

Additional notes:
1. Both test cases from comment 8 worked as expected
2. [2] [OSX only] Firefox 37.0b7 (20150319212106) is unresponsive after loading https://bug1144988.bugzilla.mozilla.org/attachment.cgi?id=8579746&t=l5YvUhTDtq; forced kill the process to generate a report: 

bp-a8e184c9-dce3-44d3-82c0-7b4f82150320 
bp-dacc8130-8fd5-4635-8afe-1a3c82150320 

Any ideas regarding the issues mentioned at [1] and [2]?
Flags: needinfo?(bugs)
ni dveditz and abillings to comment on comment 46 as well.
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #46)
> Additional notes:
> 1. Both test cases from comment 8 worked as expected

Thanks -- yeah, that's expected.  We didn't end up changing SVG behavior here; I spun that off as bug 1145195.

I don't know about question [2].
dholbert, is your reply to what you quoted or to the actual "[1]" above (sorry, there are two "1"s there) which is "the correct value is displayed at 1st load; but when loaded in a new tab, the issue is still reproducible [1]"?
Adding ni? for dholbert for my question, but if someone else can answer this, please remove it.
Flags: needinfo?(dholbert)
Attached image Screenshot mobile
On mobile, as per the test mentioned in Comment #6 and Comment #8 I am able to see the same behavior before fixing on 36.0.3 and 37 Beta 7 . 

This is always reproducing - at first load, in new tab, at reload
However - if i go to https://bug1145195.bugzilla.mozilla.org/attachment.cgi?id=8580091#svgView(viewBox(50,50,100,100)) without being logged in and I log in - we get this screen but the URL is changed to a different one.
No idea about the osx hang. Perhaps it is about the sync XHR? I don't have access to OSX.
Do you have profile? What if you change the testcase so that you try to all the time
just recreate a sync XHR? Does the hang happen then even without the patch?
Flags: needinfo?(bugs)
Flags: needinfo?(abillings)
On OSX, did you have your browser set to use pdfjs for PDFs?  Because the testcase basically does sync XHR in a loop until the pdfjs bits load, so if you're not using pdfjs for PDFs testcase just loops forever.
Flags: needinfo?(bogdan.maris)
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #49)
> dholbert, is your reply to what you quoted or to the actual "[1]" above

(Yup, it was to the text I quoted.)
Flags: needinfo?(dholbert)
(In reply to Ioana Chiorean from comment #51)
> On mobile, as per the test mentioned in Comment #6 and Comment #8 I am able
> to see the same behavior before fixing on 36.0.3 and 37 Beta 7 . 
> 
> This is always reproducing

Yeah, that's spun off to bug 1145195 now.

(In reply to Ioana Chiorean from comment #52)
> However - if i go to
> https://bug1145195.bugzilla.mozilla.org/attachment.
> cgi?id=8580091#svgView(viewBox(50,50,100,100)) without being logged in and I
> log in - we get this screen but the URL is changed to a different one.

(I bet bugzilla login process strips off the #suffix from the URL, so that's probably expected.)
(In reply to Daniel Holbert [:dholbert] from comment #56)
> (In reply to Ioana Chiorean from comment #51)
> > On mobile, as per the test mentioned in Comment #6 and Comment #8 I am able
> > to see the same behavior before fixing on 36.0.3 and 37 Beta 7 . 
> > 
> > This is always reproducing
> 
> Yeah, that's spun off to bug 1145195 now.

So is there a way to verify this for mobile then?
Flags: needinfo?(dholbert)
I don't know, but I'd imagine you'd want to use the original proof-of-concept testcase, and verify in the same way that we did on desktop.  (I may be missing something -- does the proof-of-concept not work on mobile for some reason?)
Flags: needinfo?(dholbert)
Attachment #8579844 - Attachment description: (do not load directly; helper file to determine whether svgView() overrides viewBox attribute) → (part of testcase for the SVG bug here, which is now spun off to bug 1145195)
Attachment #8579844 - Attachment is obsolete: true
On mobile we do not get the alert when loading the POC liek on desktop:

"function MessageChannel() {
     [native code]
}"

We are investigating how to get this on IRC.
No worries, & thanks!

(In reply to Ioana Chiorean from comment #59)
> On mobile we do not get the alert when loading the POC liek on desktop:

This may be because PDF.js isn't functioning in the same way (or at all?) on Mobile.

I've installed the PDF Viewer addon [version 1.0.277] from AMO, in Firefox Beta on Android, but I can't get any PDFs to actually render in the browser. If I click a link to a PDF, the loading bar fills up for a second, but the page doesn't actually change.

The POC seems to use this data URL to trigger the PDF viewer:
  data:application/pdf,
I can bring up the PDF viewer on Desktop Firefox by directly entering this URL into the URL bar. But nothing happens when I try to visit this URL on mobile. [probably because PDF Viewer is broken there]

I suspect Ioana may be hitting the same issue when trying to reproduce on Mobile. Does anyone have know how to get PDF Viewer working on Mobile?
[autohiding comments 60-64, to make this bug a bit less confusing]
The original PoC passes in ESR Fx31.5.1 - Mac/Win/Linux. We no longer see the alert/message.

AFAICT, the PoC behaves the same in ESR Fx31.5.0 - perhaps the ESR branch was not vulnerable in the same way? No alert or message there.

Regardless, I was able to run the PoC against Fx36.0.1 vs Fx36.0.3 to see the change in behavior, so I'm confident that this was fixed.

Again, apologies for any confusion that I may have added.
I see this in logcat when I try to load a PDF on Android [with PDF viewer installed]:
W/GeckoConsole(26758): [JavaScript Error: "ReferenceError: PdfJsTelemetry is not defined" {file: "jar:file:///data/data/org.mozilla.firefox_beta/files/mozilla/b9hozhms.default/extensions/uriloader@pdf.js.xpi!/content/PdfStreamConverter.jsm" line: 903}]

Seems like PDF Viewer is just broken on Android right now. Oops.
OK, I successfully reproduced the bug (alert w/ "native code" when viewing POC), and I can now load PDFs, on Firefox Beta for Android, using the PDF.js extension from github:
  http://mozilla.github.io/pdf.js/extensions/firefox/pdf.js.xpi
(it reports itself as PDF viewer 1.1.9 in about:addons)
Ioana, can you make sure you've got the github-hosted version of PDF.js & can view PDFs? (If so, it seems like you should be able to trigger this bug in Firefox Beta at least, per comment 69, & verify that it's fixed w/ the update.)
Flags: needinfo?(ioana.chiorean)
[looks like the PDF.js issue I was hitting is https://github.com/mozilla/pdf.js/issues/5477 BTW. We apparently just haven't updated PDF.js on AMO since last June. ]
At lmandel's request, I verified that this bug is fixed, using updated Firefox Beta APK here:
 https://ftp.mozilla.org/pub/mozilla.org/mobile/candidates/37.0b7-candidates/build1/android-api-11/multi/

I verified that I'm still able to view PDF documents inside of Firefox (using my previously-installed PDF Viewer 1.1.9 add-on, from Github).

Given this result vs. comment 69, I'm confident saying that the bug is fixed in this particular APK.
(I also skimmed my 'adb logcat' output when viewing the POC for any tell-tale PdfJsTelemetry error output like in comment 68, and I didn't see any, which is good.)

I verified that this is fixed in our release-version update, too. Specifically:
 * I was able to reproduce the bug (the alert in comment 59) in current release, Firefox 36.0.2, obtained from Google Play Store.
 * I was unable to reproduce the bug in Firefox 36.0.3, obtained from
https://ftp.mozilla.org/pub/mozilla.org/mobile/candidates/36.0.3-candidates/build1/android/multi/
Mariusz filed a followup, bug 1145870, that says this patch is not a complete fix.
To elaborate, ESR passes the testcase only because MessageChannel is not available to resource URIs on ESR.  As was noted in comment 67, it passes before and after the fix.  ESR needs to pick up bug 1046022, or something like it.
khuey pointed out that the MessageChannel test used to determine exploitability in the original testcase isn't valid on ESR31, since MessageChannel on ESR31 isn't conditional on being a resource: URI.

Here's a variant of the testcase here that's just an XSS that "steals" some of the source code of pdf.js.
(In reply to Matt Wobensmith from comment #67)
> The original PoC passes in ESR Fx31.5.1 - Mac/Win/Linux. We no longer see
> the alert/message.
> 
> AFAICT, the PoC behaves the same in ESR Fx31.5.0 - perhaps the ESR branch
> was not vulnerable in the same way? No alert or message there.

As Kyle points out, this is actually because the technique the PoC uses is not valid against ESR31, and this bug is in fact not fixed on ESR31.

attachment 8581001 [details] shows the source code alert in the 2015-03-19 mozilla-central nightly (without the fixes that landed) but does not show the source code alert in the 2015-03-20 mozilla-central nightly.

It does show the source code on ESR31 tip.


Hopefully pulling bug 1046022 into ESR31 will fix that.
(In reply to David Baron [:dbaron] (UTC-7) from comment #78)
> Hopefully pulling bug 1046022 into ESR31 will fix that.

And khuey confirmed that it does (or at least that it does with both bug 1046022 and bug 1116977; I'm not sure which combinations of backports he's tested against which testcases).
Attached patch Patch for ESR31 (obsolete) — Splinter Review
See my previous comment.  With this backport ESR passes dbaron's testcase in this bug.
Attachment #8581005 - Flags: review?(bzbarsky)
I believe the questions asked for me have been answered. Sorry I've been offline traveling.
Flags: needinfo?(dveditz)
Attached patch Patch for ESR31 (obsolete) — Splinter Review
Attachment #8581005 - Attachment is obsolete: true
Attachment #8581005 - Flags: review?(bzbarsky)
Attachment #8581005 - Flags: approval-mozilla-esr31?
Attachment #8581022 - Flags: review?(bzbarsky)
Comment on attachment 8580382 [details] [diff] [review]
limit the blocking only to around ScrollToAnchor (trunk, aurora)

r=me
Attachment #8580382 - Flags: review?(bzbarsky) → review+
Attached patch Patch for ESR31Splinter Review
Attachment #8581022 - Attachment is obsolete: true
Attachment #8581022 - Flags: review?(bzbarsky)
Attachment #8581025 - Flags: review?(bzbarsky)
Comment on attachment 8581025 [details] [diff] [review]
Patch for ESR31

r=me, as an addition to that other patch.
Attachment #8581025 - Flags: review?(bzbarsky) → review+
Attachment #8581025 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
For QA, the failure case for dbaron's testcase is an alert that says "The page at resource://pdf.js says:" and has some pdf.js source code in it.  The success case is no alert.  The testcase should fail before the changes and pass afterwards on esr31.
Here are my results from testing this on the new candidate builds:
* Firefox 31.5.3esr en-US shows no alert
* Firefox 31.5.2esr en-US shows an alert with some pdf.js code
* Firefox 36.0.4 en-US shows no alert
* Firefox 36.0.3 en-US shows no alert
* Firefox 36.0.1 en-US show an alert with some pdf.js code

Based on these results I'm marking this verified fixed for 36 and 31esr.
Lawrence asked me to confirm this as well on our test branches.

* Firefox Nightly 39.0a1 2015-03-21 shows no alert
* Firefox Aurora 38.0a2 2015-03-21 shows no alert
* Firefox Beta 37.0b7 shows no alert

I'm marking this verified fixed based on these results.
Tested Android ARM and x86 builds. Firefox 36.0.2 alerts, Firefox 36.0.4 does not.
Depends on: CVE-2015-0801
Flags: needinfo?(bogdan.maris)
Whiteboard: [filed bug 1145195 to change svgView() behavior] → [filed bug 1145195 to change svgView() behavior][adv-main37-][adv-esr31.6-]
Also verified as fixed on Firefox 38.0 ESR across platforms (Windows 8.1 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 64-bit).
Why adv-, Al?
Flags: needinfo?(abillings)
Whiteboard: [filed bug 1145195 to change svgView() behavior][adv-main37-][adv-esr31.6-] → [filed bug 1145195 to change svgView() behavior][adv-main37-][adv-esr31.6-][b2g-adv-main2.2?]]
(In reply to Christiane Ruetten [:cr] from comment #95)
> Why adv-, Al?

Because I wrote mfsa2015-28 for this in Firefox 36.0.4 Firefox ESR 31.5.3's chemspill, predating the Firefox 37 release.
Flags: needinfo?(abillings)
Whiteboard: [filed bug 1145195 to change svgView() behavior][adv-main37-][adv-esr31.6-][b2g-adv-main2.2?]] → [filed bug 1145195 to change svgView() behavior][adv-main37-][adv-esr31.6-][b2g-adv-main2.2-]
Group: core-security → core-security-release
Flags: needinfo?(ioana.chiorean)
Attachment #8579746 - Attachment is private: true
Attachment #8581001 - Attachment is private: true
Group: core-security-release
Attachment #8579746 - Attachment is private: false
Attachment #8581001 - Attachment is private: false
You need to log in before you can comment on or make changes to this bug.