1336832 - (CVE-2017-5440) UAF in txExecutionState destructor during XSLT processing

Status: VERIFIED FIXED

(Core :: XSLT, defect)

Description

[Nicolas Grégoire]

Tested under xpcshell "JavaScript-C54.0a1"

[+] Relevant parts

xsl_str = '<xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform" version="1.0">' +
          '  <xsl:variable name="var_1">' +
          '    <xsl:for-each select="/">' +
          '      <xsl:value-of select="count()"/>' +
          '    </xsl:for-each>' +
          '  </xsl:variable>' +
          '  <xsl:template match="/">' +
          '    <xsl:value-of select="//*[1 = $var_1]"/>' +
          '  </xsl:template>' +
          '</xsl:stylesheet>'

[+] How to reproduce

$ ./poc_xslt_uaf_txExecutionState_dtor.js
==20312==ERROR: AddressSanitizer: heap-use-after-free on address 0x603000060790 at pc 0x7ff772355740 bp 0x7ffd9f4ad0c0 sp 0x7ffd9f4ad0b8
READ of size 8 at 0x603000060790 thread T0
    #0 0x7ff77235573f in txExecutionState::~txExecutionState() /home/worker/workspace/build/src/dom/xslt/xslt/txExecutionState.cpp:98:5
    #1 0x7ff77238e015 in txMozillaXSLTProcessor::TransformToDoc(nsIDOMDocument**, bool) /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:701:1
    #2 0x7ff772393bdb in TransformToDocument /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:645:12
    ...

0x603000060790 is located 0 bytes inside of 24-byte region [0x603000060790,0x6030000607a8)
freed by thread T0 here:
    #0 0x4b20db in free /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:38:3
    #1 0x7ff772363cdc in txLoopNodeSet::execute(txExecutionState&) /home/worker/workspace/build/src/dom/xslt/xslt/txInstructions.cpp:477:9
    #2 0x7ff7723c4aed in txXSLTProcessor::execute(txExecutionState&) /home/worker/workspace/build/src/dom/xslt/xslt/txXSLTProcessor.cpp:49:14
    #3 0x7ff77238dc18 in txMozillaXSLTProcessor::TransformToDoc(nsIDOMDocument**, bool) /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:676:14
    #4 0x7ff772393bdb in TransformToDocument /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:645:12
     ...

previously allocated by thread T0 here:
    #0 0x4b23fb in __interceptor_malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:52:3
    #1 0x4df2ad in moz_xmalloc /home/worker/workspace/build/src/memory/mozalloc/mozalloc.cpp:83:17
    #2 0x7ff772355bdc in operator new /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/mozalloc.h:194:12
    #3 0x7ff772355bdc in txExecutionState::init(txXPathNode const&, txOwningExpandedNameMap<txIGlobalParameter>*) /home/worker/workspace/build/src/dom/xslt/xslt/txExecutionState.cpp:110
    #4 0x7ff77238dc01 in txMozillaXSLTProcessor::TransformToDoc(nsIDOMDocument**, bool) /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:672:19
    #5 0x7ff772393bdb in TransformToDocument /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:645:12
    ...

Comment 1

[Nicolas Grégoire]

Can't upload PoC...
Problem on https://bugzilla.mozilla.org/attachment.cgi?

Comment 2

[Nicolas Grégoire]

PoC can be downloaded from http://nicob.net/firefox-od9Gooxu/poc_xslt_uaf_txExecutionState_dtor.js

Comment 3

[Nicolas Grégoire]

Attachment: XPCshell PoC

Comment 4

[Jesse Schwartzentruber (:truber)]

Attachment: testcase.html

Comment 5

[Jesse Schwartzentruber (:truber)]

Confirmed in mozilla-central rev 1cc159c7a044.

Comment 6

[Andrew Overholt [:overholt]]

Peter, can you please take a look?

Comment 7

[Peter Van der Beken [:peterv]]

Oh boy, we push a context from txPushNewContext::execute, txExecutionState::getVariable fails and then does popAndDeleteEvalContextUntil(mInitialEvalContext), the error code gets eaten by txPredicatedNodeTest::matches and then we end up in txLoopNodeSet::execute which tries to pop the context pushed by txPushNewContext::execute.

We really need to propagate errors in matching, but that's a pretty invasive change :-(.

Comment 8

[Peter Van der Beken [:peterv]]

And I think bug 1336828 is fundamentally the same issue, so maybe we should just bite the bullet.

Comment 9

[Julien Cristau [:jcristau]]

Tracking sec-high issue for 52/53/54.

Comment 10

[Peter Van der Beken [:peterv]]

Attachment: Branch patch v1

This is a spot fix we should take on branches, the underlying issue would be fixed by the trunk patch in bug 1336828.

Comment 11

[Eric Rahm [:erahm]]

Comment on attachment 8840865 [details] [diff] [review]
Branch patch v1

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

r=me assuming only one context can be left on the stack

Comment 12

[Eric Rahm [:erahm]]

Comment on attachment 8840865 [details] [diff] [review]
Branch patch v1

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

Sorry, missed something. r+ as long as it's okay to pop the mInitialEvalContext.

::: dom/xslt/xslt/txExecutionState.cpp
@@ +165,5 @@
>  void
> +txExecutionState::popAndDeleteEvalContext()
> +{
> +  auto ctx = popEvalContext();
> +  if (ctx && ctx != mInitialEvalContext) {

Well one question, is it okay to pop the initial context or should we do a mEvalContext.peek() first? Considering we return NS_OK below it seems like we need to be in whatever state OK implies.

Comment 13

[Peter Van der Beken [:peterv]]

Attachment: Branch patch v2

Actually, we're not popping anything in this case, because the stack is empty already (popEvalContext returns what used to be in mEvalContext). Checking for mInitialEvalContext before deleting is still ok (mEvalContext could contain mInitialEvalContext at various points). But it might be better to add a check for |mEvalContextStack.isEmpty()| before trying to pop here.

Comment 14

[Eric Rahm [:erahm]]

Comment on attachment 8842007 [details] [diff] [review]
Branch patch v2

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

Thanks Peter, looks good.

Comment 15

[Peter Van der Beken [:peterv]]

Comment on attachment 8842007 [details] [diff] [review]
Branch patch v2

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

Not easily I think.

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

The testcase does.

Which older supported branches are affected by this flaw?

All.

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

All branches are affected.

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

Should just apply to branches.

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

Shouldn't cause regressions, it avoids double-deleting a pointer.

Note that this is a lower-risk patch intended for branches. I have a different, much bigger change for trunk (in bug 1336828). Please let me know if it's ok to not land the branch patch on trunk, otherwise we could land this on trunk too, and then backout and land the riskier change from bug 1336828 only on trunk.

Comment 16

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

As with your other patches, I'll give sec-approval+ for the FIX ONLY to be checked into the tree on March 21. You do *not* have approval to check in tests then. 

Tests need to not go in for sec-high and sec-critical bugs until after we ship the fix for the security problem or we 0day our own users because people watch our security checkins.

Please separate the patches and create a separate bug to check in tests.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Calling Fx55 fixed by bug 1336828. Please request Aurora/Beta/ESR52/ESR45 approval on this when you get a chance.

Comment 18

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bab10e543e04ef925b3b22fb5230f8b1df7f9d3a
Bug 1336832 - Properly delete pushed eval contexts, r=erahm.

Comment 19

[Carsten Book [:Tomcat]]

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

Comment 20

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

Peter, can you request uplift to aurora, beta, and esr52? (as Al says, on patches without tests)

Comment 21

[Peter Van der Beken [:peterv]]

Attachment: Branch patch v2

Same as attachment 8842007 [details] [diff] [review], but without the automated testcase.

Approval Request Comment
[Feature/Bug causing the regression]: XSLT.
[User impact if declined]: Crash.
[Is this code covered by automated tests?]: Yes, but the crashtest for this specific issues will be landed separately.
[Has the fix been verified in Nightly?]: I verified that the testcase doesn't crash anymore.
[Needs manual test from QE? If yes, steps to reproduce]: Yes, until we can land the automated test. To reproduce: load attachment 8834051 [details], shouldn't crash.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low-risk
[Why is the change risky/not risky?]: It's a narrow spot-fix to avoid double-deleting an object (the architectural change was moved to a separate bug).
[String changes made/needed]: None.

Comment 22

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

Comment on attachment 8851089 [details] [diff] [review]
Branch patch v2

Fix for sec-high issue, let's try to get this into the next beta (7) build.

Comment 23

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

Peter, can you also nominate this for esr52 uplift? Thanks.

Comment 24

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7d216471a7c2c4c25c0d6b15ce8ae61486ab8f57
https://hg.mozilla.org/releases/mozilla-beta/rev/6188b6cab49e2bfa036b24d8112cda5f5f9416f0

Comment 25

[Peter Van der Beken [:peterv]]

Comment on attachment 8851089 [details] [diff] [review]
Branch patch v2

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
User impact if declined: crash
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): low-risk, narrow spot-fix to avoid double-deleting an object
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 26

[Julien Cristau [:jcristau]]

Comment on attachment 8851089 [details] [diff] [review]
Branch patch v2

fix sec-high uaf/double-free in esr45/esr52

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/d88bd03d1234
https://hg.mozilla.org/releases/mozilla-esr45/rev/ebeb0b45a84b

Comment 28

[Andrei Vaida [:avaida]]

Flagging this for manual testing, per Comment 21.

Comment 29

[Camelia Badau [:cbadau], Desktop Test Engineering]

Reproduced the issue on Ubuntu 16.04 x64 using Nightly 54.0a1 (2017-02-05).

Verified fixed on Ubuntu 16.04 x64 using latest Nightly 55.0a1 (2017-04-10), latest Aurora 54.0a2 (2017-04-10) and Firefox 53 Beta 10 (buildID: 20170407103631).

I will verify the issue on Firefox 52.1.0 ESR and Firefox 45.9.0 ESR once the builds are available.

Comment 30

[Camelia Badau [:cbadau], Desktop Test Engineering]

Verified fixed on Ubuntu 14.04 x64 using Firefox 52.1.0 ESR and Firefox 45.9.0 ESR.