New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 249502 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2013
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security
wip
Nag



Sign in to add a comment

Security: (Shared) (WebSQL) Worker races cause invalid pointers in DatabaseObserver::databaseClosed and DatabaseObserver::reportOpenDatabaseResult

Reported by therealh...@gmail.com, Jun 13 2013

Issue description

VULNERABILITY DETAILS
Chrome crashes in DatabaseObserver::databaseClosed and DatabaseObserver::reportOpenDatabaseResult with an invalid pointer when multiple (random name) databases are opened while the script reloads (continuously) from multiple shared workers onmessage events.

Many other trivial 0-ptr crashes like chromiumOpen, chromiumAccess and chromiumDelete can occur. All seem to be caused by worker races.

It is also possible that the script causes an invalid handle (stack corruption?) while reloading the worker (process). Without a debugger this can (probably) happen unnoticed.

VERSION
Chrome Version: 27.0.1453.110 stable - 29.0.1538.0 continuous (trace)
Operating System: Windows: XP SP3, 7 SP1

REPRODUCTION CASE
Launch the added script

FOR CRASHES, PLEASE INCLUDE THE FOLLOWING ADDITIONAL INFORMATION
Type of crash: worker
Crash State: see added stack trace files

 
databaseclosed_crash_trace.txt
8.6 KB View Download
reportopendatabaseresult_crash_trace.txt
20.7 KB View Download
websql_dbclosed_reportopen_crash_repro.html
786 bytes View Download

Comment 1 by meacer@google.com, Jun 13 2013

Labels: Security_Severity-High OS-All Security_Impact-Stable Cr-Blink-Workers Pri-1 Cr-Blink
Owner: mea...@chromium.org
Status: Assigned
It's a UAF in DatabaseObserver. WorkerThread's web_database_observer_impl_ is passed to WebDatabase::setObserver when worker thread is created. The observer is freed when thread shuts down, but it's still used in DatabaseObserver.

I've touched this stuff recently, I can take a look.

Comment 2 by mea...@chromium.org, Jun 18 2013

Cc: dim...@chromium.org dgro...@chromium.org michaeln@chromium.org jsb...@chromium.org
CC'ing folks who have commented on bugs similar to this.

As I mentioned in #1, the problem is that web_database_observer_impl_ is being used from DatabaseObserver after the worker thread shuts down. This is causing a UAF.

I tried to patch this by calling web_database_observer_impl_->WaitForAllDatabasesToClose(); in WorkerThread::Shutdown before anything else, but that brings other null derefs such as Platform::current.
Cc: abarth@chromium.org
I gotta question the priority and severity of this class of bug that occurs only in the act of exiting the process, past the point of no return.

The root cause of these problems are that content::WorkerThread/RenderThread shutdown and destruction occur prior the the background threads started in blink having run to completion. A systemic way to deal with that would be the better than one off "fixes" to each individual symptom of that larger problem.

WebKit::shutdown() could be responsible for terminating those background threads. Or instead of getting that far along, the ChildThread could send a sync IPC to the browser process that sayz "kill me now". Code execution on the child thread will never get past that point. So we'd never even try to shutdown webkit.

cc'ing adam who was looking into recasting wecore's threading in terms of chromium's primitives.


In this particular case, there is one instance of WebDatabaseObserverImpl created in the process. It's deleted by content::WorkerThread and content::RenderThread (depending on process type) at the end of time. I think leaking that observer object instead of deleting it would "fix" this bug.

Comment 4 by mea...@chromium.org, Jun 18 2013

>I gotta question the priority and severity of this class of bug that occurs only in the act of exiting the process, past the point of no return.

Yes, I didn't initially realize this was during process exit.

> Or instead of getting that far along, the ChildThread could send a sync IPC to the browser process that sayz "kill me now".

Is it possible that killing the worker process instantly by the browser process can corrupt IDB and those kind of stuff?

Comment 5 by jsb...@chromium.org, Jun 18 2013

> Is it possible that killing the worker process instantly by the browser process can corrupt IDB and those kind of stuff?

The various storage APIs need to be robust against crashing renderer (or shared worker) processes anyway. IDB has specific tests in this area to ensure the state is consistent even if a renderer process crashes, give or take any unknown bugs in this area.

+1 to webcore -> chromium threading; when I saw abarth's initial work in that area I was optimistic we could resolve this more easily.

Comment 6 by abarth@chromium.org, Jun 18 2013

> +1 to webcore -> chromium threading; when I saw abarth's initial work in that area I was optimistic we could resolve this more easily.

I'm not currently working on doing that conversion, but I'm happy to share what I learned in my investigation.
> Is it possible that killing the worker process instantly by the browser
> process can corrupt IDB and those kind of stuff?

IDB not so much compared to websql, but in both cases we do have stuff in place to defend against random renderer crashes, we have to have that no matter what. In websql's case, we rely on sqlites transaction journal files.

I don't think killing the worker/renderer instantly in ~process would actually change anything in this regard since given the current state of affairs, the background threads are still running at exit time anyway and i suspect they're ultimately getting killed off w/o running to completion anyway.

We'd want to be careful about flushing the outgoing ipc queue, or rather, not changing the behavior in that regard.


Labels: M-27
On 7(/8) x32/x64, the repro (also) causes invalid handle issues (with memory corruption) on worker shutdown. 

I have no idea if this is related. It may be related to another issue.

Also, I can't repro this on Linux (with ASAN).

249502_invalid_handle_trace_x32.txt
5.3 KB View Download
249502_invalid_handle_trace_x64.txt
4.6 KB View Download
This seems to have gone idle. @meacer, are we able to reproduce anything, are you still looking into this, or should we find a new owner?
Labels: -M-27 Security_Impact-Beta M-29
Bulk move. M29 is released.
Fix labels.
Labels: reward-topanel
Status: Started
As an initial fix, sending a sync IPC to request killing the worker process seems to work. I'll polish the patch a bit.
Here is a first attempt: https://codereview.chromium.org/23496052/
Cc: atwilson@chromium.org
+atwilson so that he can see this bug.
Cc: jam@chromium.org jochen@chromium.org
Cc: dcarney@chromium.org
re #16: I think this just covers up the bug

We need to spin down the database thread when shutting down WebKit. Maybe we should consider rebasing and landing https://bugs.webkit.org/show_bug.cgi?id=68303

that would break a lot of dependencies on the database thread.
Worth discussing the gist of this approach vs an alternative, here's my take.

This solution in the CL is better than writing code to carefully terminate and
join webcore's background threads within WebKit::shutdown() without introducing
a deadlock. Any solution of that form would likely not be correct and would
practically speaking never be fully tested. Security vulnerabilities would still
be there. This solution makes this class of shutdown bug impossible. That's has a lot of appeal.

I don't think reviving the old patch jochen referred to and landing it would resolve this problem.

Killing the process to get shutdown right is a rather heavy handed solution IMO.

Also, we might run into the same problem with renderers. And what about ChromeView where we run in single-process mode?

The patch I was referring to solves the following problem: Currently, the database thread owns all WebDatabases - in order to close a database, you need to spin down the database thread. The patch changes this, so you can delete a database on the main thread, and the database thread just retains enough information to inform the embedder when a database was closed. While not part of the patch, this would make it possible to change the interface that the database closed callback just gets the database name and security origin and not the entire database. Then the database thread doesn't own anything it could use after freeing it.
> Killing the process to get shutdown right is a rather heavy handed solution IMO.

Yes it is.

> Also, we might run into the same problem with renderers.

Renderers don't run shared workers atm, but it's true that if/when renderers do run shared workers, i'd say we might want to adopt this strategy there too.

> And what about ChromeView where we run in single-process mode?

Single-process mode definitely doesn't run shared workers.

> The patch I was referring to solves the following problem...

All that is fine and good, but you realize while that large change might make it a little easier to deal with WebSQL issues at shutdown, it doesn't actually include changes to handle process shutdown better, subsequent changes are required for that (who knows how large).  Also it does nothing about any other subsystem. A much wider range of things go wrong in worker process shutdown.

Also how many new bugs does the large change you referenced introduce?

The approach is this CL casts a pretty wide net. From a security point of view, I think this is very appealing.
Did you saw our new criteria for possibly issuing higher rewards? See http://www.chromium.org/Home/chromium-security/vulnerability-rewards-program/reward-nomination-process
E.g. If you are able to provide a repro that faulted at an address of 0x41414141, it will qualify for the new higher rewards. Or, if you can show that you have control between free and crash points, etc.
Project Member

Comment 24 by ClusterFuzz, Sep 27 2013

Labels: Nag
Status: Assigned
meacer@: you haven't provided any bug update or come up with a fix for this issue in the last 7 days. Please note that this is a medium+ severity security vulnerability that needs your immediate response. If you have a patch in progress and don't want future nags, please add a codereview link and a WIP label. If the issue is already fixed or you can't reproduce it, please close the bug.
Status: Started
Mr Clusterfuzz: The CL for killing the worker right before exit is still being discussed in email threads.
Labels: WIP
Project Member

Comment 27 by ClusterFuzz, Oct 1 2013

Labels: -M-29 M-30
Fixing milestone and impact labels.
 Issue 303154  has been merged into this issue.
The title of the merged  issue 303154  (found by automated testing?) does not refer to the real issue (ThreadDataTable::RemoveAllThreads UAF). It does refer to one of the trivial crashes I mention in the original comment.

This ChromeURLRequestContextGetter::GetURLRequestContext 0-ptr browser crash is actually an old one (issue 99242).
Re #29: Yes, the automated test hit a null pointer rather than the UAF. It might hit the UAF when we rerun. But the root causes for the bugs are the same, so that's why we merged it into this one.

Comment 31 by cdn@chromium.org, Oct 17 2013

meacer@ are you actively working on this or should we find another owner?
Project Member

Comment 32 by ClusterFuzz, Oct 18 2013

Labels: Deadline-Exceeded
You have far exceeded the 60-day deadline for fixing this high severity security vulnerability.

We commit ourselves to this deadline and appreciate your utmost priority on this issue.

If you are unable to look into this soon, please find someone else to own this.
There was already a patch for this sitting around: https://codereview.chromium.org/23496052/

We discussed offline and it looks like there is a risk of exploitation, so I'm going to land the patch soon. Jochen said that he started working on shutting down threads correctly. My patch is going to be a bandaid in the meanwhile.
Project Member

Comment 34 by bugdroid1@chromium.org, Nov 5 2013

------------------------------------------------------------------------
r233099 | meacer@chromium.org | 2013-11-05T22:13:37.912217Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/worker/worker_thread.h?r1=233099&r2=233098&pathrev=233099
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/worker_host/worker_process_host.cc?r1=233099&r2=233098&pathrev=233099
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/worker_host/worker_process_host.h?r1=233099&r2=233098&pathrev=233099
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/worker/worker_thread.cc?r1=233099&r2=233098&pathrev=233099
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/common/worker_messages.h?r1=233099&r2=233098&pathrev=233099

Kill worker process by way of a sync IPC message before it cleans up.

When a worker process shuts down, it shuts down WebKit. If there are
other threads running in the worker process, this leads to crashes.
This fix tries to kill the worker process forcibly so that no cleanup
takes place.

BUG= 249502 

Review URL: https://codereview.chromium.org/23496052
------------------------------------------------------------------------
Status: Fixed
Ok, so this is tentatively fixed with https://codereview.chromium.org/23496052 but Jochen is already working on shutting down threads correctly in worker processes. That will be the actual fix for this whole class of worker bugs. Until then, I'm closing this bug and a few similar bugs that this patch should have fixed.
To clarify #36: https://codereview.chromium.org/23496052 doesn't fix bug 243840 which is also a worker related bug. That bug is different since it manifests itself during a profile being closed.
Project Member

Comment 38 by ClusterFuzz, Nov 6 2013

Labels: -M-30
Project Member

Comment 39 by ClusterFuzz, Nov 6 2013

Labels: M-32 M-31 M-30 Merge-Triage
Adding Merge-Triage label for tracking purposes.

Once your fix had sufficient bake time (on canary, dev as appropriate), please nominate your fix for merge by adding the Merge-Requested label.

When your merge is approved by the release manager, please start merging with higher milestone label first. Make sure to re-request merge for every milestone in the label list. You can get branch information on omahaproxy.appspot.com.

Your fix is very close to the branch point. After the branch happens, please make sure to check if your fix is in.

- Your friendly ClusterFuzz
Project Member

Comment 40 by ClusterFuzz, Nov 6 2013

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Project Member

Comment 41 by bugdroid1@chromium.org, Nov 6 2013

------------------------------------------------------------------------
r233367 | jochen@chromium.org | 2013-11-06T21:11:42.771746Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/child/web_database_observer_impl.cc?r1=233367&r2=233366&pathrev=233367
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/child/web_database_observer_impl.h?r1=233367&r2=233366&pathrev=233367

Decouple lifetime of database thread and WebDatabase, chromium side.

Add a method to WebDatabaseObserver to close a database without holding
on to an actual WebDatabase. This will make it possible to decouple the
lifetime of WebDatabase and the database thread.

BUG= 249502 
R=jam@chromium.org

Review URL: https://codereview.chromium.org/50883004
------------------------------------------------------------------------
Labels: -Merge-Triage Merge-Requested
 Issue 258609  has been merged into this issue.
 Issue 258609  has been merged into this issue.

Comment 45 by kareng@google.com, Nov 11 2013

Labels: -Merge-Requested Merge-Approved
Project Member

Comment 46 by bugdroid1@chromium.org, Nov 12 2013

Labels: -Merge-Approved merge-merged-1700
------------------------------------------------------------------------
r234344 | meacer@chromium.org | 2013-11-12T00:16:33.888160Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1700/src/content/worker/worker_thread.cc?r1=234344&r2=234343&pathrev=234344
   M http://src.chromium.org/viewvc/chrome/branches/1700/src/content/common/worker_messages.h?r1=234344&r2=234343&pathrev=234344
   M http://src.chromium.org/viewvc/chrome/branches/1700/src/content/worker/worker_thread.h?r1=234344&r2=234343&pathrev=234344
   M http://src.chromium.org/viewvc/chrome/branches/1700/src/content/browser/worker_host/worker_process_host.cc?r1=234344&r2=234343&pathrev=234344
   M http://src.chromium.org/viewvc/chrome/branches/1700/src/content/browser/worker_host/worker_process_host.h?r1=234344&r2=234343&pathrev=234344

Merge 233099 "Kill worker process by way of a sync IPC message b..."

> Kill worker process by way of a sync IPC message before it cleans up.
> 
> When a worker process shuts down, it shuts down WebKit. If there are
> other threads running in the worker process, this leads to crashes.
> This fix tries to kill the worker process forcibly so that no cleanup
> takes place.
> 
> BUG= 249502 
> 
> Review URL: https://codereview.chromium.org/23496052

TBR=meacer@chromium.org

Review URL: https://codereview.chromium.org/69753002
------------------------------------------------------------------------
Jochen: Do we need to merge r233367 as well?
From mail thread: No need to merge.
Labels: -M-32 -M-30 Merge-Requested
Merge-Requested for m31.
I believe that this is fixed on ToT and we can remove the stop-gap.

I'll try to verify locally that this dosn't repro anymore after removing the stop-gap.
Labels: -Merge-Requested Merge-Approved
Labels: -Merge-Approved Merge-Merged Release-1-M31
Merged r233099 to m31 in r238433.
Labels: merge-merged-1650
Project Member

Comment 54 by bugdroid1@chromium.org, Dec 3 2013

------------------------------------------------------------------------
r238433 | inferno@chromium.org | 2013-12-03T18:27:04.918288Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1650/src/content/worker/worker_thread.cc?r1=238433&r2=238432&pathrev=238433
   M http://src.chromium.org/viewvc/chrome/branches/1650/src/content/common/worker_messages.h?r1=238433&r2=238432&pathrev=238433
   M http://src.chromium.org/viewvc/chrome/branches/1650/src/content/worker/worker_thread.h?r1=238433&r2=238432&pathrev=238433
   M http://src.chromium.org/viewvc/chrome/branches/1650/src/content/browser/worker_host/worker_process_host.cc?r1=238433&r2=238432&pathrev=238433
   M http://src.chromium.org/viewvc/chrome/branches/1650/src/content/browser/worker_host/worker_process_host.h?r1=238433&r2=238432&pathrev=238433

Merge 233099 "Kill worker process by way of a sync IPC message b..."

> Kill worker process by way of a sync IPC message before it cleans up.
> 
> When a worker process shuts down, it shuts down WebKit. If there are
> other threads running in the worker process, this leads to crashes.
> This fix tries to kill the worker process forcibly so that no cleanup
> takes place.
> 
> BUG= 249502 
> 
> Review URL: https://codereview.chromium.org/23496052

TBR=meacer@chromium.org

Review URL: https://codereview.chromium.org/102543003
------------------------------------------------------------------------
Labels: -reward-topanel reward-unpaid CVE-2013-6633 reward-1000
Thanks for the report! This one qualifies for a $1000 reward. It did not qualify at a higher reward level because it did not seem like this would be easily exploitable.
Thanks!
Labels: -Release-1-M31 Release-0-M32
giving compile failures on m31 merge, punting to m32.
Labels: -CVE-2013-6633
Project Member

Comment 60 by bugdroid1@chromium.org, Dec 3 2013

------------------------------------------------------------------------
r238483 | inferno@chromium.org | 2013-12-03T23:32:41.856164Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1650/src/content/worker/worker_thread.cc?r1=238483&r2=238482&pathrev=238483
   M http://src.chromium.org/viewvc/chrome/branches/1650/src/content/common/worker_messages.h?r1=238483&r2=238482&pathrev=238483
   M http://src.chromium.org/viewvc/chrome/branches/1650/src/content/worker/worker_thread.h?r1=238483&r2=238482&pathrev=238483
   M http://src.chromium.org/viewvc/chrome/branches/1650/src/content/browser/worker_host/worker_process_host.cc?r1=238483&r2=238482&pathrev=238483
   M http://src.chromium.org/viewvc/chrome/branches/1650/src/content/browser/worker_host/worker_process_host.h?r1=238483&r2=238482&pathrev=238483

Revert 238433 "Merge 233099 "Kill worker process by way of a syn..."

> Merge 233099 "Kill worker process by way of a sync IPC message b..."
> 
> > Kill worker process by way of a sync IPC message before it cleans up.
> > 
> > When a worker process shuts down, it shuts down WebKit. If there are
> > other threads running in the worker process, this leads to crashes.
> > This fix tries to kill the worker process forcibly so that no cleanup
> > takes place.
> > 
> > BUG= 249502 
> > 
> > Review URL: https://codereview.chromium.org/23496052
> 
> TBR=meacer@chromium.org
> 
> Review URL: https://codereview.chromium.org/102543003

TBR=inferno@chromium.org

Review URL: https://codereview.chromium.org/103243002
------------------------------------------------------------------------
Cc: devora@google.com
Labels: -reward-unpaid reward-inprocess
Payment just kicked off here. Thanks again for your help!
Labels: CVE-2013-6646
Labels: -reward-inprocess
Project Member

Comment 65 by ClusterFuzz, Mar 28 2014

Labels: -Restrict-View-SecurityNotify
Bulk update: removing view restriction from closed bugs.
Project Member

Comment 66 by ClusterFuzz, Feb 2 2016

Labels: -Security_Impact-Beta
Project Member

Comment 67 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 68 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
Labels: CVE_description-submitted

Sign in to add a comment