894137 - (CVE-2013-1724) Heap-use-after-free in mozilla::dom::HTMLFormElement::IsDefaultSubmitElement

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Scott Bell]

Attachment: Testcase

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.

Comment 1

[Scott Bell]

Attachment: ASAN log

Comment 2

[Boris Zbarsky [:bzbarsky]]

Hmm.  Something about the <select> inside the keygen?

Comment 3

[Scott Bell]

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

I wonder whether the form is dead but the select still has a pointer to it or something...

Comment 5

[Daniel Veditz [:dveditz]]

Assuming the worst (sec-critical) if it's a UAF of an object with virtual methods.

Comment 6

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

Boris, any suggestions on who we can assign this issue to unless you want to take it?

Comment 7

[Boris Zbarsky [:bzbarsky]]

I won't have time to work on it in the next week or so, most likely.  I'd try Andrea first, I guess.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Or mounir

Comment 9

[Mounir Lamouri (:mounir)]

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...

Comment 10

[Mats Palmgren (inactive)]

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).

Comment 11

[Alex Keybl [:akeybl]]

(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.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

This needs an owner. Mounir, assign to someone else if you don't have time to look at this.

Comment 13

[Mounir Lamouri (: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

Comment 14

[Mounir Lamouri (:mounir)]

There is no obvious suspect here so I will do an hg bisect locally to find out which commit might have created the regression.

Comment 15

[Scott Bell]

Could it be to do with the implementation of dir=auto?

http://hg.mozilla.org/mozilla-central/rev/7f2e295e4981

Comment 16

[Mounir Lamouri (:mounir)]

(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'.

Comment 17

[(no longer active)]

Simon can you please take a look?  Thanks!

Comment 18

[Mats Palmgren (inactive)]

Note that the underlying problem might predate dir=auto though.  See comment 10.

Comment 19

[Simon Montagu :smontagu]

Attachment: Patch

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

Comment 20

[Simon Montagu :smontagu]

Attachment: Test for checkin after bug is open

Comment 21

[Simon Montagu :smontagu]

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)

Comment 22

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

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.

Comment 23

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

Release Management, is this fine on Beta for you since it is early in the cycle?

Comment 24

[Carsten Book [:Tomcat]]

checkin https://hg.mozilla.org/mozilla-central/rev/acacd90dd161

Comment 25

[Alex Keybl [:akeybl]]

Still very early in the cycle, let's definitely land on Aurora/Beta.

Comment 26

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

Ok. Simon, please create Aurora and Beta patches.

Comment 27

[Simon Montagu :smontagu]

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 28

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

Comment on attachment 786785 [details] [diff] [review]
Patch

Cleared to land per Alex.

Comment 29

[Simon Montagu :smontagu]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2f898f833ad0
https://hg.mozilla.org/releases/mozilla-beta/rev/9d0a0b056e86

Comment 30

[Simon Montagu :smontagu]

Filed bug 905036 on the remaining assertion: transferring relevant tracking flags to there.

Comment 31

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

I'm going to undo marking this fixed on 23, which we shipped.

Comment 33

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed crash on ASan FF24, 2013-06-06.
Verified fixed on ASan FF26, 2013-11-20.

Comment 35

[Simon Montagu :smontagu]

Checked in testcase: https://hg.mozilla.org/integration/mozilla-inbound/rev/bd04d03f7980

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 37

[Simon Montagu :smontagu]

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.

Comment 38

[Cameron McCormack (:heycam)]

This also needs to go now in dom/base/crashtests/ rather than content/base/crashtests/.

Comment 39

[Simon Montagu :smontagu]

(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

Comment 40

[Simon Montagu :smontagu]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d26673206131

Comment 41

[Carsten Book [:Tomcat]]

(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

Comment 42

[Simon Montagu :smontagu]

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.

Comment 43

[Simon Montagu :smontagu]

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.

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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