Closed Bug 894137 (CVE-2013-1724) Opened 6 years ago Closed 5 years ago
Heap-use-after-free in mozilla::dom::HTMLForm
Element::Is Default Submit Element
1.34 KB, text/html
5.20 KB, text/plain
5.39 KB, patch
|Details | Diff | Splinter Review|
2.11 KB, patch
|Details | Diff | Splinter Review|
ASAN reports a use-after-free in mozilla::dom::HTMLFormElement::IsDefaultSubmitElement when the attached repro is opened. It seems to trigger when a gc() happens, so it might take a few seconds to crash.
Component: Untriaged → Layout: Form Controls
Product: Firefox → Core
Hmm. Something about the <select> inside the keygen?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Here's a better stack from a debug build. ================================================================= ==32552== ERROR: AddressSanitizer: heap-use-after-free on address 0x7fb49d449db8 at pc 0x7fb4b201dd77 bp 0x7fff86323b80 sp 0x7fff86323b78 READ of size 8 at 0x7fb49d449db8 thread T0 #0 0x7fb4b201dd76 in mozilla::dom::HTMLFormElement::IsDefaultSubmitElement(nsIFormControl const*) const /builds/slave/m-cen-l64-dbg-asan-00000000000/build/content/html/content/src/HTMLFormElement.cpp:1766 #1 0x7fb4b200be20 in nsGenericHTMLFormElement::IntrinsicState() const /builds/slave/m-cen-l64-dbg-asan-00000000000/build/content/html/content/src/nsGenericHTMLElement.cpp:2516 #2 0x7fb4b1f71a06 in mozilla::dom::HTMLSelectElement::IntrinsicState() const /builds/slave/m-cen-l64-dbg-asan-00000000000/build/content/html/content/src/HTMLSelectElement.cpp:1551 #3 0x7fb4b1ad177b in mozilla::dom::Element::UpdateState(bool) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/content/base/src/Element.cpp:167 #4 0x7fb4b1acb7b7 in mozilla::dom::Element::SetDirectionality(mozilla::Directionality, bool) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/../../../../dist/include/mozilla/dom/Element.h:326 #5 0x7fb4b1acbb19 in mozilla::WalkDescendantsSetDirectionFromText(mozilla::dom::Element*, bool, nsINode*) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/content/base/src/DirectionalityUtils.cpp:418 #6 0x7fb4b1acd259 in mozilla::nsTextNodeDirectionalityMap::ResetNodeDirection(nsPtrHashKey<mozilla::dom::Element>*, void*) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/content/base/src/DirectionalityUtils.cpp:505 #7 0x7fb4b1accc62 in nsCheapSet<nsPtrHashKey<mozilla::dom::Element> >::EnumerateEntries(PLDHashOperator (*)(nsPtrHashKey<mozilla::dom::Element>*, void*), void*) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/../../../dist/include/nsCheapSets.h:68 #8 0x7fb4b1cf3cc7 in nsTextNode::UnbindFromTree(bool, bool) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/content/base/src/nsTextNode.cpp:150 #9 0x7fb4b1b04112 in ContentUnbinder::UnbindSubtree(nsIContent*) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/content/base/src/FragmentOrElement.cpp:1019 #10 0x7fb4b1b040c8 in ContentUnbinder::UnbindSubtree(nsIContent*) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/content/base/src/FragmentOrElement.cpp:1018 #11 0x7fb4b1b03c7d in ContentUnbinder::Run() /builds/slave/m-cen-l64-dbg-asan-00000000000/build/content/base/src/FragmentOrElement.cpp:1031 #12 0x7fb4b0cc7402 in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/xpcom/threads/nsThread.cpp:621 #13 0x7fb4b0bf6d81 in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/obj-firefox/xpcom/build/nsThreadUtils.cpp:238 #14 0x7fb4affc8cbb in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/ipc/glue/MessagePump.cpp:81 #15 0x7fb4b0d8a9e1 in MessageLoop::RunInternal() /builds/slave/m-cen-l64-dbg-asan-00000000000/build/ipc/chromium/src/base/message_loop.cc:219 #16 0x7fb4b0d8a8de in MessageLoop::Run() /builds/slave/m-cen-l64-dbg-asan-00000000000/build/ipc/chromium/src/base/message_loop.cc:186 #17 0x7fb4b33d9e91 in nsBaseAppShell::Run() /builds/slave/m-cen-l64-dbg-asan-00000000000/build/widget/xpwidgets/nsBaseAppShell.cpp:163 #18 0x7fb4b2f53bbf in nsAppStartup::Run() /builds/slave/m-cen-l64-dbg-asan-00000000000/build/toolkit/components/startup/nsAppStartup.cpp:269 #19 0x7fb4afcbd039 in XREMain::XRE_mainRun() /builds/slave/m-cen-l64-dbg-asan-00000000000/build/toolkit/xre/nsAppRunner.cpp:3853 #20 0x7fb4afcbe377 in XREMain::XRE_main(int, char**, nsXREAppData const*) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/toolkit/xre/nsAppRunner.cpp:3921 #21 0x7fb4afcbed01 in XRE_main /builds/slave/m-cen-l64-dbg-asan-00000000000/build/toolkit/xre/nsAppRunner.cpp:4123 #22 0x40c796 in do_main(int, char**, nsIFile*) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/browser/app/nsBrowserApp.cpp:272 #23 0x40bcbf in main /builds/slave/m-cen-l64-dbg-asan-00000000000/build/browser/app/nsBrowserApp.cpp:632 #24 0x7fb4bd4c576c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226 0x7fb49d449db8 is located 376 bytes inside of 512-byte region [0x7fb49d449c40,0x7fb49d449e40) freed by thread T0 here: #0 0x43c5a0 in free ??:0 #1 0x7fb4b0cf3d8a in ~SnowWhiteKiller /builds/slave/m-cen-l64-dbg-asan-00000000000/build/xpcom/base/nsCycleCollector.cpp:2173 #2 0x7fb4b0ce1ace in nsCycleCollector::FreeSnowWhite(bool) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/xpcom/base/nsCycleCollector.cpp:2280 #3 0x7fb4b0cf25e8 in AsyncFreeSnowWhite::Run() /builds/slave/m-cen-l64-dbg-asan-00000000000/build/xpcom/base/nsCycleCollector.cpp:2260 #4 0x7fb4b0cc7402 in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/xpcom/threads/nsThread.cpp:621 #5 0x7fb4b0bf6d81 in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/obj-firefox/xpcom/build/nsThreadUtils.cpp:238 #6 0x7fb4affc8cbb in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/ipc/glue/MessagePump.cpp:81 #7 0x7fb4b0d8a9e1 in MessageLoop::RunInternal() /builds/slave/m-cen-l64-dbg-asan-00000000000/build/ipc/chromium/src/base/message_loop.cc:219 #8 0x7fb4b0d8a8de in MessageLoop::Run() /builds/slave/m-cen-l64-dbg-asan-00000000000/build/ipc/chromium/src/base/message_loop.cc:186 #9 0x7fb4b33d9e91 in nsBaseAppShell::Run() /builds/slave/m-cen-l64-dbg-asan-00000000000/build/widget/xpwidgets/nsBaseAppShell.cpp:163 #10 0x7fb4b2f53bbf in nsAppStartup::Run() /builds/slave/m-cen-l64-dbg-asan-00000000000/build/toolkit/components/startup/nsAppStartup.cpp:269 #11 0x7fb4afcbd039 in XREMain::XRE_mainRun() /builds/slave/m-cen-l64-dbg-asan-00000000000/build/toolkit/xre/nsAppRunner.cpp:3853 #12 0x7fb4afcbe377 in XREMain::XRE_main(int, char**, nsXREAppData const*) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/toolkit/xre/nsAppRunner.cpp:3921 #13 0x7fb4afcbed01 in XRE_main /builds/slave/m-cen-l64-dbg-asan-00000000000/build/toolkit/xre/nsAppRunner.cpp:4123 previously allocated by thread T0 here: #0 0x43c660 in __interceptor_malloc ??:0 #1 0x7fb4bb7db547 in moz_xmalloc /builds/slave/m-cen-l64-dbg-asan-00000000000/build/memory/mozalloc/mozalloc.cpp:54 #2 0x7fb4b2015373 in operator new(unsigned long) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/../../../../dist/include/mozilla/mozalloc.h:201 #3 0x7fb4b2048d96 in CreateHTMLElement(unsigned int, already_AddRefed<nsINodeInfo>, mozilla::dom::FromParser) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/content/html/document/src/nsHTMLContentSink.cpp:498 #4 0x7fb4b2048f89 in NS_NewHTMLElement(nsIContent**, already_AddRefed<nsINodeInfo>, mozilla::dom::FromParser) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/content/html/document/src/nsHTMLContentSink.cpp:481 #5 0x7fb4b1c9cbc6 in NS_NewElement(nsIContent**, already_AddRefed<nsINodeInfo>, mozilla::dom::FromParser) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/content/base/src/nsNameSpaceManager.cpp:194 #6 0x7fb4b27ad162 in nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor*, nsIContent**) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/parser/html/nsHtml5TreeOperation.cpp:350 #7 0x7fb4b27a5ba9 in nsHtml5TreeOpExecutor::RunFlushLoop() /builds/slave/m-cen-l64-dbg-asan-00000000000/build/parser/html/nsHtml5TreeOpExecutor.cpp:557 #8 0x7fb4b2774ea4 in nsHtml5ExecutorFlusher::Run() /builds/slave/m-cen-l64-dbg-asan-00000000000/build/parser/html/nsHtml5StreamParser.cpp:129 Shadow byte and word: 0x1ff693a893b7: fd 0x1ff693a893b0: fd fd fd fd fd fd fd fd More shadow bytes: 0x1ff693a89390: fd fd fd fd fd fd fd fd 0x1ff693a89398: fd fd fd fd fd fd fd fd 0x1ff693a893a0: fd fd fd fd fd fd fd fd 0x1ff693a893a8: fd fd fd fd fd fd fd fd =>0x1ff693a893b0: fd fd fd fd fd fd fd fd 0x1ff693a893b8: fd fd fd fd fd fd fd fd 0x1ff693a893c0: fd fd fd fd fd fd fd fd 0x1ff693a893c8: fa fa fa fa fa fa fa fa 0x1ff693a893d0: fa fa fa fa fa fa fa fa Stats: 397M malloced (335M for red zones) by 757537 calls Stats: 46M realloced by 48371 calls Stats: 355M freed by 528039 calls Stats: 307M really freed by 466693 calls Stats: 293M (75202 full pages) mmaped in 501 calls mmaps by size class: 7:298935; 8:77786; 9:19437; 10:9709; 11:7650; 12:2304; 13:1472; 14:800; 15:208; 16:408; 17:456; 18:26; 19:39; 20:21; 21:1; 23:4; mallocs by size class: 7:524949; 8:118069; 9:45014; 10:32167; 11:22598; 12:4797; 13:3613; 14:2702; 15:509; 16:1162; 17:1808; 18:54; 19:56; 20:29; 21:2; 23:8; frees by size class: 7:364271; 8:68317; 9:34973; 10:28248; 11:20073; 12:3639; 13:2812; 14:2429; 15:373; 16:993; 17:1780; 18:48; 19:46; 20:28; 21:1; 23:8; rfrees by size class: 7:320737; 8:60534; 9:31514; 10:25042; 11:17623; 12:3211; 13:2626; 14:2297; 15:330; 16:909; 17:1757; 18:40; 19:44; 20:24; 21:1; 23:4; Stats: malloc large: 3629 small slow: 9049 ==32552== ABORTING
I wonder whether the form is dead but the select still has a pointer to it or something...
Assuming the worst (sec-critical) if it's a UAF of an object with virtual methods.
Boris, any suggestions on who we can assign this issue to unless you want to take it?
I won't have time to work on it in the next week or so, most likely. I'd try Andrea first, I guess.
I had a quick look at this. This is indeed mForm being non-null and pointing to some garbage. Also, the crash is reproducible on Firefox 22 so it is unlikely a regression from HTMLFormElement WebIDL conversion. I tried to get a regression range but builds from mozregression are always crashing at startup...
esr17/b2g18 doesn't crash for me (Linux64 debug), probably due to not having dir="auto" that the attached test uses. I do see this though: ###!!! ASSERTION: mForm should be null at this point!: '!mForm', file content/html/content/src/nsGenericHTMLElement.cpp, line 3170 so it seems the underlying issue is probably there too and a different test could trigger the crash (use-after-free via mForm).
Severity: normal → critical
Component: Layout: Form Controls → DOM: Core & HTML
(In reply to Mats Palmgren (:mats) from comment #10) > esr17/b2g18 doesn't crash for me (Linux64 debug), probably due to not having > dir="auto" that the attached test uses. I do see this though: > > ###!!! ASSERTION: mForm should be null at this point!: '!mForm', file > content/html/content/src/nsGenericHTMLElement.cpp, line 3170 > > so it seems the underlying issue is probably there too and a different test > could trigger the crash (use-after-free via mForm). For now, let's call this affected in that case. We can revisit if a final solution is too difficult to backport.
This needs an owner. Mounir, assign to someone else if you don't have time to look at this.
Assignee: nobody → mounir
I did a regression check on my Mac Mini and as expected, this is an old bug: Last good nightly: 2012-11-21 First bad nightly: 2012-11-22 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4f19e7fd8bea&tochange=20ec9014f220
There is no obvious suspect here so I will do an hg bisect locally to find out which commit might have created the regression.
Could it be to do with the implementation of dir=auto? http://hg.mozilla.org/mozilla-central/rev/7f2e295e4981
(In reply to Scott Bell from comment #15) > Could it be to do with the implementation of dir=auto? Good catch. I forgot that this test was using dir='auto'.
Simon can you please take a look? Thanks!
Note that the underlying problem might predate dir=auto though. See comment 10.
QA Contact: smontagu
In spite of the underlying problem, which this patch doesn't address, I think this is a definite bug with dir=auto, since when tearing down the DOM we don't need to reset the direction of ancestors of text nodes as we unbind them. So this prevents the uaf and crash, but brings back the assertion from comment 10. Try run: https://tbpl.mozilla.org/?tree=Try&rev=82f71cf4f33e
Assignee: mounir → smontagu
Attachment #786785 - Flags: review?(ehsan)
Comment on attachment 786785 [details] [diff] [review] Patch [Security approval request comment] How easily could an exploit be constructed based on the patch? I don't think anything in the patch points towards an exploit. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No Which older supported branches are affected by this flaw? 22, 23, 24, 25 (This patch doesn't fix the part that caused esr17 to be marked as affected) If not all supported branches, which bug introduced the flaw? Bug 548206 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Not yet, but there should be only minor differences in the backports How likely is this patch to cause regressions; how much testing does it need? It's not risk-free, and could do with testing. I've tested it with the not-yet-checked-in testcases from other uaf bugs in this area which I have in my tree, but not with every testcase attached to every such bug (and with these testcases sometimes manual testing fails where automatic testing doesn't)
Attachment #786785 - Flags: sec-approval?
Comment on attachment 786785 [details] [diff] [review] Patch sec-approval+ for trunk. I'll mark it tracking+ for Aurora so you can make an Aurora patch after it goes into trunk. We should get release management opinions on taking this on Beta and doing an ESR version. For Beta, I'd like to see some focused testing somewhere.
Attachment #786785 - Flags: sec-approval? → sec-approval+
Release Management, is this fine on Beta for you since it is early in the cycle?
Still very early in the cycle, let's definitely land on Aurora/Beta.
Ok. Simon, please create Aurora and Beta patches.
Comment on attachment 786785 [details] [diff] [review] Patch It turns out the patch as checked in to trunk applies cleanly on Aurora and Beta [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 548206 User impact if declined: Exposure to sec-crit vulnerability Testing completed (on m-c, etc.): Baked on m-c since 2013-08-08 Risk to taking this patch (and alternatives if risky): Minor risk of regressions in dir=auto String or IDL/UUID changes made by this patch: None
Comment on attachment 786785 [details] [diff] [review] Patch Cleared to land per Alex.
Filed bug 905036 on the remaining assertion: transferring relevant tracking flags to there.
I'm going to undo marking this fixed on 23, which we shipped.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Confirmed crash on ASan FF24, 2013-06-06. Verified fixed on ASan FF26, 2013-11-20.
Checked in testcase: https://hg.mozilla.org/integration/mozilla-inbound/rev/bd04d03f7980
Flags: in-testsuite? → in-testsuite+
aiiie, I only just noticed that the checkin didn't include adding the testcase to crashtests.list. ni-ing myself so I don't forget.
This also needs to go now in dom/base/crashtests/ rather than content/base/crashtests/.
(In reply to Cameron McCormack (:heycam) from comment #38) > This also needs to go now in dom/base/crashtests/ rather than > content/base/crashtests/. Right, it was insufficiently addressed merge conflicts after the content ==> dom restructuring that caused the problem with crashtests.list
(In reply to Simon Montagu :smontagu from comment #40) > https://hg.mozilla.org/integration/mozilla-inbound/rev/d26673206131 sorry had to back this out for test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4151782&repo=mozilla-inbound
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Yes, sorry, the test needs an asserts() annotation because of the remaining assertion (filed as bug 905036) as noted in comment 19. I'll recheck in with that after a try push. Meanwhile reresolving because the last 2 checkins only added in-testsuite+ and the bug was fixed before that.
Status: REOPENED → RESOLVED
Closed: 6 years ago → 5 years ago
Resolution: --- → FIXED
Unfortunately the annotation doesn't help because this test makes the assetion fire in other tests. Removed the testcase in https://hg.mozilla.org/integration/mozilla-inbound/rev/ed10a7953a89 pending bug 905036.
Depends on: 905036
Flags: in-testsuite+ → in-testsuite?
Attachment #776057 - Attachment mime type: text/plain → text/html
Whiteboard: [adv-main24+] → [adv-main24+][b2g-adv-main2.2-]]
Whiteboard: [adv-main24+][b2g-adv-main2.2-]] → [adv-main24+][b2g-adv-main2.2-]
Whiteboard: [adv-main24+][b2g-adv-main2.2-] → [adv-main24+]
You need to log in before you can comment on or make changes to this bug.