Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Issue 179580 Devtools uses dangling WebContents* when extension reloads
Starred by 1 user Project Member Reported by jyasskin@chromium.org, Mar 2 2013 Back to list
Status: Fixed
Owner:
Closed: Mar 2013
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug-Security



Sign in to add a comment
The attached unpacked extension reloads in a continuous loop. If you try to inspect its background page in a Debug chrome, you usually get the following stack trace:

[3026:3026:0301/173101:FATAL:lock_impl_posix.cc(46)] Check failed: rv == 0 (22 vs. 0)Invalid argument

#1  0x00007fffebfe1a2a in logging::LogMessage::~LogMessage (this=0x7fffffff0520)
    at ../../base/logging.cc:649
#2  0x00007fffec31a269 in base::internal::LockImpl::Lock (this=0x604a000a01d0)
    at ../../base/synchronization/lock_impl_posix.cc:46
#3  0x00007fffebcc32c0 in base::Lock::Acquire (this=0x604a000a01c8)
    at ../../base/synchronization/lock.h:41
#4  0x00007fffebcc318b in base::AutoLock::AutoLock (this=0x7fffffff0b20, lock=...)
    at ../../base/synchronization/lock.h:100
#5  0x00007fffebcc2b9d in base::AutoLock::AutoLock (this=0x7fffffff0b20, lock=...)
    at ../../base/synchronization/lock.h:101
#6  0x00007fffec41ded4 in base::ThreadCheckerImpl::EnsureThreadIdAssigned (this=0x604a000a01c8)
    at ../../base/threading/thread_checker_impl.cc:28
#7  0x00007fffec41e35f in base::ThreadCheckerImpl::CalledOnValidThread (this=0x604a000a01c8)
    at ../../base/threading/thread_checker_impl.cc:17
#8  0x00007fffec300a59 in base::SupportsUserData::GetUserData (this=0x604a000a0190,
    key=0x55556d1136c0 <content::WebContentsUserData<SessionTabHelper>::kLocatorKey>)
    at ../../base/supports_user_data.cc:15
#9  0x0000555558e4f7ca in content::WebContentsUserData<SessionTabHelper>::FromWebContents (
    contents=0x604a000a0180) at ../../content/public/browser/web_contents_user_data.h:46
#10 0x000055555d1e2270 in DevToolsWindow::AddDevToolsExtensionsToClient (this=0x602400454740)
    at ../../chrome/browser/devtools/devtools_window.cc:466
#11 0x000055555d1e398a in DevToolsWindow::Observe (this=0x602400454740, type=7, source=...,
    details=...) at ../../chrome/browser/devtools/devtools_window.cc:531
#12 0x00007fffba2eb8f8 in content::NotificationServiceImpl::Notify (this=0x601600008180, type=7,
    source=..., details=...) at ../../content/browser/notification_service_impl.cc:130
#13 0x00007fffbb0b727d in content::WebContentsImpl::SetIsLoading (this=0x604a000cd300,
    is_loading=false, details=0x6018004ad700)
    at ../../content/browser/web_contents/web_contents_impl.cc:2534
#14 0x00007fffbb0c36fe in content::WebContentsImpl::DidStopLoading (this=0x604a000cd300,
    render_view_host=0x604a000cc600)
    at ../../content/browser/web_contents/web_contents_impl.cc:2998
...

Frame #10 uses DevToolsWindow::inspected_web_contents_ after it's been destroyed. inspected_web_contents_ is assigned at https://cs.corp.google.com/#chrome/src/chrome/browser/devtools/devtools_window.cc&l=266&rcl=1361885264 and never updated, even when the RenderViewHost is destroyed by a chrome.runtime.reload call.

Pavel, you seem to have touched the devtools code most recently; could you look into this?
 
reload.zip
635 bytes Download
Labels: -Pri-1 Pri-2 OS-All SecSeverity-Medium Mstone-27
Looks like a racy UAF in a malicious extension, which jschuh and I think mitigates the severity of the bug. Still definitely good to fix though.

On a ToT Debug build (Linux), I get only a total browser freeze, no stack trace. What version(s) have you tested this on, jyasskin?
I think I was running it under gdb, r184931. I also get just the freeze outside of gdb. asan/Debug (outside of gdb) showed me this trace at least once, and asan/Release flags the heap-use-after-free.
Cc: kaznacheev@chromium.org
Owner: kaznacheev@chromium.org
Status: Started
https://codereview.chromium.org/12431011/
Project Member Comment 5 by bugdroid1@chromium.org, Mar 10 2013
Labels: -Type-Security -Feature-Extensions -Feature-DevTools -SecSeverity-Medium -Mstone-27 Cr-Platform-Extensions Security-Severity-Medium Cr-Platform-DevTools M-27 Type-Bug-Security
Patch https://chromiumcodereview.appspot.com/12431011/ fixes the problem (checked with ASAN)
Project Member Comment 7 by bugdroid1@chromium.org, Mar 13 2013
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=187848

------------------------------------------------------------------------
r187848 | kaznacheev@chromium.org | 2013-03-13T13:15:26.886264Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/devtools/devtools_window.cc?r1=187848&r2=187847&pathrev=187848
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/devtools/devtools_window.h?r1=187848&r2=187847&pathrev=187848
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/devtools/render_view_devtools_agent_host.cc?r1=187848&r2=187847&pathrev=187848

Avoid retaining the pointer to inspected WebContents in DevToolsWindow

This fixes the 'use-after-free' bug.

BUG= 179580 


Review URL: https://chromiumcodereview.appspot.com/12431011
------------------------------------------------------------------------
Status: Fixed
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify Merge-Approved
This is Merge-Approved and M-27, but M27 branch is not cut yet. Should I merge it into M26?
Labels: -Merge-Approved Release-0
Merge seems non-trivial. Given that it requires an evil extension (and may also require the devtools to be open?) seems like it's a reasonable call to let this fix land in M27.
BTW, is this an M26 regression? Or the bug exists in M25 stable?
This might factor into the merge vs. no-merge decision, too.
I think it has been there for ages. And it is quite exotic. It needs DevTools to be opened for extension (from extensions developer mode). And extension needs to reload from onload. I.e. be malicious / broken.
@pfeldman: awesome, thanks for the details! Give how exotic it is, let's just let the fix drift into M27.

Gotta <3 the 6 week release cycle.
Project Member Comment 15 by bugdroid1@chromium.org, Mar 21 2013
Labels: -Security-Severity-Medium Security_Severity-Medium
Labels: -Restrict-View-SecurityNotify
Bulk release of old security bug reports.

Project Member Comment 17 by sheriffbot@chromium.org, Jun 14 2016
Labels: -release-0
This bug is a regression and does not impact stable. Removing incorrectly added Release- labels.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member Comment 18 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 19 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