Bug (CVE-2016-4759) – Crash in TreeScope::focusedElement

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Ryosuke Niwa]

We can get into a state where a focused element's treeScope doesn't match that of Document yet the tree scope isn't of a shadow tree
because we can get into a state where document->m_focusedElement->document() != document.

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x00007fff96399aff WebCore::TreeScope::focusedElement() + 207
1   com.apple.WebCore             	0x00007fff9629b37f WebCore::Document::removeFocusedNodeOfSubtree(WebCore::Node*, bool) + 47
2   com.apple.WebCore             	0x00007fff966edc8e WebCore::Element::removeShadowRoot() + 78
3   com.apple.WebCore             	0x00007fff96234239 WebCore::Element::~Element() + 25
4   com.apple.WebCore             	0x00007fff962e700e WebCore::HTMLInputElement::~HTMLInputElement() + 14
5   com.apple.JavaScriptCore      	0x00007fff8dfb8ff6 JSC::MarkedBlock::FreeList JSC::MarkedBlock::sweepHelper<true>(JSC::MarkedBlock::SweepMode) + 326
6   com.apple.JavaScriptCore      	0x00007fff8da7f51b JSC::MarkedBlock::sweep(JSC::MarkedBlock::SweepMode) + 75
7   com.apple.JavaScriptCore      	0x00007fff8de68500 JSC::IncrementalSweeper::sweepNextBlock() + 96
8   com.apple.JavaScriptCore      	0x00007fff8da87bb8 JSC::IncrementalSweeper::doSweep(double) + 24
9   com.apple.JavaScriptCore      	0x00007fff8da87aaa JSC::HeapTimer::timerDidFire(__CFRunLoopTimer*, void*) + 186
10  com.apple.CoreFoundation      	0x00007fff8a1c3b94 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 20
11  com.apple.CoreFoundation      	0x00007fff8a1c3823 __CFRunLoopDoTimer + 1075
12  com.apple.CoreFoundation      	0x00007fff8a1c337a __CFRunLoopDoTimers + 298
13  com.apple.CoreFoundation      	0x00007fff8a1ba871 __CFRunLoopRun + 1841
14  com.apple.CoreFoundation      	0x00007fff8a1b9ed8 CFRunLoopRunSpecific + 296
15  com.apple.HIToolbox           	0x00007fff95ece935 RunCurrentEventLoopInMode + 235
16  com.apple.HIToolbox           	0x00007fff95ece76f ReceiveNextEventCommon + 432
17  com.apple.HIToolbox           	0x00007fff95ece5af _BlockUntilNextEventMatchingListInModeWithFilter + 71
18  com.apple.AppKit              	0x00007fff9438bdf6 _DPSNextEvent + 1067
19  com.apple.AppKit              	0x00007fff9438b226 -[NSApplication _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 454
20  com.apple.AppKit              	0x00007fff9437fd80 -[NSApplication run] + 682
21  com.apple.AppKit              	0x00007fff94349368 NSApplicationMain + 1176
22  libxpc.dylib                  	0x00007fff9a786194 _xpc_objc_main + 795
23  libxpc.dylib                  	0x00007fff9a784bbe xpc_main + 494
24  com.apple.WebKit.WebContent   	0x000000010cf70b4a 0x10cf70000 + 2890
25  libdyld.dylib                 	0x00007fff8b0c35ad start + 1

[Ryosuke Niwa]

<rdar://problem/24337904>

[Ryosuke Niwa]

Oh snap, that inconsistency is a lot tricker than I had initially thought.  There is a fundamental API design issue at a hand.

When an element is removed, we fire blur event if the element had been focused. We also fire unload event if it's an iframe. But we can focus the iframe back while unload event is being dispatched, or add the iframe back while focus event is being dispatched.

In a way, we can end up in a mutually recursive behavior between unloading an iframe and focusing the same iframe inside blur/unload event handlers.

The current behavior was introduced by http://trac.webkit.org/changeset/127534 four years ago, and it prefers keeping iframe in a consistent state.

For now, I'm going to preserve the status quo and let this inconsistency manifest into m_focusedElement.

[Ryosuke Niwa]

Huh, that doesn't work either because of assertions in UserActionElementSet.

[Ryosuke Niwa]

Created attachment 279941 [details]
Stops dispatching blur and change events

[Ryosuke Niwa]

I ran all layout tests in both debug WTR and DRT on El Capitan.

[Enrica Casucci]

Comment on attachment 279941 [details]
Stops dispatching blur and change events

Look ok to me.

[Enrica Casucci]

Just a bit worried about compatibility but I think it is worth trying.

[Ryosuke Niwa]

Committed r201471: <http://trac.webkit.org/changeset/201471>