1088635 - (CVE-2014-1592) 1410H - Firefox 32,33 xul.dll!nsHtml5TreeOperation use-after-free (poisoned)

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[SkyLined]

Attachment: repro.html

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0
Build ID: 20141011015303

Steps to reproduce:

<script>
  window.onload = function() {
    oMenuElement = document.createElement("MENU");
    document.open();
    document.insertBefore(oMenuElement, null);
    document.write("<script></"+"script>\n");
  };
</script>


Actual results:

Access Violation around 0x5a5a5a5a


Expected results:

Synopsis
--------
Firefox can be made to use data from a freed and poisoned object through
specially crafted HTML, resulting in access violations around address
0x5a5a5a5a in x66 systems.

Known affected versions
-----------------------
  + Mozilla Firefox 32 & 33
    (other versions not tested)

Attack vectors
--------------
  * Mozilla Firefox 32 & 33
    
    An attacker would need to get a target user to open a specially crafted
    webpage.

Mitigations
-----------
  * Mozilla Firefox 32 & 33
    
    Disabling JavaScript should prevent an attacker from triggering the
    vulnerable code path.

Description
-----------
The repro causes the nsHtml5TreeOperation's mOne member object to be freed. The
freed memory is not released, but poisoned by filling it with "z" (0x5a). A
pointer to the freed object is kept in mOne and  continues to be used. The repro
causes xul!nsHtml5TreeOperation::Perform to pass the "node" member of mOne to
xul!nsHtml5TreeOperation::AppendText as a pointer to an object, but the pointer
is 0x5a5a5a5a and this will cause an access violation.

http://lxr.mozilla.org/mozilla-central/source/parser/html/nsHtml5TreeOperation.cpp#687

Exploit
-------
I am not sure if this is exploitable, as I am not familiar with the heap
management and mitigations (such as poisoning) of Firefox. I am reporting this
as a security issue in case the poisoning is temporarily and the object can get
released and reused, or in case a heap spray could allocate memory at the
address 0x5a5a5a5a.

Comment 1

[SkyLined]

I would like to know a little more about frame poisoning in Firefox, so I can distinguish between security issues and non-exploitable crashes. I was unable to find any documentation online and I don't have the time to reverse the code to figure it out for myself. If there are any resources that can give me crash course, I would appreciate it!

Comment 2

[Benjamin Smedberg]

Memory poisoning is a technique to make exploitation harder, but a good heap-spray could still cause problems. Initially calling this critical based on UAF symptoms.

Comment 3

[Benjamin Smedberg]

https://crash-stats.mozilla.com/report/index/9f85bf04-2a43-4ab4-8997-3ad382141024

This crash signature is also tracked by bug 1047695.

Comment 4

[SkyLined]

Slight modifications to the repro can cause the crash to happen in nsHtml5TreeOperation::Append and probably other functions as well (see the switch in nsHtml5TreeOperation::Perform) - there are probably more crash signatures.

Comment 5

[Andrew McCreight [:mccr8]]

(In reply to SkyLined from comment #1)
> I would like to know a little more about frame poisoning in Firefox, so I
> can distinguish between security issues and non-exploitable crashes.

Well, 0x5a is the value we use with jemalloc to poison.  I don't recall offhand what the frame poisoning looks like.

Here's a crash report from this test case:
  https://crash-stats.mozilla.com/report/index/43b874d1-1ce1-4491-bc71-44d522141026

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> This crash signature is also tracked by bug 1047695.

FWIW, the crash in that bug, and what shows up on crash-stats, is all null derefs, mostly on OSX.

Comment 6

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

If I read this right...

We fail to add a root element 
###!!! ASSERTION: Inserting element child when we already have one: 'Error', file dom/base/nsDocument.cpp, line 4016
and http://hg.mozilla.org/mozilla-central/annotate/441055ff60e5/parser/html/nsHtml5Parser.cpp#l382 causes executor's mBroken to be true, but we don't deal with that.
So we then enter to http://hg.mozilla.org/mozilla-central/annotate/441055ff60e5/parser/html/nsHtml5Parser.cpp#l441

We don't crash if nsHtml5TreeOpExecutor::FlushDocumentWrite() checks not only 
MOZ_UNLIKELY(!mParser) but also NS_FAILED(IsBroken()).
But not sure that is the right or best fix for this.

Comment 7

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

Oh, aParent is 0xc0dedbad in nsHtml5TreeOperation::AppendText (in debug builds).
That is set in nsHtml5TreeBuilder::AllocateContentHandle().

How are we even trying to use 0xc0dedbad as nsIContent* here.

Comment 8

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

FlushDocumentWrite stops processing ops once something fails. That may include not processing 
eTreeOpCreateElement(Not)Network. Yet then in nsHtml5Parser::Parse we continue parsing.

So, a regression from bug 943519, as far as I see.
Commenting out
if (NS_FAILED(rv)) {
  MarkAsBroken(rv);
  break;
}
in FlushDocumentWrite helps.

But still not sure what is the safest option to fix this on branches, and what is the right fix for this on trunk.
Does the spec define what the parser should do in this case?

Comment 9

[Henri Sivonen (:hsivonen)]

(In reply to Olli Pettay [:smaug] from comment #8)
> FlushDocumentWrite stops processing ops once something fails. That may
> include not processing 
> eTreeOpCreateElement(Not)Network.

Yes.

> Yet then in nsHtml5Parser::Parse we
> continue parsing.

That's the security bug. Clearly, we don't check whether the parser has been marked as broken in all places where we should check.

> Does the spec define what the parser should do in this case?

It says: "If the adjusted insertion location cannot accept more elements, e.g. because it's a Document that already has an element child, then the newly created element is dropped on the floor."

That is, instead of getting marked as broken, the parser should gracefully proceed to accept input even though the nodes don't get inserted in a document.

Comment 10

[Henri Sivonen (:hsivonen)]

Attachment: Fix v1

This makes opt-builds do the spec-wise right thing. Even though that change makes the parser no longer marked as broken with this test case, the patch also adds more brokenness checks where appropriate. (Triggered e.g. if we run out of memory when adding text to the DOM.)

This patch is still wrong in the sense that it relies on executing a code path that has NS_ERROR() here:
nsresult
nsDocument::InsertChildAt(nsIContent* aKid, uint32_t aIndex,
                          bool aNotify)
{
  if (aKid->IsElement() && GetRootElement()) {
    NS_ERROR("Inserting element child when we already have one");
    return NS_ERROR_DOM_HIERARCHY_REQUEST_ERR;
  }

  return doInsertChildAt(aKid, aIndex, aNotify, mChildren);
}
...and when assertions are treated as fatal in debug builds, this is not OK.

I suggest we address this simply by downgrading the NS_ERROR here. smaug, does that make sense?

Comment 11

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

Yeah, I think it is fine to use warning there.

Comment 12

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

Comment on attachment 8511965 [details] [diff] [review]
Fix v1

>       if (mTreeBuilder->HasScript()) {
>         mTreeBuilder->Flush(); // Move ops to the executor
>         mExecutor->FlushDocumentWrite(); // run the ops
>+        if (NS_FAILED(rv = mExecutor->IsBroken())) {
>+          return rv;
>+        }
I wonder if FlushDocumentWrite could just return nsresult.
The code wouldn't look so special.

Comment 13

[Henri Sivonen (:hsivonen)]

(In reply to Olli Pettay [:smaug] from comment #12)
> I wonder if FlushDocumentWrite could just return nsresult.
> The code wouldn't look so special.

I started writing it this way. Then I figured that it would require FlushDocumentWrite to return brokenness status that's set indirectly during that call.

I'll try to make it that way, which will involve a slight change to brokenness propagation from the tree builder.

Comment 14

[David Bolter [:davidb] (NeedInfo me for attention)]

[Tracking Requested - why for this release]:

Comment 15

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

davidb - Does this impact ESR 31?

Comment 16

[Andrew McCreight [:mccr8]]

Henri is a better person to ask.

Comment 17

[Henri Sivonen (:hsivonen)]

Attachment: Fix v2

Let's make the code less weird in how nsresult propagates.

Comment 18

[Henri Sivonen (:hsivonen)]

Attachment: Crashtest and comment for the code

Comment 19

[Henri Sivonen (:hsivonen)]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #15)
> davidb - Does this impact ESR 31?

Comment 8 suggests it does.

Comment 20

[Henri Sivonen (:hsivonen)]

Comment on attachment 8515941 [details] [diff] [review]
Fix v2

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

The use-after-free circumstance is very easy to construct from the patch. It's unclear to me how easy it is to go from there to an actual expoit.

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

I have left the most obvious comment and test in a separate patch, but even this patch has an NS_WARNING that paints a bulls-eye and even if that bit didn't exist, it's pretty obvious what this patc is about.

> Which older supported branches are affected by this flaw?

Per comment 8, anything Gecko 29 and newer.

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

Per comment 8, bug 943519.

> Do you have backports for the affected branches? If not, 
> how different, hard to create, and risky will they be?

There are just three changes that don't apply cleanly to ESR31 and even those are trivial to rebase manually. Thus, backporting is very easy even though the patch doesn't apply 100% as-is.

> How likely is this patch to cause regressions; how much testing does it need?

This changes the control flow in potentially many error cases. However, many of those cases might result in security bugs without this patch, so taking the patch seems a good bet. It should be pretty clear from the patch that whenever NS_OK would have been returned previously, the control flow is the same as before, so hopefully the risk of regressions for normal cases is low.

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 8515941 [details] [diff] [review]
Fix v2

[Triage Comment]
sec-approval+, a=dveditz

Please land everywhere asap so we can get some beta testing. Don't land the tests until after we've released the fix (use in-testsuite? flag to track or file a separate "land the tests for xxxx" bug).

Comment 22

[Henri Sivonen (:hsivonen)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e066f112521a
https://hg.mozilla.org/releases/mozilla-aurora/rev/07a3bd26b6c1
https://hg.mozilla.org/releases/mozilla-beta/rev/2be3d4150683

Comment 23

[Henri Sivonen (:hsivonen)]

Attachment: Rebase to ESR31

This rebase is so trivial that I think the same review applies.

Comment 24

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

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

Comment 25

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Comment on attachment 8517500 [details] [diff] [review]
Rebase to ESR31

You still need to fill out an approval request on this.

Comment 26

[Henri Sivonen (:hsivonen)]

Comment on attachment 8517500 [details] [diff] [review]
Rebase to ESR31

[Approval Request Comment]
> User impact if declined: 

At risk of remote code execution exploit via reliably reproducible use-after-free.

> Fix Landed on Version:
34, 35, 36

> Risk to taking this patch (and alternatives if risky): 

Should be low risk, since the usual success flows don't change.

> String or UUID changes made by this patch: 

None.

Comment 27

[Henri Sivonen (:hsivonen)]

Please consider attachment 8517500 [details] [diff] [review] ready for the "checkin?" flag to mozilla-esr31 if gets "approval-mozilla-esr31+".

Comment 28

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/46ae75f63ddc

Comment 29

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/2be3d4150683

Comment 30

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/706236c87244
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/706236c87244
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/7be59353b627

Comment 31

[SkyLined]

Two quick questions: has this been assigned a CVE id and how do I found out which Firefox update a fix for a vuln such as this released in? Is this information in the bug somewhere?

Comment 32

[Andrew McCreight [:mccr8]]

(In reply to SkyLined from comment #31)
> Two quick questions: has this been assigned a CVE id and how do I found out
> which Firefox update a fix for a vuln such as this released in? Is this
> information in the bug somewhere?

I don't think it has been assigned a CVE yet.  When it has, the CVE will be part of the summary of the bug at the top.  You can see bug 1064636 for an example of what that looks like.

You can see what versions this has been fixed in by looking at the tracking flags.  status-firefox34 is set to fixed, so that means 34 is the first version that it will be fixed in.

It will be assigned a CVE, and this bug will be updated with CVE information, by the day 34 is released.  I think that will be in another week or two.

Comment 33

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

I'll be assigning a CVE next week. It will be an "alias" on the bug at the top.

Comment 34

[Kamil Jozwiak [:kjozwiak]]

Reproduced the crash using the following builds/OS's:

Win 8.1 x64:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/33.0/win32/en-US/
- https://crash-stats.mozilla.com/report/index/54ffaf54-1e5c-46c6-87ea-2ea992141125

OSX 10.10.1:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/33.0/mac/en-US/
- https://crash-stats.mozilla.com/report/index/75777aaf-c88c-4f05-8ace-c42122141125

Ubuntu 14.04.1
- http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/33.0/linux-x86_64/en-US/
- https://crash-stats.mozilla.com/report/index/5a4c13cd-7270-4bc1-9912-fc6bc2141125

Used the following OS's for each build and test cases listed below:
- Win 8.1 x64 (VM) - PASSED
- OSX 10.10.1 - PASSED
- Ubuntu 14.04.1 (VM) - PASSED

Went through verification using the following builds:

- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-11-24-10-01-36-mozilla-central/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-11-24-00-40-01-mozilla-aurora/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/34.0b11/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-11-24-00-05-01-mozilla-esr31/

Test Cases:

- opened the poc 5x in several new windows/tabs
- opened the poc 5x in several new private wndows/tabs
- opened the poc 5x in several new e10s windows/tabs