1144988 - (CVE-2015-0818) Same-origin bypass via SVG hash navigation (ZDI-CAN-2825)

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[Daniel Veditz [:dveditz]]

Attachment: 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).

Comment 1

[Daniel Holbert [:dholbert]]

(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.

Comment 2

[Jet Villegas (inactive)]

(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.

Comment 3

[Daniel Holbert [:dholbert]]

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.

Comment 4

[Daniel Holbert [:dholbert]]

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

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: (part of testcase for the SVG bug here, which is now spun off to bug 1145195)

Comment 6

[Daniel Holbert [:dholbert]]

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.

Comment 7

[Daniel Holbert [:dholbert]]

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.

Comment 8

[Robert Longson [:longsonr]]

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.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: possible patch

I think we want this or something close to this - do the scary stuff
at the end of doShortCircuitedLoad

Comment 11

[Boris Zbarsky [: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...

Comment 12

[Boris Zbarsky [:bzbarsky]]

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.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

(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?

Comment 14

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Preparing a patch to follow the approach from comment 12.


(looks like resize event could be a way to trigger JS when flushing layout.)

Comment 16

[Daniel Veditz [:dveditz]]

(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.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Attachment: -w patch for block-loading approach

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Attachment: block-loading approach

Comment 19

[Daniel Holbert [:dholbert]]

(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.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8580099 [details] [diff] [review]
block-loading approach

r=me based on the diff -w

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Attachment: block-loading approach (for beta)

Docshell uses still the old coding style in beta so need a separate patch to
deal with 4 spaces indentation etc.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 23

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

Comment on attachment 8580099 [details] [diff] [review]
block-loading approach

We'll want beta, aurora, and ESR31 patches too, for completeness.

Comment 24

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

I assume this should be rated sec-critical?

Comment 25

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

Does the patch apply to ESR31?

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Let me clone esr31 and check.

Comment 27

[Daniel Veditz [:dveditz]]

(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.

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Attachment: block-loading approach (for esr31)

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d94c4cf9813

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8580119 [details] [diff] [review]
block-loading approach (for beta)

Approval Request Comment
See comment 30

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8580301 [details] [diff] [review]
block-loading approach (for esr31)

[Approval Request Comment]
See comment 30

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Test failures. Session restore is doing something a bit unexpected. looking.

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/61661f98f3e3 while smaug looks into this.

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

Attachment: limit the blocking only to around ScrollToAnchor (trunk, aurora)

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Attachment: limit the blocking only to around ScrollToAnchor (beta, release)

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

Attachment: limit the blocking only to around ScrollToAnchor (esr31)

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

(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")

Comment 40

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(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

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Can we get the branch patches formatted as patches and not raw diffs?

Comment 42

[Boris Zbarsky [:bzbarsky]]

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.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/86d0cb51955b
https://hg.mozilla.org/releases/mozilla-beta/rev/9b93e6033d5d
https://hg.mozilla.org/releases/mozilla-release/rev/d5a003cc284a
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/76bf2f0c7f07
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/4e229bde42dd
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/39bd4902518f
https://hg.mozilla.org/releases/mozilla-esr31/rev/56363f079b5e (GECKO3150esr_2015021713_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-esr31/rev/58c0e7447cbf (default)

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ec28e56febf1

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/9b93e6033d5d
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/76bf2f0c7f07
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/4e229bde42dd

Comment 46

[Bogdan Maris, Desktop Test Engineering]

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]?

Comment 47

[Lawrence Mandel [:lmandel] (use needinfo)]

ni dveditz and abillings to comment on comment 46 as well.

Comment 48

[Daniel Holbert [:dholbert]]

(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].

Comment 49

[Robert Kaiser]

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]"?

Comment 50

[Robert Kaiser]

Adding ni? for dholbert for my question, but if someone else can answer this, please remove it.

Comment 51

[Ioana Chiorean]

Attachment: 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

Comment 52

[Ioana Chiorean]

Attachment: Screenshot_2015-03-20-17-42-49.png

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.

Comment 53

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 54

[Boris Zbarsky [:bzbarsky]]

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.

Comment 55

[Daniel Holbert [:dholbert]]

(In reply to Robert Kaiser (:[e-mail]) 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.)

Comment 56

[Daniel Holbert [: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.)

Comment 57

[Ioana Chiorean]

(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?

Comment 58

[Daniel Holbert [: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?)

Comment 59

[Ioana Chiorean]

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.

Comment 60

[Matt Wobensmith [:mwobensmith][:matt:]]

This is still broken on ESR Fx31.5.1. I've looked at Mac/Windows (at least) and the values set in the hash/function appear in the alert box. 

I also tried Fx36.0.3 on Mac to see what it looks like there, and same behavior - appears broken. I'm not sure if that agrees with Ioana's observation in comment 51, but from my perspective, this bug has not been fixed anywhere that I've tested it.

Comment 61

[Matt Wobensmith [:mwobensmith][:matt:]]

FWIW, I tested this by logging into bugzilla and viewing the test case with the URL parameters/hash. I also downloaded the test case and ran locally, again including the URL parameters/hash. Same results for both.

Comment 62

[Boris Zbarsky [:bzbarsky]]

> the values set in the hash/function appear in the alert box. 

As in, the alert box says "function MessageChannel { [native code] }" or so?

Comment 63

[Daniel Holbert [:dholbert]]

Matt, it sounds like you're testing using my testcase in comment 6 -- is that correct?

If so, you're not actually testing this bug (not the part that was fixed anyway). You're testing bug 1145195.  To test this bug, run the original PoC, and see if you get an alert like comment 59.

Comment 64

[Matt Wobensmith [:mwobensmith][:matt:]]

Darn, you are correct, and I am wrong. I did not parse this bug correctly, and I indeed did exactly what you mentioned. I am very sorry.

I will run the test again and report back immediately.

Comment 65

[Daniel Holbert [:dholbert]]

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?

Comment 66

[Daniel Holbert [:dholbert]]

[autohiding comments 60-64, to make this bug a bit less confusing]

Comment 67

[Matt Wobensmith [:mwobensmith][:matt:]]

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.

Comment 68

[Daniel Holbert [:dholbert]]

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/[e-mail]!/content/PdfStreamConverter.jsm" line: 903}]

Seems like PDF Viewer is just broken on Android right now. Oops.

Comment 69

[Daniel Holbert [:dholbert]]

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)

Comment 70

[Daniel Holbert [:dholbert]]

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.)

Comment 71

[Daniel Holbert [:dholbert]]

[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. ]

Comment 72

[Daniel Holbert [:dholbert]]

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.

Comment 73

[Daniel Holbert [:dholbert]]

(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/

Comment 74

[Andrew McCreight [:mccr8]]

Mariusz filed a followup, bug 1145870, that says this patch is not a complete fix.

Comment 75

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This is not fixed on ESR.

Comment 76

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 77

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: XSS POC variant for ESR31

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.

Comment 78

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 79

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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).

Comment 80

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch for ESR31

See my previous comment.  With this backport ESR passes dbaron's testcase in this bug.

Comment 81

[Daniel Veditz [:dveditz]]

I believe the questions asked for me have been answered. Sorry I've been offline traveling.

Comment 82

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch for ESR31

Comment 83

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8580382 [details] [diff] [review]
limit the blocking only to around ScrollToAnchor (trunk, aurora)

r=me

Comment 84

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch for ESR31

Comment 85

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8581025 [details] [diff] [review]
Patch for ESR31

r=me, as an addition to that other patch.

Comment 86

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/releases/mozilla-esr31/rev/5e0631195898

Comment 87

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

And on default:

http://hg.mozilla.org/releases/mozilla-esr31/rev/72dd5f988428

Comment 88

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 89

[u279076]

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.

Comment 90

[u279076]

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.

Comment 91

[Kevin Brosnan [Ex-Mozilla]]

Tested Android ARM and x86 builds. Firefox 36.0.2 alerts, Firefox 36.0.4 does not.

Comment 92

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/bd3c9050906f
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/761495056f96

Comment 93

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/bd3c9050906f

Comment 94

[Bogdan Maris, Desktop Test Engineering]

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).

Comment 95

[Christiane Ruetten [INACTIVE]]

Why adv-, Al?

Comment 96

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

(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.