New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 0 users
Status: Fixed
Owner:
Closed: Dec 2012
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security

Restricted
  • Only users with EditIssue permission may comment.



Sign in to add a comment
Heap-use-after-free in WebKit::WebElement::document
Project Member Reported by infe...@chromium.org, Jul 18 2012 Back to list
Detailed report: https://cluster-fuzz.appspot.com/testcase?key=79094550

Fuzzer: Fermin_swf_bitflip

Crash Type: Heap-use-after-free READ 8
Crash Address: 0x7fc3246d58a0
Crash State:
  - crash stack -
  WebKit::WebElement::document
  webkit::ppapi::PluginInstance::GetWindowObject
  - free stack -
  WebCore::ContainerNode::removeAllChildren
  WebCore::Document::removedLastRef
  

Minimized Testcase (238.71 Kb): https://cluster-fuzz.appspot.com/download/AMIfv96T1I3bxlSfLZjN2pKErZ6KoXX328sGwOMV1xy8ZqZBm58tdgQPDtSuozWJ30RBcnRFpARmmHUabG_uvueaPU8Nprpn4relG8Ju6OkpvQwdSrtJK4VtUaKks4Xz-xfgtiAMqTyAbuTWz8Te9JT6YdWuxlV0lcuVb4XyoyYvyNCuDFkDjR4
 
Cc: fjserna@google.com
Owner: viettrungluu@chromium.org
Status: Assigned
This does not look flash code, but our ppapi handling code.
Cc: brettw@chromium.org viettrungluu@chromium.org
Owner: ----
Comment 3 by jsc...@chromium.org, Jul 22 2012
Labels: Feature-Flash Feature-Plugins-Pepper SecImpacts-Beta
Comment 4 by tsepez@chromium.org, Jul 23 2012
Owner: tsepez@chromium.org
looking.
Comment 5 by tsepez@chromium.org, Jul 26 2012
Looks like the m_pluginWidget member of FrameLoaderClientImpl is keeping the widget alive past the destruction of the document.  This keeps the instance registered so that a delayed GetWindowObject is still processed, rather than just erroring upon enter.  The plugininstance has a raw ptr to a webplugincontentsimpl which has a raw ptr to an HTMLPluginElement from the document which no longer exists.

Now, the m_pluginWidget is just about to be cleared by FrameLoaderClientImpl::redirectDataToPlugin (called from PluginDocumentParser::appendBytes), but before that can happen, layout causes the plugin to be created, which processes a sync message, which can catch the delayed GetWindowObject in its nested message loop.


Comment 6 by tsepez@chromium.org, Jul 26 2012
More background:  A Debug build will likely trip over an assert showing that the m_pluginWidget outlived it page when it gets cleared (frame 28):


(gdb) where
#0  0x00007fbb68fc34a5 in WebCore::disposeUnderlyingV8Object (
    npObject=0x7fbb57192f60)
    at third_party/WebKit/Source/WebCore/bindings/v8/NPV8Object.cpp:178
#1  0x00007fbb68fc2d6d in WebCore::freeV8NPObject (npObject=0x7fbb57192f60)
    at third_party/WebKit/Source/WebCore/bindings/v8/NPV8Object.cpp:78
#2  0x00007fbb69037aa3 in _NPN_DeallocateObject (npObject=0x7fbb57192f60)
    at third_party/WebKit/Source/WebCore/bindings/v8/npruntime.cpp:310
#3  0x00007fbb69037b86 in _NPN_ReleaseObject (npObject=0x7fbb57192f60)
    at third_party/WebKit/Source/WebCore/bindings/v8/npruntime.cpp:323
#4  0x00007fbb68810616 in WebKit::WebBindings::releaseObject (
    object=0x7fbb57192f60)
    at third_party/WebKit/Source/WebKit/chromium/src/WebBindings.cpp:148
#5  0x00007fbb69e1c42b in ppapi::NPObjectVar::~NPObjectVar (
    this=0x7fbb5713d6e0, __in_chrg=<optimized out>)
    at webkit/plugins/ppapi/npobject_var.cc:31
#6  0x00007fbb676f2414 in base::RefCounted<ppapi::Var>::Release (
    this=0x7fbb5713d6e8) at ./base/memory/ref_counted.h:95
#7  0x00007fbb687f6589 in scoped_refptr<ppapi::Var>::~scoped_refptr (
    this=0x7fbb57192fa0, __in_chrg=<optimized out>)
    at ./base/memory/ref_counted.h:252
#8  0x00007fbb687f81b8 in ppapi::VarTracker::VarInfo::~VarInfo (
    this=0x7fbb57192fa0, __in_chrg=<optimized out>)
    at ./ppapi/shared_impl/var_tracker.h:92
#9  0x00007fbb687f8236 in std::pair<int const, ppapi::VarTracker::VarInfo>::~pair (this=0x7fbb57192f98, __in_chrg=<optimized out>)
    at /usr/include/c++/4.4/bits/stl_pair.h:68
#10 0x00007fbb687fa0b2 in __gnu_cxx::new_allocator<std::pair<int const, ppapi::VarTracker::VarInfo> >::destroy (this=0x7fffc027eccf, __p=0x7fbb57192f98)
    at /usr/include/c++/4.4/ext/new_allocator.h:115
#11 0x00007fbb687f9715 in __gnu_cxx::hashtable<std::pair<int const, ppapi::VarTracker::VarInfo>, int, __gnu_cxx::hash<int>, std::_Select1st<std::pair<int const, ppapi::VarTracker::VarInfo> >, std::equal_to<int>, std::allocator<ppapi::VarTracker::VarInfo> >::_M_delete_node (this=0x7fbb65648600, __n=0x7fbb57192f90)
    at /usr/include/c++/4.4/backward/hashtable.h:616
#12 0x00007fbb687f8e1f in __gnu_cxx::hashtable<std::pair<int const, ppapi::VarTracker::VarInfo>, int, __gnu_cxx::hash<int>, std::_Select1st<std::pair<int const, ppapi::VarTracker::VarInfo> >, std::equal_to<int>, std::allocator<ppapi::VarTracker::VarInfo> >::erase (this=0x7fbb65648600, __it=...)
    at /usr/include/c++/4.4/backward/hashtable.h:913
#13 0x00007fbb687f84cb in __gnu_cxx::hash_map<int, ppapi::VarTracker::VarInfo, __gnu_cxx::hash<int>, std::equal_to<int>, std::allocator<ppapi::VarTracker::VarInfo> >::erase (this=0x7fbb65648600, __it=...)
    at /usr/include/c++/4.4/ext/hash_map:237
#14 0x00007fbb687f8169 in ppapi::VarTracker::DeleteObjectInfoIfNecessary (
    this=0x7fbb656485c0, iter=...) at ppapi/shared_impl/var_tracker.cc:225
#15 0x00007fbb69e1966b in webkit::ppapi::HostVarTracker::ForceReleaseNPObject (
    this=0x7fbb656485c0, object=...)
    at webkit/plugins/ppapi/host_var_tracker.cc:145
#16 0x00007fbb69e19328 in webkit::ppapi::HostVarTracker::DidDeleteInstance (
    this=0x7fbb656485c0, instance=1673976585)
    at webkit/plugins/ppapi/host_var_tracker.cc:121
#17 0x00007fbb69e14d09 in webkit::ppapi::HostGlobals::InstanceDeleted (
    this=0x7fbb65648540, instance=1673976585)
    at webkit/plugins/ppapi/host_globals.cc:243
#18 0x00007fbb69e24fa6 in webkit::ppapi::PluginInstance::~PluginInstance (
    this=0x7fbb656db900, __in_chrg=<optimized out>)
    at webkit/plugins/ppapi/ppapi_plugin_instance.cc:388
#19 0x00007fbb69e2f7e6 in base::RefCounted<webkit::ppapi::PluginInstance>::Release (this=0x7fbb656db908) at ./base/memory/ref_counted.h:95
#20 0x00007fbb6b1bacd0 in scoped_refptr<webkit::ppapi::PluginInstance>::operator= (this=0x7fbb5717c258, p=0x0) at ./base/memory/ref_counted.h:280
#21 0x00007fbb6b1b9ee0 in webkit::ppapi::WebPluginImpl::destroy (
    this=0x7fbb5717c240) at webkit/plugins/ppapi/ppapi_webplugin_impl.cc:119
#22 0x00007fbb6883f411 in WebKit::WebPluginContainerImpl::~WebPluginContainerImpl (this=0x7fbb57163680, __in_chrg=<optimized out>)
    at third_party/WebKit/Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:629
#23 0x00007fbb68841cc6 in WTF::RefCounted<WebCore::Widget>::deref (
    this=0x7fbb57163688) at third_party/WebKit/Source/WTF/wtf/RefCounted.h:190
#24 0x00007fbb68841563 in WTF::derefIfNotNull<WebKit::WebPluginContainerImpl> (
    ptr=0x7fbb57163680) at third_party/WebKit/Source/WTF/wtf/PassRefPtr.h:52
#25 0x00007fbb688960bc in WTF::RefPtr<WebKit::WebPluginContainerImpl>::operator= (this=0x7fbb576f1118, optr=0x7fbb57163820)
    at third_party/WebKit/Source/WTF/wtf/RefPtr.h:126
#26 0x00007fbb68894ab9 in WebKit::FrameLoaderClientImpl::redirectDataToPlugin (
    this=0x7fbb576f1028, pluginWidget=0x7fbb57163820)
    at third_party/WebKit/Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp:1495
#27 0x00007fbb68ca8a4a in WebCore::PluginDocumentParser::appendBytes (
    this=0x7fbb5713d960)
    at third_party/WebKit/Source/WebCore/html/PluginDocument.cpp:129
---Type <return> to continue, or q <return> to quit---y
#28 0x00007fbb6928507c in WebCore::DocumentWriter::addData (
    this=0x7fbb577650c0, bytes=0x0, length=0)
    at third_party/WebKit/Source/WebCore/loader/DocumentWriter.cpp:218
#29 0x00007fbb69275009 in WebCore::DocumentLoader::commitData (
    this=0x7fbb57765000, bytes=0x0, length=0)
    at third_party/WebKit/Source/WebCore/loader/DocumentLoader.cpp:355
#30 0x00007fbb69274c04 in WebCore::DocumentLoader::finishedLoading (
    this=0x7fbb57765000)
    at third_party/WebKit/Source/WebCore/loader/DocumentLoader.cpp:296
#31 0x00007fbb692af1b5 in WebCore::MainResourceLoader::didFinishLoading (
    this=0x7fbb57117400, finishTime=3734932.239753)
    at third_party/WebKit/Source/WebCore/loader/MainResourceLoader.cpp:516
#32 0x00007fbb692c2ca5 in WebCore::ResourceLoader::didFinishLoading (
    this=0x7fbb57117400, finishTime=3734932.239753)
    at third_party/WebKit/Source/WebCore/loader/ResourceLoader.cpp:436
#33 0x00007fbb6b0cd2fe in WebCore::ResourceHandleInternal::didFinishLoading (
    this=0x7fbb57171700, finishTime=3734932.239753)
    at third_party/WebKit/Source/WebCore/platform/network/chromium/ResourceHandle.cpp:156

Comment 7 by tsepez@chromium.org, Jul 27 2012
Cc: w...@chromium.org darin@chromium.org
So here's a change that solves the problem. Seems similar to https://bugs.webkit.org/show_bug.cgi?id=66517 so adding author/reviewer of that patch to ask the question:  Is this an appropriate place to clear the m_pluginWidget?  Or is there some method that didn't happen in the PPAPI case that should have taken care of this?  Let me know if this is OK and I'll file a bug upstream to land it.

Index: third_party/WebKit/Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp
===================================================================
--- third_party/WebKit/Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp	(revision 123007)
+++ third_party/WebKit/Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp	(working copy)
@@ -133,12 +133,16 @@
 
 void FrameLoaderClientImpl::dispatchDidClearWindowObjectInWorld(DOMWrapperWorld*)
 {
+    // Don't let the plugin widget outlive the rest of the page.
+    m_pluginWidget = 0;
     if (m_webFrame->client())
         m_webFrame->client()->didClearWindowObject(m_webFrame);
 }
 
 void FrameLoaderClientImpl::documentElementAvailable()
 {
+    // Verify that the plugin widget didn't outlive the rest of the page.
+    ASSERT(!m_pluginWidget);
     if (m_webFrame->client())
         m_webFrame->client()->didCreateDocumentElement(m_webFrame);
 }

Labels: Security-CodeYellow
Please do read Mark's email titled "Code Yellow: Security Bug Backlog" on chrome-team mailing list.
Comment 9 by w...@chromium.org, Aug 2 2012
Cc: abarth@chromium.org
Adding abarth@, who knows things about the document-loader code.

We've had niggling issues with plugin widget lifetimes seemingly forever; various parts of WebKit assume that they won't outlive the page but at some point that guarantee has been broken - it's possible that this change could fix a swathe of such issues, if the FrameLoaderClientImpl's reference to the widget is the root cause of it living too long.
I don't know anything about plugins.
Argh.  By the time dispatchDidClearWindowObjectInWorld() happens, its already too late.
Ah. This seems like it would do the trick.  FrameLoader::clear() is about to clear out the document in question, so this would be a good time to drop the ref.

Index: WebKit/chromium/src/FrameLoaderClientImpl.cpp
===================================================================
--- WebKit/chromium/src/FrameLoaderClientImpl.cpp	(revision 124582)
+++ WebKit/chromium/src/FrameLoaderClientImpl.cpp	(working copy)
@@ -1491,9 +1491,8 @@
 // (e.g., acrobat reader).
 void FrameLoaderClientImpl::redirectDataToPlugin(Widget* pluginWidget)
 {
-    if (pluginWidget->isPluginContainer())
-        m_pluginWidget = static_cast<WebPluginContainerImpl*>(pluginWidget);
-    ASSERT(m_pluginWidget);
+    ASSERT(!pluginWidget || pluginWidget->isPluginContainer());
+    m_pluginWidget = static_cast<WebPluginContainerImpl*>(pluginWidget);
 }
 
 PassRefPtr<Widget> FrameLoaderClientImpl::createJavaAppletWidget(
Index: WebCore/loader/FrameLoader.cpp
===================================================================
--- WebCore/loader/FrameLoader.cpp	(revision 124582)
+++ WebCore/loader/FrameLoader.cpp	(working copy)
@@ -510,7 +510,9 @@
     if (!m_needsClear)
         return;
     m_needsClear = false;
-    
+
+    m_client->redirectDataToPlugin(0);
+
     if (!m_frame->document()->inPageCache()) {
         m_frame->document()->cancelParsing();
         m_frame->document()->stopActiveDOMObjects();


Seems to run cleanly with this change.  Question about whether this should be a 
separate method in all the FLCs or if all the FLCs should change to expect a NULL argument.

Calling this out of PluginDocument::detach() seems cleaner.
upstremed as https://bugs.webkit.org/show_bug.cgi?id=93283.  We're going to have to handle this on the webkit side.
Comment 15 by cdn@chromium.org, Aug 9 2012
Labels: Mstone-22
Tom, does this affect Stable ? c#3 is confusing, is this a pepper bug ?
I suspect this could affect other plugins, so a merge seems reasonable.
Labels: -Mstone-22 Mstone-21 SecImpacts-Stable
Thanks Tom.
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify Merge-Approved
Status: FixUnreleased
http://trac.webkit.org/changeset/125500
Labels: -Mstone-21 -Merge-Approved Mstone-22 Merge-Merged
M22: http://trac.webkit.org/changeset/127633
Project Member Comment 21 by bugdroid1@chromium.org, Oct 14 2012
Labels: Restrict-AddIssueComment-Commit
This issue has been closed for some time. No one will pay attention to new comments.
If you are seeing this bug or have new data, please click New Issue to start a new bug.
Status: Fixed
Project Member Comment 23 by bugdroid1@chromium.org, Jan 18 2013
Labels: Restrict-View-EditIssue
Restrict-View-EditIssue is preferred since it allows anyone who can edit an issue (committers and contributors) to view the bug.
Project Member Comment 24 by bugdroid1@chromium.org, Mar 10 2013
Labels: -Area-WebKit -Type-Security -SecSeverity-High -Stability-AddressSanitizer -Feature-Flash -Feature-Plugins-Pepper -SecImpacts-Beta -Mstone-22 -SecImpacts-Stable Cr-Content Security-Impact-Beta Type-Bug-Security Cr-Content-Plugins-Flash M-22 Security-Severity-High Security-Impact-Stable Cr-Content-Plugins-Pepper Performance-Memory-AddressSanitizer
Project Member Comment 25 by bugdroid1@chromium.org, Mar 14 2013
Labels: -Restrict-AddIssueComment-Commit Restrict-AddIssueComment-EditIssue
Labels: -Restrict-View-SecurityNotify -Restrict-View-EditIssue
Project Member Comment 27 by bugdroid1@chromium.org, Mar 21 2013
Labels: -Security-Severity-High Security_Severity-High
Project Member Comment 28 by bugdroid1@chromium.org, Mar 21 2013
Labels: -Security-Impact-Stable Security_Impact-Stable
Project Member Comment 29 by bugdroid1@chromium.org, Mar 21 2013
Labels: -Security-Impact-Beta Security_Impact-Beta
Project Member Comment 30 by bugdroid1@chromium.org, Apr 1 2013
Labels: -Performance-Memory-AddressSanitizer Stability-Memory-AddressSanitizer
Project Member Comment 31 by bugdroid1@chromium.org, Apr 6 2013
Labels: -Cr-Content Cr-Blink
Project Member Comment 32 by bugdroid1@chromium.org, Apr 6 2013
Labels: -Cr-Content-Plugins-Flash Cr-Internals-Plugins-Flash
Project Member Comment 33 by bugdroid1@chromium.org, Apr 6 2013
Labels: Cr-Internals-Plugins
Project Member Comment 34 by bugdroid1@chromium.org, Apr 6 2013
Labels: -Cr-Content-Plugins-Pepper Cr-Internals-Plugins-Pepper
Project Member Comment 35 by sheriffbot@chromium.org, Jun 14 2016
Labels: -security_impact-beta
Project Member Comment 36 by sheriffbot@chromium.org, Oct 1 2016
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member Comment 37 by sheriffbot@chromium.org, Oct 2 2016
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: allpublic
Sign in to add a comment