1311687 - (CVE-2017-5376) ASan heap UAF via xsl:key and xsl:variable

Status: VERIFIED FIXED

(Core :: XSLT, defect)

Description

[Nicolas Grégoire]

Tested with XPCshell "JavaScript-C52.0a1"

[+] XML document

<foo />

[+] XSLT code

<xsl:transform xmlns:xsl="http://www.w3.org/1999/XSL/Transform" version="2.0">
 <xsl:variable name="v">
  <xsl:foobar />
 </xsl:variable>
 <xsl:key match="/" name="k" use="$v" />
 <xsl:template match="/">
  <xsl:value-of select="key('k', '/')" />
 </xsl:template>
</xsl:transform>

[+] ASan output

==2814==ERROR: AddressSanitizer: heap-use-after-free on address 0x60300006c160 at pc 0x7f1c68baee30 bp 0x7ffdb6bc1ca0 sp 0x7ffdb6bc1c98
READ of size 8 at 0x60300006c160 thread T0
    #0 0x7f1c68baee2f in txExecutionState::~txExecutionState() /home/worker/workspace/build/src/dom/xslt/xslt/txExecutionState.cpp:98:5
    #1 0x7f1c68be6535 in txMozillaXSLTProcessor::TransformToDoc(nsIDOMDocument**, bool) /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:700:1
    #2 0x7f1c68bec01b in TransformToDocument /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:644:12
    #3 0x7f1c68bec01b in txMozillaXSLTProcessor::TransformToDocument(nsINode&, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:1321
    #4 0x7f1c6699d27d in mozilla::dom::XSLTProcessorBinding::transformToDocument(JSContext*, JS::Handle<JSObject*>, txMozillaXSLTProcessor*, JSJitMethodCallArgs const&) /home/worker/workspace/build/src/obj-firefox/dom/bindings/XSLTProcessorBinding.cpp:154:43
    #5 0x7f1c671ce5a0 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:2857:13
    #6 0x7f1c6d3c2a35 in CallJSNative /home/worker/workspace/build/src/js/src/jscntxtinlines.h:239:15
    #7 0x7f1c6d3c2a35 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:446
    #8 0x7f1c6d3a8dda in CallFromStack /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:509:12
    #9 0x7f1c6d3a8dda in Interpret(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:2922
    #10 0x7f1c6d38dbd3 in js::RunScript(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:404:12
    #11 0x7f1c6d3c5282 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:685:15
    #12 0x7f1c6d3c5b1b in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:717:12
    #13 0x7f1c6cecac60 in JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/jsapi.cpp:4333:12
    #14 0x7f1c6417b809 in ProcessFile(mozilla::dom::AutoJSAPI&, char const*, _IO_FILE*, bool) /home/worker/workspace/build/src/js/xpconnect/src/XPCShellImpl.cpp:886:31
    #15 0x7f1c6417bea2 in Process(mozilla::dom::AutoJSAPI&, char const*, bool) /home/worker/workspace/build/src/js/xpconnect/src/XPCShellImpl.cpp:943:15
    #16 0x7f1c6415a0fb in ProcessArgs /home/worker/workspace/build/src/js/xpconnect/src/XPCShellImpl.cpp:1144:16
    #17 0x7f1c6415a0fb in XRE_XPCShellMain /home/worker/workspace/build/src/js/xpconnect/src/XPCShellImpl.cpp:1578
    #18 0x4defbc in main /home/worker/workspace/build/src/js/xpconnect/shell/xpcshell.cpp:62:18
    #19 0x7f1c5d56382f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #20 0x41b958 in _start (/work/firefox/bin/xpcshell+0x41b958)

0x60300006c160 is located 0 bytes inside of 24-byte region [0x60300006c160,0x60300006c178)
freed by thread T0 here:
    #0 0x4b20ab in free /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:38:3
    #1 0x7f1c68bc7d4b in txXSLKey::testNode(txXPathNode const&, txKeyValueHashKey&, nsTHashtable<txKeyValueHashEntry>&, txExecutionState&) /home/worker/workspace/build/src/dom/xslt/xslt/txKeyFunctionCall.cpp:353:13
    #2 0x7f1c68bc7441 in txXSLKey::indexTree(txXPathNode const&, txKeyValueHashKey&, nsTHashtable<txKeyValueHashEntry>&, txExecutionState&) /home/worker/workspace/build/src/dom/xslt/xslt/txKeyFunctionCall.cpp:299:19
    #3 0x7f1c68bc6e93 in txXSLKey::indexSubtreeRoot(txXPathNode const&, nsTHashtable<txKeyValueHashEntry>&, txExecutionState&) /home/worker/workspace/build/src/dom/xslt/xslt/txKeyFunctionCall.cpp:283:12
    #4 0x7f1c68bb251e in txKeyHash::getKeyNodes(txExpandedName const&, txXPathNode const&, nsAString_internal const&, bool, txExecutionState&, txNodeSet**) /home/worker/workspace/build/src/dom/xslt/xslt/txKeyFunctionCall.cpp:221:19
    #5 0x7f1c68bc656a in getKeyNodes /home/worker/workspace/build/src/dom/xslt/xslt/txExecutionState.cpp:425:12
    #6 0x7f1c68bc656a in txKeyFunctionCall::evaluate(txIEvalContext*, txAExprResult**) /home/worker/workspace/build/src/dom/xslt/xslt/txKeyFunctionCall.cpp:89
    #7 0x7f1c68bc550a in txValueOf::execute(txExecutionState&) /home/worker/workspace/build/src/dom/xslt/xslt/txInstructions.cpp:929:19
    #8 0x7f1c68c1c30d in txXSLTProcessor::execute(txExecutionState&) /home/worker/workspace/build/src/dom/xslt/xslt/txXSLTProcessor.cpp:49:14
    #9 0x7f1c68be6138 in txMozillaXSLTProcessor::TransformToDoc(nsIDOMDocument**, bool) /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:675:14
    #10 0x7f1c68bec01b in TransformToDocument /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:644:12
    #11 0x7f1c68bec01b in txMozillaXSLTProcessor::TransformToDocument(nsINode&, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:1321
    #12 0x7f1c6699d27d in mozilla::dom::XSLTProcessorBinding::transformToDocument(JSContext*, JS::Handle<JSObject*>, txMozillaXSLTProcessor*, JSJitMethodCallArgs const&) /home/worker/workspace/build/src/obj-firefox/dom/bindings/XSLTProcessorBinding.cpp:154:43
    #13 0x7f1c671ce5a0 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:2857:13
    #14 0x7f1c6d3c2a35 in CallJSNative /home/worker/workspace/build/src/js/src/jscntxtinlines.h:239:15
    #15 0x7f1c6d3c2a35 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:446
    #16 0x7f1c6d3a8dda in CallFromStack /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:509:12
    #17 0x7f1c6d3a8dda in Interpret(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:2922
    #18 0x7f1c6d38dbd3 in js::RunScript(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:404:12
    #19 0x7f1c6d3c5282 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:685:15
    #20 0x7f1c6d3c5b1b in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:717:12
    #21 0x7f1c6cecac60 in JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/jsapi.cpp:4333:12
    #22 0x7f1c6417b809 in ProcessFile(mozilla::dom::AutoJSAPI&, char const*, _IO_FILE*, bool) /home/worker/workspace/build/src/js/xpconnect/src/XPCShellImpl.cpp:886:31
    #23 0x7f1c6417bea2 in Process(mozilla::dom::AutoJSAPI&, char const*, bool) /home/worker/workspace/build/src/js/xpconnect/src/XPCShellImpl.cpp:943:15
    #24 0x7f1c6415a0fb in ProcessArgs /home/worker/workspace/build/src/js/xpconnect/src/XPCShellImpl.cpp:1144:16
    #25 0x7f1c6415a0fb in XRE_XPCShellMain /home/worker/workspace/build/src/js/xpconnect/src/XPCShellImpl.cpp:1578
    #26 0x4defbc in main /home/worker/workspace/build/src/js/xpconnect/shell/xpcshell.cpp:62:18
    #27 0x7f1c5d56382f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)

previously allocated by thread T0 here:
    #0 0x4b23cb in __interceptor_malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:52:3
    #1 0x4df14d in moz_xmalloc /home/worker/workspace/build/src/memory/mozalloc/mozalloc.cpp:83:17
    #2 0x7f1c68baf29c in operator new /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/mozalloc.h:194:12
    #3 0x7f1c68baf29c in txExecutionState::init(txXPathNode const&, txOwningExpandedNameMap<txIGlobalParameter>*) /home/worker/workspace/build/src/dom/xslt/xslt/txExecutionState.cpp:110
    #4 0x7f1c68be6121 in txMozillaXSLTProcessor::TransformToDoc(nsIDOMDocument**, bool) /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:671:19
    #5 0x7f1c68bec01b in TransformToDocument /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:644:12
    #6 0x7f1c68bec01b in txMozillaXSLTProcessor::TransformToDocument(nsINode&, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/xslt/xslt/txMozillaXSLTProcessor.cpp:1321
    #7 0x7f1c6699d27d in mozilla::dom::XSLTProcessorBinding::transformToDocument(JSContext*, JS::Handle<JSObject*>, txMozillaXSLTProcessor*, JSJitMethodCallArgs const&) /home/worker/workspace/build/src/obj-firefox/dom/bindings/XSLTProcessorBinding.cpp:154:43
    #8 0x7f1c671ce5a0 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:2857:13
    #9 0x7f1c6d3c2a35 in CallJSNative /home/worker/workspace/build/src/js/src/jscntxtinlines.h:239:15
    #10 0x7f1c6d3c2a35 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:446
    #11 0x7f1c6d3a8dda in CallFromStack /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:509:12
    #12 0x7f1c6d3a8dda in Interpret(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:2922
    #13 0x7f1c6d38dbd3 in js::RunScript(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:404:12
    #14 0x7f1c6d3c5282 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:685:15
    #15 0x7f1c6d3c5b1b in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:717:12
    #16 0x7f1c6cecac60 in JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/jsapi.cpp:4333:12
    #17 0x7f1c6417b809 in ProcessFile(mozilla::dom::AutoJSAPI&, char const*, _IO_FILE*, bool) /home/worker/workspace/build/src/js/xpconnect/src/XPCShellImpl.cpp:886:31
    #18 0x7f1c6417bea2 in Process(mozilla::dom::AutoJSAPI&, char const*, bool) /home/worker/workspace/build/src/js/xpconnect/src/XPCShellImpl.cpp:943:15
    #19 0x7f1c6415a0fb in ProcessArgs /home/worker/workspace/build/src/js/xpconnect/src/XPCShellImpl.cpp:1144:16
    #20 0x7f1c6415a0fb in XRE_XPCShellMain /home/worker/workspace/build/src/js/xpconnect/src/XPCShellImpl.cpp:1578
    #21 0x4defbc in main /home/worker/workspace/build/src/js/xpconnect/shell/xpcshell.cpp:62:18
    #22 0x7f1c5d56382f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)

SUMMARY: AddressSanitizer: heap-use-after-free /home/worker/workspace/build/src/dom/xslt/xslt/txExecutionState.cpp:98:5 in txExecutionState::~txExecutionState()

Comment 1

[Andrew McCreight [:mccr8]]

Peter could you look at this please?

Comment 2

[Andrew McCreight [:mccr8]]

It would also be good to confirm that this affects the browser and not just XPCShell.

Comment 3

[Eric Rahm [:erahm]]

Quick summary: This is a double-delete. We have a variable |mInitialEvalContext| that we seem to treat specially, it ends up in the |mEvalContextStack| and gets deleted after being popped out of the stack in |txXSLKey::testNode| [1]. We then explicitly delete |mInitialEvalContext| in |txExecutionState::~txExecutionState| [2].

I think the presumption here is that every |pushEvalContext| call is paired with a |popEvalContext| call. The only place I see where we explicitly add |mInitialEvalContext| to the stack is in |txExecutionState::getVariable| [3] and then pop it further down [4]. My guess is we're hitting an early return path in between [5] and not popping the eval context.

I'm not well versed in the xslt code so I can't say for sure that's what's going on but it feels about right.

[1] https://dxr.mozilla.org/mozilla-central/rev/99a239e1866a57f987b08dad796528e4ea30e622/dom/xslt/xslt/txKeyFunctionCall.cpp#353
[2] https://dxr.mozilla.org/mozilla-central/rev/99a239e1866a57f987b08dad796528e4ea30e622/dom/xslt/xslt/txExecutionState.cpp#98
[3] https://dxr.mozilla.org/mozilla-central/rev/99a239e1866a57f987b08dad796528e4ea30e622/dom/xslt/xslt/txExecutionState.cpp#223
[4] https://dxr.mozilla.org/mozilla-central/rev/99a239e1866a57f987b08dad796528e4ea30e622/dom/xslt/xslt/txExecutionState.cpp#258
[5] https://dxr.mozilla.org/mozilla-central/rev/99a239e1866a57f987b08dad796528e4ea30e622/dom/xslt/xslt/txExecutionState.cpp#230,234,237,245,249,256

Comment 4

[Eric Rahm [:erahm]]

Instrumented code seems to point to the |txXSLTProcessor::execute(*this);| call [1] which is returning NS_ERROR_XSLT_EXECUTION_FAILURE. I then see a tab crash.

If I then add a pop before returning I see the standard error page instead: "Error during XSLT transformation: XSLT transformation failed."

[1] https://dxr.mozilla.org/mozilla-central/rev/99a239e1866a57f987b08dad796528e4ea30e622/dom/xslt/xslt/txExecutionState.cpp#248

Comment 5

[Eric Rahm [:erahm]]

Attachment: Pop eval context on early returns

This adds a helper RAII class that will push an eval context in its
ctor and then pop that context and optionally clean it up in its dtor. An
assertion is added to ensure that the popped context is the one that we
expect.

Peter this fixes the crash for me locally. I'm pretty sure the assertion that
the context being popped should equal the one pushed is correct, but I could
use some clarification on that point.

MozReview-Commit-ID: AUNen1xt9He

Comment 6

[Peter Van der Beken [:peterv]]

Comment on attachment 8803191 [details] [diff] [review]
Pop eval context on early returns

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

I think we should not do the cleanup inside the guard class, it's easy to get the delete argument wrong. For example, I might be missing something but the code in txNodeSorter::sortNodeSet seems to pass true for the delete argument but still does |delete aEs->popEvalContext();|?

The assert should be fine afaict.

What I think we should do is to remove push/popEvalContext, make the guard class a friend and have it manipulate the stack directly. If we convert all the callers to use the guard instead then we should be able to simply put the contexts on the stack (and make mInitialEvalContext a txSingleNodeContext member). Then we can remove all that nasty code in the txExecutionState destructor. However, I'm ok with postponing those changes to a separate bug if you think they're too invasive for a security bug.

Comment 7

[Eric Rahm [:erahm]]

(In reply to Peter Van der Beken [:peterv] from comment #6)
> Comment on attachment 8803191 [details] [diff] [review]
> Pop eval context on early returns
> 
> Review of attachment 8803191 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think we should not do the cleanup inside the guard class, it's easy to
> get the delete argument wrong. For example, I might be missing something but
> the code in txNodeSorter::sortNodeSet seems to pass true for the delete
> argument but still does |delete aEs->popEvalContext();|?

This seems reasonable, maybe just use a UniquePtr to manage the allocated context (if necessary). Or just allocate on the stack as suggested below.

> The assert should be fine afaict.
> 
> What I think we should do is to remove push/popEvalContext, make the guard
> class a friend and have it manipulate the stack directly. If we convert all
> the callers to use the guard instead then we should be able to simply put
> the contexts on the stack (and make mInitialEvalContext a
> txSingleNodeContext member). Then we can remove all that nasty code in the
> txExecutionState destructor. However, I'm ok with postponing those changes
> to a separate bug if you think they're too invasive for a security bug.

The simplest thing to do would just be to convert all the NS_ENSUREs in |txExecutionState::getVariable| to |if (NS_FAILED(...)) pop|. Lets just do that here so we can fix this quickly, I'll file a follow-up for a cleaner fix as you've suggested.

One piece to note is that |txPushNewContext::execute| pushes a context without popping it [1] and |txLoopNodeSet::execute| pops a context without pushing one [2].

[1] https://dxr.mozilla.org/mozilla-central/rev/215f9686117673a2c914ed207bc7da9bb8d741ad/dom/xslt/xslt/txInstructions.cpp#657
[2] https://dxr.mozilla.org/mozilla-central/rev/215f9686117673a2c914ed207bc7da9bb8d741ad/dom/xslt/xslt/txInstructions.cpp#477

Comment 8

[Eric Rahm [:erahm]]

Attachment: Pop eval context on early returns

Make sure the eval context stack is cleaned up on failure.

MozReview-Commit-ID: AUNen1xt9He

Comment 9

[Eric Rahm [:erahm]]

Comment on attachment 8804039 [details] [diff] [review]
Pop eval context on early returns

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

::: dom/xslt/xslt/txExecutionState.cpp
@@ -231,4 @@
>      }
>      else {
>          nsAutoPtr<txRtfHandler> rtfHandler(new txRtfHandler);
> -        NS_ENSURE_TRUE(rtfHandler, NS_ERROR_OUT_OF_MEMORY);

Our |new| is infallible, so this check isn't necessary.

Comment 10

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

Are other branches affected, or just 52?

Comment 11

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]:

Comment 12

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

Tracking for 51+ since this is sec-critical. We could still aim to fix this in 51.

Comment 13

[Eric Rahm [:erahm]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #10)
> Are other branches affected, or just 52?

This almost certainly affects all branches.

Comment 14

[Eric Rahm [:erahm]]

Peter, does the latest iteration look good?

Comment 15

[Peter Van der Beken [:peterv]]

Comment on attachment 8804039 [details] [diff] [review]
Pop eval context on early returns

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

Sorry for the delay.

Comment 16

[Eric Rahm [:erahm]]

Comment on attachment 8804039 [details] [diff] [review]
Pop eval context on early returns

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

Not sure, we do provide a crashtest.

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

The patch itself looks more like a memory leak, but again there's a crashtest.

> Which older supported branches are affected by this flaw?

All.

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

N/A

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

I'm reasonably sure the patch will apply as-is.

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

Not likely, we provide a test.

Comment 17

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

When we check this in, it needs to go in without a test until we ship the fix in a public release. Otherwise, we run the risk of 0daying ourselves.

sec-approval+ for trunk checkin on 11/29, two weeks into the new cycle. At that point, we'll want branch patches made and nominated as well.

Comment 18

[Eric Rahm [:erahm]]

(In reply to Al Billings [:abillings] from comment #17)
> When we check this in, it needs to go in without a test until we ship the
> fix in a public release. Otherwise, we run the risk of 0daying ourselves.
> 
> sec-approval+ for trunk checkin on 11/29, two weeks into the new cycle. At
> that point, we'll want branch patches made and nominated as well.

Do you want me to land this on trunk without the test on 11/29, or land this now without the test and test on 11/29?

Comment 19

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

Land nothing until 11/29. Then land the patch with no tests.
Until we have a *public* release with this fix out to users, we can't land tests because they will 0day those users if they have no release with a fix.

Comment 20

[Eric Rahm [:erahm]]

Attachment: Pop eval context on early returns

Updated patch with test removed

Comment 21

[Eric Rahm [:erahm]]

Attachment: DO NOT CHECK IN UNTIL PUBLIC: Crashtest

Split out crashtest.

Comment 22

[Eric Rahm [:erahm]]

Comment on attachment 8808264 [details] [diff] [review]
Pop eval context on early returns

(carrying forward r+)

Comment 23

[Eric Rahm [:erahm]]

Comment on attachment 8808265 [details] [diff] [review]
DO NOT CHECK IN UNTIL PUBLIC: Crashtest

(carrying forward r+)

Comment 24

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]: sec-critical

Comment 25

[Marcia Knous [:marcia]]

Tracking 53+ for this sec-critical issue.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Getting this back on the radar for 50.1 as well.

https://hg.mozilla.org/integration/mozilla-inbound/rev/667c1a30679730fbf513e310b2768bc8eea5cc51

Comment 27

[Andrew McCreight [:mccr8]]

Looks like this got backed out for a UAF.

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a3c15e0b3c4ca1a8f3f3cae1e35bdae18696f6f3

Comment 28

[Andrew McCreight [:mccr8]]

Here's a link to a failing log:
  https://treeherder.mozilla.org/logviewer.html#?job_id=40074221&repo=mozilla-inbound#L5123

Comment 29

[Eric Rahm [:erahm]]

Starting a local asan build to see if I can repro.

Comment 30

[Eric Rahm [:erahm]]

Okay I can repro, it would seem for this test we have another double-delete on |delete mInitialEvalContext|.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Aurora/Beta/Release/ESR45 approval ASAP so we can hopefully get it uplifted in time for 51b6.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Sorry, I obviously hit submit on that before noticing the last comments here :)

Comment 33

[Eric Rahm [:erahm]]

Attachment: Pop eval context on early returns

Peter, this is an update that just deletes all entries off the stack until
|mInitialEvalContext|. That allows us to cleanup after other failures and now
passes all xslt crashtests under ASAN.

MozReview-Commit-ID: AUNen1xt9He

Comment 34

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ccc5ede0fd337923f51a7ebe2bb1f6504d53fe61
Bug 1311687 - Pop eval context on early returns. r=peterv

Comment 35

[Phil Ringnalda (:philor)]

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

Comment 36

[Eric Rahm [:erahm]]

Comment on attachment 8815911 [details] [diff] [review]
Pop eval context on early returns

Approval Request Comment
[Feature/Bug causing the regression]:

UAF in XSLT.

[User impact if declined]:

Possible UAF when accessing an XSLT document.

[Is this code covered by automated tests?]:

There are tests for XSLT, we have a crashtest but it is not landed.

[Has the fix been verified in Nightly?]:

Landed in nightly, locally verified with attached crashtest.

[Needs manual test from QE? If yes, steps to reproduce]: 

Yes. Apply and run attached crashtest. Verify an XSLT error is produced instead.

[List of other uplifts needed for the feature/fix]:

None that I know of.

[Is the change risky?]:

Lower risk.

[Why is the change risky/not risky?]:

Cleans up a stack after failure, existing XSLT tests are happy.

[String changes made/needed]:

None.

Comment 37

[Eric Rahm [:erahm]]

Comment on attachment 8815911 [details] [diff] [review]
Pop eval context on early returns

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:

Possible UAF with specially crafted XSLT.

User impact if declined: 

Possible UAF in XSLT documents.

Fix Landed on Version:

53, nominated for 52, 51, 50.

Risk to taking this patch (and alternatives if risky): 

See nomination for 52, 51, 50.

String or UUID changes made by this patch: 

None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 38

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

Comment on attachment 8815911 [details] [diff] [review]
Pop eval context on early returns

Let's take this for aurora, beta, and esr45. This should make it into 51 beta 7 next week, and we want it in ESR 45.6.0 for tomorrow's build.

Comment 39

[Eric Rahm [:erahm]]

remote:   https://hg.mozilla.org/releases/mozilla-esr45/rev/6bc7cc7a33a6880bebbe7ff6a41698aa1e940e30
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/9de9a34b6793b36218032a215178cc7ddbd66c98
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/4d147e29f30c9da246d9ecbeb8b778fbd26fd749

Comment 40

[Carsten Book [:Tomcat]]

setting flags

Comment 41

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

Comment on attachment 8815911 [details] [diff] [review]
Pop eval context on early returns

51 is now on the mozilla-release branch and this is already fixed in 51.

Comment 42

[Eric Rahm [:erahm]]

Attachment: DO NOT CHECK IN UNTIL PUBLIC: Crashtest

Fixing xsl import in crashtest.

Comment 43

[Mihai Boldan, Desktop QA [:mboldan]]

Attachment: Test Results.txt

I managed to test the fix on Latest Nightly, Latest Aurora, Firefox 52.0b2, Firefox 51.0.2.
I was not able to test the fix on Firefox ESR 45, due some errors while building Firefox related to gstream (See the error in the attachment).
Eric, can you please confirm that this is the expected result for this test (see the results in the attachment). 

Note that the tests were performed under Ubuntu 16.04x64.

Comment 44

[Eric Rahm [:erahm]]

(In reply to Mihai Boldan, QA [:mboldan] from comment #43)
> Created attachment 8831067 [details]
> Test Results.txt
> 
> I managed to test the fix on Latest Nightly, Latest Aurora, Firefox 52.0b2,
> Firefox 51.0.2.
> I was not able to test the fix on Firefox ESR 45, due some errors while
> building Firefox related to gstream (See the error in the attachment).
> Eric, can you please confirm that this is the expected result for this test
> (see the results in the attachment). 
> 
> Note that the tests were performed under Ubuntu 16.04x64.

The '-Mozilla-Release-' output appears to be cut off. The rest looks as expected.

Comment 45

[Mihai Boldan, Desktop QA [:mboldan]]

After more investigation, it seems that the result for Mozilla-Release was not pasted entirely. 
According to Comment 44, I'm marking this issue Verified Fixed.
The tests were performed on Ubuntu 16.04x64.

Note that the fix was not tested on ESR45 build, due to some errors with gstream while building Mozilla ESR 45.
If someone knows how to bypass this error, please let me know in order to test this fix on ESR build as well.