Closed Bug 1311687 (CVE-2017-5376) Opened 8 years ago Closed 8 years ago

ASan heap UAF via xsl:key and xsl:variable

Categories

(Core :: XSLT, defect)

Product:
Core
Component:
XSLT
Version:
52 Branch
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox-esr45 51+ fixed
firefox50 + wontfix
firefox51 + verified
firefox52 + verified
firefox53 + verified

People

(Reporter: nicolas.gregoire, Assigned: erahm)

References

(
URL
)

Details

(Keywords: csectype-uaf, reporter-external, sec-critical, Whiteboard: [post-critsmash-triage][adv-main51+][adv-esr45.7+])

Attachments

(3 files, 4 obsolete files)

Pop eval context on early returns
4.29 KB, patch
peterv
: review+
lizzard
: approval-mozilla-aurora+
lizzard
: approval-mozilla-beta+
lizzard
: approval-mozilla-release-
lizzard
: approval-mozilla-esr45+
Details | Diff | Splinter Review
DO NOT CHECK IN UNTIL PUBLIC: Crashtest
1.33 KB, patch
Details | Diff | Splinter Review
Test Results.txt
8.95 KB, text/plain
Details
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()
Peter could you look at this please?
Flags: needinfo?(peterv)
Keywords: csectype-uaf, sec-critical
It would also be good to confirm that this affects the browser and not just XPCShell.
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
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
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
Attachment #8803191 - Flags: review?(peterv)
Assignee: nobody → erahm
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(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.
Attachment #8803191 - Flags: review?(peterv) → review-
(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
Make sure the eval context stack is cleaned up on failure. MozReview-Commit-ID: AUNen1xt9He
Attachment #8804039 - Flags: review?(peterv)
Attachment #8803191 - Attachment is obsolete: true
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.
Group: core-security → dom-core-security
Are other branches affected, or just 52?
status-firefox51: --- → ?
status-firefox52: --- → affected
[Tracking Requested - why for this release]:
status-firefox50: --- → wontfix
status-firefox51: ? → affected
status-firefox-esr45: --- → affected
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
Tracking for 51+ since this is sec-critical. We could still aim to fix this in 51.
tracking-firefox51: ? → +
tracking-firefox52: ? → +
tracking-firefox-esr45: --- → ?
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #10) > Are other branches affected, or just 52? This almost certainly affects all branches.
Peter, does the latest iteration look good?
Flags: needinfo?(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.
Attachment #8804039 - Flags: review?(peterv) → review+
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.
Flags: needinfo?(peterv)
Attachment #8804039 - Flags: sec-approval?
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.
tracking-firefox-esr45: ? → 51+
Whiteboard: [checkin on 11/29]
Attachment #8804039 - Flags: sec-approval? → sec-approval+
(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?
Flags: needinfo?(abillings)
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.
Flags: needinfo?(abillings)
Updated patch with test removed
Attachment #8804039 - Attachment is obsolete: true
Split out crashtest.
Comment on attachment 8808264 [details] [diff] [review] Pop eval context on early returns (carrying forward r+)
Attachment #8808264 - Flags: review+
Comment on attachment 8808265 [details] [diff] [review] DO NOT CHECK IN UNTIL PUBLIC: Crashtest (carrying forward r+)
Attachment #8808265 - Flags: review+
[Tracking Requested - why for this release]: sec-critical
status-firefox53: --- → affected
tracking-firefox53: --- → ?
Tracking 53+ for this sec-critical issue.
tracking-firefox53: ? → +
Getting this back on the radar for 50.1 as well. https://hg.mozilla.org/integration/mozilla-inbound/rev/667c1a30679730fbf513e310b2768bc8eea5cc51
status-firefox50: wontfix → affected
tracking-firefox50: --- → ?
tracking-firefox-esr45: 51+ → ?
Flags: in-testsuite?
Whiteboard: [checkin on 11/29]
Starting a local asan build to see if I can repro.
Flags: needinfo?(erahm)
Okay I can repro, it would seem for this test we have another double-delete on |delete mInitialEvalContext|.
Please nominate this for Aurora/Beta/Release/ESR45 approval ASAP so we can hopefully get it uplifted in time for 51b6.
Flags: needinfo?(erahm)
Sorry, I obviously hit submit on that before noticing the last comments here :)
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
Attachment #8815911 - Flags: review?(peterv)
Attachment #8808264 - Attachment is obsolete: true
Flags: needinfo?(erahm)
Attachment #8815911 - Flags: review?(peterv) → review+
https://hg.mozilla.org/mozilla-central/rev/ccc5ede0fd33
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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.
Attachment #8815911 - Flags: approval-mozilla-release?
Attachment #8815911 - Flags: approval-mozilla-beta?
Attachment #8815911 - Flags: approval-mozilla-aurora?
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.
Attachment #8815911 - Flags: approval-mozilla-esr45?
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.
Attachment #8815911 - Flags: approval-mozilla-esr45?
Attachment #8815911 - Flags: approval-mozilla-esr45+
Attachment #8815911 - Flags: approval-mozilla-beta?
Attachment #8815911 - Flags: approval-mozilla-beta+
Attachment #8815911 - Flags: approval-mozilla-aurora?
Attachment #8815911 - Flags: approval-mozilla-aurora+
Group: dom-core-security → core-security-release
Flags: sec-bounty?
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]
Flags: sec-bounty? → sec-bounty+
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.
Attachment #8815911 - Flags: approval-mozilla-release? → approval-mozilla-release-
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main51+][adv-esr45.7+]
Alias: CVE-2017-5376
Fixing xsl import in crashtest.
Attachment #8808265 - Attachment is obsolete: true
Attached file 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.
Flags: needinfo?(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.
Flags: needinfo?(erahm)
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.
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
status-firefox52: fixed → verified
status-firefox53: fixed → verified
Flags: qe-verify+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: