1321066 - (CVE-2016-9079) Reported Firefox SVG 0-day (Iterator invalidation in nsSMILTimeContainer::NotifyTimeChange())

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Daniel Veditz [:dveditz]]

Attachment: 0-day sample -- BE CAREFUL, MAY NOT BE FULLY DE-FANGED

An admin at obscuredfiles.com received this sample as an email attachment. The vulnerability uses SVG animation and workers, and includes what looks like ASLR ROP programming to attack windows. The example is Firefox specific, and the letter writer speculates that some of the Obscured folks are known to use the Tor browser and maybe that's why they were sent this Firefox-specific file.

The cssbanner.js text has been pretty-printed by the obscured folks. The original was minified and they did not send us the original version.

I've commented out the original payload so hopefully this PoC will "work" but be harmless. Would appreciate people being extra careful with it in case there's a landmine hidden in the rop code that I left.

Comment 1

[Tyson Smith [:tsmith] (PTO)]

Attachment: 50.1_log.txt

Comment 2

[Tyson Smith [:tsmith] (PTO)]

Attachment: nightly_log.txt

Comment 3

[Tyson Smith [:tsmith] (PTO)]

Attachment: test_case_fixed.zip

Comment 4

[Daniel Holbert [:dholbert]]

The nightly_log.txt ASAN stacks seem to be mostly in SMIL timing-model stuff (e.g. nsSMILTimeContainer, nsSMILTimedElement, MilestoneEntry), which birtles knows the most about.

CC'ing him [he may be the best able to get to the bottom of this], but I'll take a closer look as well.

Comment 5

[Andrew McCreight [:mccr8]]

Just from the stacks, it looks like a case of iterator invalidation in nsSMILTimeContainer::NotifyTimeChange(). We iterate over an array, using a raw pointer to the elements, calling some method, and in some particular case the method we call causes the array to be reallocated, destroying the thing we're holding a pointer to.

Comment 6

[Andrew McCreight [:mccr8]]

nsTObserverArray is designed to handle this kind of reentrance issue, if it can't be prevented.

Comment 7

[Andrew McCreight [:mccr8]]

I'm not sure if this is more of a bounds failure or a UAF.

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Andrew McCreight [:mccr8] from comment #5)
> Just from the stacks, it looks like a case of iterator invalidation in
> nsSMILTimeContainer::NotifyTimeChange(). We iterate over an array, using a
> raw pointer to the elements, calling some method, and in some particular
> case the method we call causes the array to be reallocated, destroying the
> thing we're holding a pointer to.

We ought to be able to detect that in a debug build, then, or changing the assert in NotifyTimeChange into a release assert.

Comment 9

[Nathan Froyd [:froydnj]]

Though I guess if it were simply reallocated, but was the same length, the assert wouldn't fire...

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: single file test case

Here's a stripped down version of the original test case that is a single file and does not have any of the ROP/exploit/shell code in it. It still triggers the issue for me, using a patch I will attach.

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: Explicitly guard against reentrance in nsSMILTimeContainer

MozReview-Commit-ID: 9#Feedback: [e-mail]

Comment 12

[Andrew McCreight [:mccr8]]

This is basically the hackiest fix imaginable: every time we're holding one of the scary raw pointers to mMilestoneEntries, we set a field to true. Every time before we write to mMileStoneEntries, we release assert that the field is not true.

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8815469 [details] [diff] [review]
Explicitly guard against reentrance in nsSMILTimeContainer

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

Thanks for posting this!

IMO it'd make sense to take this patch as a way to neuter this attack for the moment, and then we can separately examine the (complex) logic around the reentrance here in a more measured way.

Several nits:
First, the extended commit message has "MozReview-Commit-ID: 9#Feedback: [e-mail]" on its last line. That looks like a mistake? MozReview-Commit-ID is supposed to contain a longer UUID (longer than "9" :)) -- though the UUID doesn't matter here since this isn't using MozReview.  And I've never seen this "Feedback: [e-mail]" in patch headers before, but it doesn't seem particularly useful.

::: dom/smil/nsSMILTimeContainer.cpp
@@ +277,5 @@
>  void
>  nsSMILTimeContainer::Traverse(nsCycleCollectionTraversalCallback* aCallback)
>  {
> +  MOZ_RELEASE_ASSERT(!mHoldingEntries); // XXX technically this might be okay, but then we'd have to track a depth.
> +  mHoldingEntries = true;

We should use a RAII AutoRestore here (and in the other place where we set this to true for a little while).  Then we won't have to worry about unsetting it in possible-future-early-return cases.

::: dom/smil/nsSMILTimeContainer.h
@@ +291,5 @@
>    // taken care of the milestones before the current sample time but before we
>    // actually do the full sample.
>    nsTPriorityQueue<MilestoneEntry> mMilestoneEntries;
> +
> +  bool mHoldingEntries;

Please shift this up higher to be alongside the other bools, so it has a better chance of packing nicely with another bool.  (You'll need to update the constructor init list accordingly, too, since it needs to be in the same order.)

Comment 14

[Andrew McCreight [:mccr8]]

Yeah, the weird commit message is because Emacs dies while I was editing it while uploading, so there was some extra meta data in there. Sorry about that. I'll fix up the AutoRestore stuff, I was just being lazy.

Comment 16

[Daniel Veditz [:dveditz]]

Appears to be public. mccr8 googled _resolve_pe_structures and found this pastebin from yesterday (900+ views): http://pastebin.com/7xZ27haj

Someone received a copy and posted the exploit to the tor-talk mailing list about 30 minutes ago:
https://lists.torproject.org/pipermail/tor-talk/2016-November/042639.html

Comment 17

[Andrew McCreight [:mccr8]]

This file hasn't changed substantially in 5 years, so I imagine all branches are affected.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

[Tracking Requested - why for this release]:

Comment 19

[Andrew McCreight [:mccr8]]

Attachment: Explicitly guard against reentrance in nsSMILTimeContainer

I need to do more testing (to check if this actually helps in release builds, and to make sure it doesn't break any SVG tests), but I guess it can be reviewed now.

Comment 20

[Andrew McCreight [:mccr8]]

Attachment: Explicitly guard against reentrance in nsSMILTimeContainer.

Comment 22

[Daniel Holbert [:dholbert]]

Comment on attachment 8815484 [details] [diff] [review]
Explicitly guard against reentrance in nsSMILTimeContainer.

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

r=me; two nits:

::: dom/smil/nsSMILTimeContainer.cpp
@@ +265,5 @@
>    bool gotOne = false;
>    while (!mMilestoneEntries.IsEmpty() &&
>        mMilestoneEntries.Top().mMilestone == containerMilestone)
>    {
> +    MOZ_RELEASE_ASSERT(!mHoldingEntries);

Let's pull this assert up a few lines, to happen just before the "while" loop.

(It's wasteful & unnecessary to re-check the bool on every loop iteration -- the variable isn't going to change value during the loop, except possibly during a function-call, but then that function call will restore the old value.)

@@ +276,5 @@
>  
>  void
>  nsSMILTimeContainer::Traverse(nsCycleCollectionTraversalCallback* aCallback)
>  {
> +  AutoRestore<bool> saveHolding(mHoldingEntries);

Please add the include for this at the top of this file:
#include "mozilla/AutoRestore.h"

Comment 23

[Andrew McCreight [:mccr8]]

Attachment: Explicitly guard against reentrance in nsSMILTimeContainer.

Comment 24

[Andrew McCreight [:mccr8]]

(In reply to Daniel Holbert [:dholbert] from comment #22)
> Let's pull this assert up a few lines, to happen just before the "while"
> loop.
Fixed.
 
> during a function-call, but then that function call will restore the old
> value.)

Ah, good point, I hadn't thought about that.

> Please add the include for this at the top of this file:
> #include "mozilla/AutoRestore.h"

Fixed.

I didn't see any other uses of Elements() in the SMIL directory so hopefully this pattern is not repeated elsewhere.

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d58b3d48afc1af2f772ddb92f135563d34c8c3c

Comment 25

[Tyson Smith [:tsmith] (PTO)]

Attachment: smil_crash.html

Further reduce test case

Comment 26

[Andrew McCreight [:mccr8]]

Per dholbert's suggestion, I locally ran Mochitests in content/smil/test/ and dom/svg/test/, reftests in layout/reftests/svg/smil and web platform tests in testing/web-platform/tests/svg/ and they all passed, so this at least doesn't totally break SMIL.

Comment 27

[Daniel Holbert [:dholbert]]

Attachment: reduced interactive testcase

(Here's a further-reduced testcase, based on the one Tyson just posted. It won't crash until you click a button.)

Comment 28

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for 50+

Comment 29

[Daniel Veditz [:dveditz]]

Comment on attachment 8815492 [details] [diff] [review]
Explicitly guard against reentrance in nsSMILTimeContainer.

Let's get this landed on the usual branches at least. Is this ready to land on -release and -esr or do we need more testing?

Comment 30

[Daniel Holbert [:dholbert]]

Comment on attachment 8815501 [details]
reduced interactive testcase

>    <animate id="ia" end="50s"></animate>
>    <animate begin="60s" end="ic.end"></animate>

Note: the hardcoded times in the testcase here aren't magic numbers -- they only matter a bit.  Specifically -- for the crash to happen...
 (1) The first animation's end-time must be before the second animation's begin-time.
 (2) You must to call pauseAnimations() [click the button] before the first animation has begun.

Comment 31

[Andrew McCreight [:mccr8]]

[Security approval request comment]
How easily could an exploit be constructed based on the patch? It is fairly obvious where the problem is. Also an exploit for this is already public.

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

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

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The two patched files haven't changed much in the last year, so backports should be trivial.

How likely is this patch to cause regressions; how much testing does it need? It looks fairly safe to me. My patch causes an intentional crash, but only when we're in danger of hitting memory corruption anyways. Plus I think SMIL isn't a very widely used feature, which reduces the risk.

Comment 32

[Andrew McCreight [:mccr8]]

(In reply to Daniel Veditz [:dveditz] from comment #29)
> Let's get this landed on the usual branches at least. Is this ready to land
> on -release and -esr or do we need more testing?

Like I said, this code hasn't changed in any substantial way in around 5 years so it should be okay everywhere.

Comment 33

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9271347b07d201df26cdffde75483c0b0001528c

Comment 34

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8815492 [details] [diff] [review]
Explicitly guard against reentrance in nsSMILTimeContainer.



[Triage Comment]

Comment 35

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #30)
> Note: the hardcoded times in the testcase here aren't magic numbers -- they
> only matter a bit.  Specifically -- for the crash to happen...
>  (1) The first animation's end-time must be before the second animation's
> begin-time.
>  (2) You must to call pauseAnimations() [click the button] before the first
> animation has begun.

(Sorry -- in (2), I meant "before the first animation's end-time". [It has no explicit begin time.])

Comment 36

[Andrew McCreight [:mccr8]]

For what it is worth, I think this may have been introduced in bug 474743, which landed in 2010.

Comment 37

[Liz Henry (:lizzard) (relman/hg->git project)]

mccr8 and dholbert will land this directly on affected branches. Then I think releng will take care of getting it into the particular release relbranches.

Comment 38

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bbdf2771fc05481d45509d631c1ebf8b589dca49

Comment 39

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c736f46410bfef32acf9f38adaa930a9684b28c1

Comment 40

[Daniel Veditz [:dveditz]]

Jim, Bob: This 0-day exploit might be a good chance to test the effectiveness of the Windows sandbox, and maybe learn more what we need to do.

Comment 41

[Daniel Veditz [:dveditz]]

Looks like Thunderbird can hit this code, too: bp-11fc7f68-e391-4b23-9d01-d23702161128 (that's not me, found this UAF in crash-stats). At least no scripting in the mail view so it's not easily wormable.

Comment 42

[Andrew McCreight [:mccr8]]

ESR45:
https://hg.mozilla.org/releases/mozilla-esr45/rev/e6eb0a3a3c76f6d487402516d29bcf8183707be3

release: (thanks bz for pushing this)
https://hg.mozilla.org/releases/mozilla-release/rev/623b0925399476b55fbe2d6135eaf62d17679414

Comment 43

[Daniel Holbert [:dholbert]]

From a "is this going to break any websites" perspective:
 - This is a feature (SMIL) that was never implemented by IE/Edge.
 - Chrome implemented it but is intending to deprecate it.
 - The flaw (and the change) are in syncbase timing, which is a particularly niche sub-feature.
 - SMIL itself is very niche and doesn't get a lot of use on the web.

SO.  I think potentially-breakable-web-content would be hard to find, and our automated test suite is as good or better than looking for real-world stuff on the web, I think (in terms of "will this break anything").

Comment 44

[Daniel Holbert [:dholbert]]

(We added comprehensive tests for this sub-feature back in bug 537852. Apparently not quite comprehensive enough to have detected this bug here, but hopefully comprehensive enough to catch any breakage that we might be causing.

Plus, the nature of the patch is that *any* behavior-change will be changing us *away* from doing something unsafe which would probably already crash [and now we'll crash safely instead]. So, not particularly worrisome.)

Comment 45

[Wayne Mery (:wsmwk)]

Noted for Thunderbird. Thanks

Comment 46

[Liz Henry (:lizzard) (relman/hg->git project)]

We will need help smoketesting all affected versions once we have them built, since it is a national holiday in Romania right now (for the next few days). Sounds like we expect to still crash. The crash signature will be different.

Comment 47

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

FIREFOX_50_0_2_RELBRANCH: https://hg.mozilla.org/releases/mozilla-release/rev/cc272f7d48d3544ffaf242b51430ffcf3a932a29

Comment 48

[Al Tsai [:atsai]]

After a quick test on 51.0b5 on Mac OS X, Windows 7, and Ubuntu x86_64. We got the same result that all Firefox crashed after clicking the button on reduced interactive testcase

Comment 49

[Cynthia Tang [:cynthiatang]]

Attachment: Video for Firefox 51.0b5

Test result: https://youtu.be/4vMopBX4mTs

Comment 50

[Daniel Holbert [:dholbert]]

Yeah -- a crash is still expected, even in a "good" build with the fix.  The difference is whether the crash is safe or not.  (Per beginning of comment 13, we'd like to avoid crashing at all, but it's simpler to just abort safely in the error condition for now.)

To validate the fix, you should load the interactive testcase and trigger the crash, and then go ahead and submit the crash and check the crash signature. A "bad" crash looks like this:
bp-d49b2ac3-2bec-40b1-b633-b81cf2161130

A "good" crash will look a bit different -- it should at least have a "crash address" of 0x0 or close to that, I think.  I'm not sure exactly how it'll look because I don't have a expected-"good" build handy. Do you have a link to the build that you're testing?

Comment 51

[Al Tsai [:atsai]]

https://crash-stats.mozilla.com/report/index/8e4de233-4fb8-4057-840e-2a3f32161130

Comment 52

[Al Tsai [:atsai]]

Comment 51 for the crash report, I am not sure if the patch fixes it yet.

Comment 53

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

I crashed the linux64 50.0.2 build with the interactive testcase, and here's my crash report: https://crash-stats.mozilla.com/report/index/7b0b3ced-b715-45d3-8dd7-50b3d2161130

Comment 54

[Al Tsai [:atsai]]

Build link
https://archive.mozilla.org/pub/firefox/candidates/51.0b5-candidates/build1/

Comment 55

[Daniel Holbert [:dholbert]]

Thanks. Both comment 51 and comment 53 are "good" (safe) crashes. They both have
> MOZ_CRASH Reason 	MOZ_RELEASE_ASSERT(!mHoldingEntries)
...which indicates that we intentionally & safely crashed using one of mccr8's added release-build-asserts.

You can consider the presence of MOZ_RELEASE_ASSERT(!mHoldingEntries) in crash reports as confirmation that the patch has effectively neutralized the problem.

Comment 56

[Cynthia Tang [:cynthiatang]]

Hi Daniel,
I just tested it on Windows 10 pro (x64). The crash address is "0x7ff9fb1c05c0". I am not sure if we expected that. Thanks.

Crash report https://crash-stats.mozilla.com/report/index/83a20ef1-c91a-4c4b-a334-181de2161130#tab-rawdump

Comment 57

[Daniel Holbert [:dholbert]]

Thanks, Cynthia.  I believe the presence of "MOZ_RELEASE_ASSERT" in that crash report means all is well. (Additionally, the backtrace confirms that we're crashing on that new assertion line in AddMilestone, which is just before we set ourselves up for trouble.)

I guess MOZ_RELEASE_ASSERT produces a non-null crash address on Windows for some reason; not sure why. But I think that crash report is still fine -- MOZ_RELEASE_ASSERT should be all that we need to look for here.

Comment 58

[Al Tsai [:atsai]]

Currently, all tests look good to me.

We tested:
- 45.5.1esr
- 50.0.2
- 51.0b5

platform:
- Windows 7 64bit
- Mac OS X 10.12.1
- Ubuntu 14.04 LTS 64-bit

Fix works on all 9 combinations. We also have a glance on web-compatibility and graphic smoke test and didn't find significant issues.

Comment 59

[Bob Owen (:bobowen)]

(In reply to Daniel Veditz [:dveditz] from comment #40)
> Jim, Bob: This 0-day exploit might be a good chance to test the
> effectiveness of the Windows sandbox, and maybe learn more what we need to
> do.

Indeed, do we have a test case that fully demonstrates the exploit ... in a safe way. :)

Comment 60

[Frederik Braun [:freddy]]

Yes, Bob, See attachments added in comment 25 and comment 27.

Comment 61

[Jim Mathies [:jimm]]

(In reply to Bob Owen (:bobowen) from comment #59)
> (In reply to Daniel Veditz [:dveditz] from comment #40)
> > Jim, Bob: This 0-day exploit might be a good chance to test the
> > effectiveness of the Windows sandbox, and maybe learn more what we need to
> > do.
> 
> Indeed, do we have a test case that fully demonstrates the exploit ... in a
> safe way. :)

I'm curious what hackers do once they have access to kernel32. From news articles I've read it sounds like this is being used to fingerprint people using system info. The level 1 (low integrity) sandbox on release isn't going to secure against that. I would love to get the details on what they are doing so we can use these to test the sandbox in the future. If anyone has any additional detail please send it my way.

Comment 62

[Bob Owen (:bobowen)]

(In reply to Frederik Braun [:freddyb] from comment #60)
> Yes, Bob, See attachments added in comment 25 and comment 27.

Right, but as I understand it, those just trigger the crash.
They don't exploit it to get RCE and then do something interesting with it.

Comment 63

[Frederik Braun [:freddy]]

Gotcha! According to <https://twitter.com/TheWack0lian/status/803736507521474560>, the actual exploit gets IP/MAC address and sends this to a hardcoded IP address. Safe enough to test in a VM that's offline and can be rolled back?!

Comment 64

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9271347b07d2

Comment 65

[Andrew McCreight [:mccr8]]

Not that it matters, but the initial landing on m-c was this:
  https://hg.mozilla.org/mozilla-central/rev/adcc39e3cad0f32aba0efb478cc4a023a5dfc43f
I forgot to post the link in this bug and set the flag.

Comment 66

[Wayne Mery (:wsmwk)]

https://crash-stats.mozilla.com/report/index/a99ccf9b-0064-4d38-a076-6bc8b2161130 Thunderbird 45.5.1 build2, has  	MOZ_RELEASE_ASSERT(!mHoldingEntries)  tested via thunderbrowse addon. So confirmed fixed

Comment 67

[Kamil Jozwiak [:kjozwiak]]

Using the original poc with the live payload that was added in comment#0, I went through the following verification:

Platforms Used:

* Win 7 Pro SP1 x64 -> PASSED
* Win 7 Pro SP1 x86 -> PASSED
* Win 8 Pro x64 -> PASSED
* Win 8.1 x64 -> PASSED
* Win 10 x86 -> PASSED
* Win 10 x64 -> PASSED

Builds Used:

* fx50.0.2, buildId: 20161129173726, changeset: cc272f7d48d3
* fx45.5.1, buildId: 20161129180326, changeset: f2792b829193

Ensured the following was occurring when FX crashed:

* the crash is occurring under the new assertion that was added into the "nsSMILTimeContainer::AddMilestone" member function
* MOZ_CRASH Reason: MOZ_RELEASE_ASSERT(!mHoldingEntries)

Comment 68

[u279076]

Looks good for Fennec Release on my Google Pixel XL:
* Fennec 50.0.2: bp-1af50520-ad59-46cc-9647-e192d2161130 [contains MOZ_RELEASE_ASSERT(!mHoldingEntries)]
* Fennec 50.0.1: bp-384dcb09-9c0a-430e-a960-807f82161130 [does not contain a MOZ_CRASH Reason]

Comment 69

[u279076]

Looks good for Fennec Beta on my Google Pixel XL:
* Fennec 51.0b5: bp-6c583b9b-fccb-46cc-b96b-b22912161130 [contains MOZ_CRASH Reason: MOZ_RELEASE_ASSERT(!mHoldingEntries)]
* Fennec 51.0b4: bp-e339411b-a5ae-4c1c-810c-359a22161130 [does not contain a MOZ_CRASH Reason]

Comment 70

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #13)
> IMO it'd make sense to take this patch as a way to neuter this attack for
> the moment, and then we can separately examine the (complex) logic around
> the reentrance here in a more measured way.

(Following up on this: I've now filed bug 1321357 on looking into fixing this more thoroughly & preventing any crashes (even safe ones) on testcases like the ones in this bug.)

Comment 71

[Justin [:JW_SoftvisionQA]]

Looks good on Mac 10.12 and Ubuntu 16.04

* fx50.0.2, buildId: 20161129173726, changeset: cc272f7d48d3
* fx45.5.1, buildId: 20161129180326, changeset: f2792b829193

Comment 72

[Justin [:JW_SoftvisionQA]]

Marking this a Verified now that all OS's are complete.

Comment 73

[Yuhong Bao]

Attachment: Another more real world like test case

A more real world like test case with an actual animation. Click "Pause animation 1" to trigger bug.

Comment 74

[forkest]

I have read the bug description and opened obscuredfiles.com just now, before updating to 50.0.2, the site currently has some strange suspicious message and a gif file. Can somebody from the security team  please confirm me that the gif there is safe and I'm not infected? I know that this particular bug was about SVG, but...

Comment 75

[Frederik Braun [:freddy]]

The exploit we have seen made the browser crash, after the malicious shell code was executed.
It's likely that you're safe if the browser did not crash.

Comment 76

[forkest]

Thank you, Frederik. I feel safe now.

Comment 77

[Ryan VanderMeulen [:RyanVM]]

To be clear for those arriving at this bug after the fact, with the patch that landed in this bug and shipped in Fx50.0.2/ESR45.5.1, Firefox will *still* crash when the exploit attempts to run.

However, it will now do so safely before anything exploitable has a chance to happen. You can verify that the crash happened safely by looking for the "MOZ_CRASH Reason: MOZ_RELEASE_ASSERT(!mHoldingEntries)" annotation in the crash report viewable in about:crashes. Comment 51 has a link to a crash report showing what that looks like.

Bug 1321357 tracks fixing Firefox to avoid crashing at all.

Comment 78

[Frederik Bovendeerd]

Do I understand correctly, that fixes like this (in general and in this specific case) are implemented in both the new release as well as the most recent beta version?
I cannot find an explicit listing of changes from beta 51.0b4 to 51.0b5, but assume/deduct from what I read in this thread that 51.0b5 is safe, too.
I don't want to downgrade to 50.0.2 not to be vulnerable anymore. ;)

> The exploit we have seen made the browser crash, after the malicious shell code was executed.
So, prior to the fix, the exploit would have allowed anyone to execute code, THEN the browser would crash and now it essentially crashes BEFORE any malicious code can be executed, until a cleaner fix can be found?

Thanks in advance for your answers!

Comment 79

[Ryan VanderMeulen [:RyanVM]]

(In reply to Frederik Bovendeerd from comment #78)
> Do I understand correctly, that fixes like this (in general and in this
> specific case) are implemented in both the new release as well as the most
> recent beta version?

Correct, 51b5 also contains this fix. As do the latest Nightly and DevEdition builds.
https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=FIREFOX_51_0b4_RELEASE&tochange=FIREFOX_51_0b5_RELEASE

> So, prior to the fix, the exploit would have allowed anyone to execute code,
> THEN the browser would crash and now it essentially crashes BEFORE any
> malicious code can be executed, until a cleaner fix can be found?

My understanding is that previously the browser would either crash or execute the payload depending on the circumstances. But yes, the fix that landed here makes Firefox crash in a controlled way before it ever gets to that point.

Comment 80

[Frederik Bovendeerd]

Thanks for the quick reply and the link. This is exactly what I was not able to find.

Concerning crashing or no crashing, your understanding seems to differ from Frederik Braun's. ;)
> The exploit we have seen made the browser crash, after the malicious shell code was executed.

Comment 81

[Frederik Braun [:freddy]]

Given the attention this bug is getting, I feel things are getting way too complicated for all of our users to understand.
If you are reading this because you want to dig deeper, I'm happy to explain things in #security on irc.mozilla.org.

Here's my response, none the less:

An attacker exploiting this vulnerability can do anything, once they gain control over Firefox. That's how exploits work.
The attack we've seen tried to exfiltrate data and then crashed. A more skilled attacker could just resume normal program execution. But that's not what we have seen and there's no sign someone significantly improved the attack within the 24 hours we took to get the fix out (which is indeed in all branches of Firefox, as Ryan mentioned)

Due to time constraints, the fix we have landed is not the most elegant. In the patched version, Firefox notices that something is wrong and exits abruptly (crashes), which is slightly different from a crash due to undefined behavior. See more about understanding crash reports at <https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Crash_reporting/Understanding_crash_reports>

(My comment 75 was a simplification. Given that the user in comment 74 saw the same GIF as I did (we visited the web page at roughly the same time) and that his browser did not crash, my statement still holds true.)

Comment 82

[Andrei Vaida [:avaida]]

We've also managed to confirm that this is fixed on 51.0b6-build1 (20161201172143), using the following platforms: Windows 10 x64, Windows 8.1 x86, Mac OS X 10.11.6 and Ubuntu 14.04 x86.

Updating flags accordingly.

Comment 83

[Frederik Bovendeerd]

(In reply to Frederik Braun [:freddyb] from comment #81)

> An attacker exploiting this vulnerability can do anything, once they gain
> control over Firefox. That's how exploits work.
> The attack we've seen tried to exfiltrate data and then crashed. A more
> skilled attacker could just resume normal program execution. 

Thanks for clarifying that.

I guess it just bothered me that this particular exploit was "featured" on all sorts of IT news sites and blogs and although dozens of people on every site I have found were terribly worried of having been affected by 'it' (as in the exploit with the "original" shell code, that sent IP and MAC address to France), it was never once mentioned that this combination of exploit and executed code would have crashed the browser and if you experienced no such crash, you'd most likely be fine.

In addition it was rarely mentioned that exploit and code are in essence two separate things, as in the exploit being the "backdoor" to FF and by that your system and the code being executed as the "what happens then" part.

It was always along the lines of "careful there is an exploit that sends potentially private data off to God-knows-who and there is no way to figure out if it happened to you, sorry, my bad".

Comment 84

[NoOp]

https://crash-stats.mozilla.com/report/index/d7100eb5-2b95-4c58-abda-aeec92161204
From test case comment #25
User agent: Mozilla/5.0 (X11; Linux x86_64; rv:50.0) Gecko/20100101 SeaMonkey/2.47
Build identifier: 20161201022155
Using Adrian's linux 2.47 build:
https://l10n.mozilla-community.org/~akalla/unofficial/seamonkey/nightly/latest-comm-release-linux64/

Signature 	libxul.so@0x1f5c450 | libxul.so@0x1e129fa | libxul.so@0x1f5c590 | libxul.so@0x1f556ff | libxul.so@0x1f5c75a | libxul.so@0x1f5cd21 | libxul.so@0x1f5cd8d | libxul.so@0x1f57dc0 | libxul.so@0x1f5a60b | libxul.so@0x1f5c752 | libxul.so@0x1f5cd21 | libxul.so@... 
MOZ_CRASH Reason 	MOZ_RELEASE_ASSERT(!mHoldingEntries)
Crash Reason 	SIGSEGV
Crash Address 	0x0

Comment 85

[NoOp]

From Adrian's Windows 2.47 build:
User agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:50.0) Gecko/20100101 SeaMonkey/2.47
Build identifier: 2016120109390
https://crash-stats.mozilla.com/report/index/3f8c367b-f472-4113-940b-c04262161204

Signature 	xul.dll@0x172847a | xul.dll@0x172ca14 | xul.dll@0x172e81c | xul.dll@0x172e9bc | xul.dll@0x172abcf | xul.dll@0x172442a | xul.dll@0x172b124 | xul.dll@0x172e8f3 | xul.dll@0x172e9a8 | xul.dll@0x172abcf | xul.dll@0x172442a | xul.dll@0x172b124 | xul.dll@0x17... 
MOZ_CRASH Reason 	MOZ_RELEASE_ASSERT(!mHoldingEntries)
Crash Reason 	EXCEPTION_BREAKPOINT
Crash Address 	0x6401847a
This was an upgrade from 2.46 to 2.47, I'll retest with the 12.01 build.

Comment 86

[NoOp]

From Adrian's 2.47 64bit Windows build:
User agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:50.0) Gecko/20100101 SeaMonkey/2.47
Build identifier: 20161201025712

https://crash-stats.mozilla.com/report/index/b449c3d9-336d-43cc-8dfc-35cda2161204
Signature 	xul.dll@0x1c669cd | strspn
MOZ_CRASH Reason 	MOZ_RELEASE_ASSERT(!mHoldingEntries)
Crash Reason 	EXCEPTION_BREAKPOINT
Crash Address 	0x7ffcbfa769cd

Comment 87

[NoOp]

@ryanvm: Why were my 84, 85, and 86 comments marked as 'offtopic'? SeaMonkey 2.40 has not been patched or upgraded & the only other option to avoid this issue (that I'm aware of) is to use Adrian Kalla's unofficial SeaMonkey builds. So I tested using the test case presented in comment 25 to try and determine if the builds included the fix. I gather from comment 55 that the presence of MOZ_RELEASE_ASSERT(!mHoldingEntries) means that "confirmation that the patch has effectively neutralized the problem". Given the gravity of this issue I would have thought that others in this report would appreciate that I'd taken the time to test those builds. What do you consider off topic: the 'unofficial' akalla builds or SeaMonkey?

Comment 88

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

This is not a discussion forum. If it isn't directly involved in fixing this bug somewhere, please take it to email folks.

Comment 89

[Frederik Braun [:freddy]]

"MOZ_CRASH Reason 	MOZ_RELEASE_ASSERT(!mHoldingEntries)" means it *is* patched.

Comment 90

[Richard Barnes [:rbarnes]]

Exodus Intel provided the following feedback on the patch:

=====

Regarding your patch: We have a suggestion (please bear with us because we are not developers).

This boundary condition is flawed and should be addressed.

> 310 	const MilestoneEntry* p = mMilestoneEntries.Elements();
> 311 	#if DEBUG
> 312 	uint32_t queueLength = mMilestoneEntries.Length();
> 313 	#endif
> 314 	while (p < mMilestoneEntries.Elements() + mMilestoneEntries.Length()) {
> 315 	  mozilla::dom::SVGAnimationElement* elem = p->mTimebase.get();
> 316 	  elem->TimedElement().HandleContainerTimeChange();
> 317 	  MOZ_ASSERT(queueLength == mMilestoneEntries.Length(),
> 
> 322 	const MilestoneEntry* p = mMilestoneEntries.Elements();
> 323 	#if DEBUG
> 324 	uint32_t queueLength = mMilestoneEntries.Length();
> 325 	#endif
> 326 	while (p < mMilestoneEntries.Elements() + mMilestoneEntries.Length()) {
> 327 	  mozilla::dom::SVGAnimationElement* elem = p->mTimebase.get();
> 328 	  elem->TimedElement().HandleContainerTimeChange();
> 329 	  MOZ_ASSERT(queueLength == mMilestoneEntries.Length(),

The reason it is flawed is that "p" is set on initial entry, but the boundary check against is invalid once the object changes.

The flaw should be mitigated if you change the logic of the loop and it's related boundary check to account for possible changes to the object during each iteration.

Comment 91

[Daniel Veditz [:dveditz]]

It's not clear from the formatting above, but the two chunks were supposed to be parallel columns showing the old and new versions -- in other words we did not change the loop that was the source of the problem. Instead this bug provided a stop-gap patch in the lines above that, to crash if there's an attempt to modify the object while it is in that loop.

The "real" fix that rewrites that loop is in bug 1321357 (see comment 70)