Bug 1336832 (CVE-2017-5440)

UAF in txExecutionState destructor 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:
---
Dependency tree / graph
Bug Flags:
sec-bounty +
in-testsuite ?
qe-verify +

Firefox Tracking Flags

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

Details

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

Attachments

(3 attachments, 2 obsolete attachments)

Reporter

Description

2 years ago
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
    ...
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?
Keywords: sec-high
Assignee

Comment 7

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

Comment 8

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

Updated

2 years ago
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Tracking sec-high issue for 52/53/54.
Assignee

Comment 10

2 years ago
Posted patch Branch patch v1 (obsolete) — Splinter Review
This is a spot fix we should take on branches, the underlying issue would be fixed by the trunk patch in bug 1336828.
Attachment #8840865 - Flags: review?(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
Attachment #8840865 - Flags: review?(erahm) → review+
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.
Assignee

Comment 13

2 years ago
Posted patch Branch patch v2 (obsolete) — Splinter Review
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.
Attachment #8840865 - Attachment is obsolete: true
Attachment #8842007 - Flags: review?(erahm)
Comment on attachment 8842007 [details] [diff] [review]
Branch patch v2

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

Thanks Peter, looks good.
Attachment #8842007 - Flags: review?(erahm) → review+
Assignee

Comment 15

2 years ago
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.
Attachment #8842007 - Flags: sec-approval?
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.
Attachment #8842007 - Flags: sec-approval? → sec-approval+
Calling Fx55 fixed by bug 1336828. 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: 1349717
Assignee

Updated

2 years ago
Blocks: 1349720
https://hg.mozilla.org/mozilla-central/rev/bab10e543e04
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Peter, can you request uplift to aurora, beta, and esr52? (as Al says, on patches without tests)
Assignee

Comment 21

2 years ago
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.
Attachment #8842007 - Attachment is obsolete: true
Flags: needinfo?(peterv)
Attachment #8851089 - Flags: review+
Attachment #8851089 - Flags: approval-mozilla-beta?
Attachment #8851089 - Flags: approval-mozilla-aurora?
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.
Attachment #8851089 - Flags: approval-mozilla-beta?
Attachment #8851089 - Flags: approval-mozilla-beta+
Attachment #8851089 - Flags: approval-mozilla-aurora?
Attachment #8851089 - Flags: approval-mozilla-aurora+
Peter, can you also nominate this for esr52 uplift? Thanks.
Flags: needinfo?(peterv)
Assignee

Comment 25

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

fix sec-high uaf/double-free in esr45/esr52
Attachment #8851089 - Flags: approval-mozilla-esr52?
Attachment #8851089 - Flags: approval-mozilla-esr52+
Attachment #8851089 - Flags: approval-mozilla-esr45?
Attachment #8851089 - Flags: approval-mozilla-esr45+
Group: dom-core-security → core-security-release
Whiteboard: [adv-main53+][adv-esr52.1+][adv-esr45.9+]
Alias: CVE-2017-5440
Flagging this for manual testing, per Comment 21.
Flags: qe-verify+
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.
Verified fixed on Ubuntu 14.04 x64 using Firefox 52.1.0 ESR and Firefox 45.9.0 ESR.
Status: RESOLVED → VERIFIED
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.