1336828 - (CVE-2017-5438) UAF in nsAutoPtr destructor during XSLT processing

Status: VERIFIED FIXED

(Core :: XSLT, defect)

Description

[Nicolas Grégoire]

Attachment: XPCshell PoC

Tested under xpcshell "JavaScript-C54.0a1"
Note: the "match" attribute of the XSLT node "template" must match the name of the XML root tag

[+] Relevant parts

xml_str = '<tag_name/>';

xsl_str = '<xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform" version="42">' +
          ' <xsl:variable name="var_name"><xsl:template/></xsl:variable>' +
          ' <xsl:template match="tag_name[$var_name]"/>' +
          '</xsl:stylesheet>'

xml = parse_str(xml_str, 'text/xml');
xsl = parse_str(xsl_str, 'text/xml');
transform(xml, xsl);

[+] How to reproduce

$ ./poc_xslt_uaf_nsAutoPtr_dtor.js

==20218==ERROR: AddressSanitizer: heap-use-after-free on address 0x60400003e410 at pc 0x7f2e219117be bp 0x7ffc99c5b7a0 sp 0x7ffc99c5b798

READ of size 8 at 0x60400003e410 thread T0
    #0 0x7f2e219117bd in ~nsAutoPtr /home/worker/workspace/build/src/obj-firefox/dist/include/nsAutoPtr.h:78:5
    #1 0x7f2e219117bd in txExecutionState::~txExecutionState() /home/worker/workspace/build/src/dom/xslt/xslt/txExecutionState.cpp:99
    #2 0x7f2e2194a015 in txMozillaXSLTProcessor::TransformToDoc(nsIDOMDocument**, bool) /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:701:1
    #3 0x7f2e2194fbdb in TransformToDocument /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:645:12
    #4 0x7f2e2194fbdb in txMozillaXSLTProcessor::TransformToDocument(nsINode&, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:1297
    ...

0x60400003e410 is located 0 bytes inside of 40-byte region [0x60400003e410,0x60400003e438)
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 0x7f2e21910697 in txExecutionState::~txExecutionState() /home/worker/workspace/build/src/dom/xslt/xslt/txExecutionState.cpp:90:9
    #2 0x7f2e2194a015 in txMozillaXSLTProcessor::TransformToDoc(nsIDOMDocument**, bool) /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:701:1
    #3 0x7f2e2194fbdb in TransformToDocument /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:645:12
    #4 0x7f2e2194fbdb in txMozillaXSLTProcessor::TransformToDocument(nsINode&, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:1297
    ...

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 0x7f2e21942bb2 in operator new /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/mozalloc.h:194:12
    #3 0x7f2e21942bb2 in txToDocHandlerFactory::createHandlerWith(txOutputFormat*, txAXMLEventHandler**) /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:93
    #4 0x7f2e21911d13 in txExecutionState::init(txXPathNode const&, txOwningExpandedNameMap<txIGlobalParameter>*) /home/worker/workspace/build/src/dom/xslt/xslt/txExecutionState.cpp:115:10
    #5 0x7f2e21949c01 in txMozillaXSLTProcessor::TransformToDoc(nsIDOMDocument**, bool) /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:672:19
    #6 0x7f2e2194fbdb in TransformToDocument /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:645:12
    #7 0x7f2e2194fbdb in txMozillaXSLTProcessor::TransformToDocument(nsINode&, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:1297
    ...

Comment 1

[Jesse Schwartzentruber (:truber)]

Attachment: testcase.html

Comment 2

[Jesse Schwartzentruber (:truber)]

Confirmed in mozilla-central rev 1cc159c7a044.

Comment 3

[Andrew Overholt [:overholt]]

Peter, can you please take a look?

Comment 4

[Peter Van der Beken [:peterv]]

We've pushed the unknown result handler onto the owning mResultHandlerStack (because we're getting a variable), but it's also owned by the execution state in mObsoleteHandler.

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: v1

This actually fixes bug 1336832 and bug 1338277 too. I have a simpler fix just for this bug and bug 1336832 though, so I'm trying to figure out if I could make one for bug 1338277, as that might be preferable for branches. If a simpler fix for bug 1338277 is too hard then we should mark the other two as dupes of this one.

Comment 6

[Julien Cristau [:jcristau]]

Peter, we're building the last beta of the 52 cycle today, and next week is RC week.  Is there any chance we can get this fixed in time for 52 RC, or should we leave it to 53 or to a possible dot release?

Comment 7

[Peter Van der Beken [:peterv]]

Attachment: v2

This is what I'd like to land on trunk. It makes us bubble up errors during matching. Currently we don't return those errors, but we do usually bail out of the function where they happened. This lead to various bugs where we'd skip some setup but didn't stop processing, and things wouldn't be set up correctly when we continued processing.
It is a big change to take on branches imo. I have smaller spot-fixes for the known issues that this causes, but there might be unknown ones. For now, I think we should take this on trunk and the spot fixes on branches.

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: Branch patch v1

This is a smaller patch that we could take on branches.

Comment 9

[Eric Rahm [:erahm]]

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

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

r=me

Comment 10

[Eric Rahm [:erahm]]

Branch patches are reviewed, looking at the larger patch now.

Comment 11

[Eric Rahm [:erahm]]

Comment on attachment 8840863 [details] [diff] [review]
v2

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

Looks good, just a few issues that need to be fixed.

::: dom/xslt/xpath/txNodeTypeTest.cpp
@@ +52,2 @@
>          }
>      }

This still needs a return and assignment to |aMatched|. I guess if we want to match the previous behavior:

> aMatched = true;
> return NS_OK;

::: dom/xslt/xpath/txUnionNodeTest.cpp
@@ +19,5 @@
> +        nsresult rv = mNodeTests[i]->matches(aNode, aContext, aMatched);
> +        NS_ENSURE_SUCCESS(rv, rv);
> +
> +        if (aMatched) {
> +            break;

This should be |return NS_OK;|

::: dom/xslt/xslt/txStylesheet.cpp
@@ -100,2 @@
>      *aImportFrame = nullptr;
> -    txInstruction* matchTemplate = nullptr;

There'd be less churn if you left this...

@@ +185,4 @@
>          }
>      }
>  
> +    return NS_OK;

...and just assigned |*aTemplate = matchTemplate| here. Either way is fine, this would just avoid a bit of blame.

::: dom/xslt/xslt/txXSLTNumber.cpp
@@ +173,5 @@
> +            if (aFromPattern && !walker.isOnNode(currNode)) {
> +                bool matched;
> +                rv = aFromPattern->matches(walker.getCurrentPosition(),
> +                                           aContext, matched);
> +                NS_ENSURE_SUCCESS(rv, rv);

All of these early returns potentially leak |countPattern|.

Comment 12

[Peter Van der Beken [:peterv]]

Attachment: v2

(In reply to Eric Rahm [:erahm] from comment #11)
> This still needs a return and assignment to |aMatched|. I guess if we want
> to match the previous behavior:

Well, that's effectively dead code, since we already handle all the enum's values. And with this we'd just get "control may reach end of non-void function" if we didn't handle all of them, so I left it as-is.

> This should be |return NS_OK;|

Good catch! Done.

> Either way is fine, this would just avoid a bit of blame.

I left it, it's a bit similar to how we deal with aImportFrame now.

> All of these early returns potentially leak |countPattern|.

Good catch, added an nsAutoPtr instead of the ownsCountPattern var.

Comment 13

[Peter Van der Beken [:peterv]]

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

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

It's not obvious from the code how to trigger the error.

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?

Patch should just apply to branches.

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

Shouldn't cause regressions, it just avoids double-deleting an object.

Note that this is a lower-risk patch intended for branches. I have a different, much bigger change for trunk. 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 only on trunk.

Comment 14

[Eric Rahm [:erahm]]

(In reply to Peter Van der Beken [:peterv] from comment #12)
> Created attachment 8842010 [details] [diff] [review]
> v2
> 
> (In reply to Eric Rahm [:erahm] from comment #11)
> > This still needs a return and assignment to |aMatched|. I guess if we want
> > to match the previous behavior:
> 
> Well, that's effectively dead code, since we already handle all the enum's
> values. And with this we'd just get "control may reach end of non-void
> function" if we didn't handle all of them, so I left it as-is.

Sounds reasonable, can you add an NS_NOTREACHED before landing? It'll make Coverity happy :)

Comment 15

[Eric Rahm [:erahm]]

Comment on attachment 8842010 [details] [diff] [review]
v2

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

Thanks Peter, looks good.

Comment 16

[Peter Van der Beken [:peterv]]

Attachment: v2

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

There's a hint that errors in matching cause issues, but don't think it's easy to construct an exploit based on that.

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

The testcases do.

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?

I have separate smaller patches to deal with the specific issues caused by this architectural issue that we've found so far. I'd like to take the smaller patches on branches, and let this big change just ride the trains.

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

There might be regressions, because we're now reporting errors in places where we didn't before.

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 three tests in the patch. 

Please open a separate bug for checking in the tests. Maybe even split them off and make a new patch of jsut the fix here. The tests patch won't be allowed to be checked in until we ship a release containing the fix. Otherwise, your tests 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/e4062da24f6b

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/4be6ea42dc48f0c534323b54707edf3c1e2f589b for build bustage:

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

Comment 20

[Peter Van der Beken [:peterv]]

So, uhm, I was going to take care of these checkins tomorrow, the 21st. I was also not going to check in the tests. It seems this was checked in today *with* the tests.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Sorry about that, was trying to time things to make tomorrow's merges to m-c and missed that the tests were there too :(

Comment 22

[Peter Van der Beken [:peterv]]

Comment on attachment 8843257 [details] [diff] [review]
v2

Moved this to bug 1349717, going to use this bug to keep track of attachment 8840864 [details] [diff] [review] (which I'll land on trunk and branches first).

Comment 23

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1da1e06485ad1b500913ad076e320e505cf019b
Bug 1336828 - Ignore the obsolete handler if the transform fails, r=erahm.

Comment 24

[Carsten Book [:Tomcat]]

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

Comment 25

[Peter Van der Beken [:peterv]]

Attachment: Branch patch v1

Same as attachment 8840864 [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 8834045 [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 26

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

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

Fix for sec-high issue, already on m-c, let's bring this to beta 7.

Comment 27

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/487284fbb1d6960efe08b63eee69802f7742d789
https://hg.mozilla.org/releases/mozilla-beta/rev/fa31c6b8354559a6fad9390a843f0f3bc636f849

Comment 28

[Peter Van der Beken [:peterv]]

Comment on attachment 8851085 [details] [diff] [review]
Branch patch 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 29

[Daniel Veditz [:dveditz]]

Peter: the original patch in this bug is quite extensive. More than we wanted for branches, but presumably changes we need going forward. Is there a follow-up bug to make those changes?

Comment 30

[Julien Cristau [:jcristau]]

Daniel, I believe bug 1349717 is the follow-up bug.

Comment 31

[Peter Van der Beken [:peterv]]

(In reply to Daniel Veditz [:dveditz] from comment #29)
> Peter: the original patch in this bug is quite extensive. More than we
> wanted for branches, but presumably changes we need going forward. Is there
> a follow-up bug to make those changes?

Yes, bug 1349717.

Comment 32

[Julien Cristau [:jcristau]]

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

uaf fix for esr45 and esr52

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/154c93b9435b
https://hg.mozilla.org/releases/mozilla-esr45/rev/982cfe33c513

Comment 34

[Andrei Vaida [:avaida]]

Flagging this for manual testing, per Comment 25.

Comment 35

[Paul Silaghi, QA [:pauly]]

Reproduced on Fx 52, Ubuntu 14.04 x64.
Verified fixed Fx 53b10, 54.0a2 (2017-04-10), 55.0a1 (2017-04-10), 45.8.1 ESR tinderbox (2017-04-10), 52.0.3 ESR tinderbox (2017-04-10).