New issue
Advanced search Search tips
This site will be read-only for 3-4 hours starting at Sunday, 08:00AM PDT
Starred by 1 user

Issue metadata

Status: Fixed
Closed: May 2013
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 0
Type: Bug-Security

Sign in to add a comment

Security: CheckDuplicateHandle (BreakDebugger) browser crash with (Web) Workers and WebSQL

Reported by, May 23 2013 Back to list

Issue description

Chrome (browser process) crashes with a CheckDuplicateHandle (BreakDebugger) crash when:

1. Minimized script: 
The browser's history is erased (minimized case) while (multiple) webworkers are triggering a reload from their onmessage event.

2. Extended script: 
Same as 1, but does not require erasing history

The log is the same in both cases: 

[3612:3504:0523/] Check failed: !(basic_info.GrantedAccess & kDangerousMask). You are attempting to duplicate a privileged handle into a sandboxed process.
 Please use the sandbox::BrokerDuplicateHandle API or contact for assistance.

Chrome Version: all (29.0.1517.0 used), issue may be more reliable on recent ToT versions
Operating System: Windows XP SP3 (and others)

1. Launch the minimized script with a clean profile and (fully) erase history

2. Launch the extended script with a clean profile

Type of crash: browser
Crash State: see added trace/log files

69.4 KB View Download
287 bytes View Download
1.5 KB View Download
97.0 KB View Download

Comment 1 by, May 23 2013

Ugh... we have a race in the browser process and we're duping a stale file handle that turns out to be reused as a process handle. That's just ugly.

Comment 2 by, May 23 2013

It looks like the race is in DatabaseMessageFilter::OnDatabaseOpenFile where there is a check for db_tracker_->IsDatabaseScheduledForDeletion(...).

Comment 3 by, May 23 2013

Labels: OS-All Security_Severity-Medium
Michael: would you be a good owner for this? Otherwise, could you help in finding an owner?

I'm tentatively assigning Medium severity: looks like a potential sandbox escape, but probably requires arbitrary code execution in the renderer to do something useful. But this could be High.

@jschuh, what do you mean "a file handle that resuled as a process handle"? And how is it stale?

@meacer, what is the IsDatabaseScheduledForDeletion test racing with?

Comment 5 by, May 23 2013

> @meacer, what is the IsDatabaseScheduledForDeletion test racing with?

The repro says to erase the history while running the test case. I guess the database is scheduled for deletion after IsDatabaseScheduledForDeletion check is done, and gets deleted before the handle is duplicated. I didn't try to reproduce or look closely into this though, so take this with a grain of salt :)
The deletion of websql database files occurs on the same thread of control that this code executes on, so shouldn't be racey.

According to the log, we're trying to dup an INVALID_HANDLE_VALUE in this call...
IPC::GetFileHandleForProcess(void * handle = 0xffffffff, void * process = 0x00000740, bool close_source_handle = true)
... big deal? I don't see why that ultimately warrants a CRASH in (wtf)?

PlatformFileForTransit GetFileHandleForProcess(base::PlatformFile handle,
                                               base::ProcessHandle process,
                                               bool close_source_handle)
...should just return IPC::InvalidPlatformFileForTransit() if 'handle' == kInvalidPlatformFileValue.

Maybe somebody on the security team can tone down the black ice?

Comment 7 by, May 24 2013

Ah, there's no race. INVALID_HANDLE_VALUE is -1, and so is the pseudo handle handle for the current process (thanks Windows). So, this would copy a full privilege handle for the browser into the sandboxed process, effectively shutting off the sandbox. It's pretty much the worst thing the browser process could ever do on Windows.

Comment 8 by, May 24 2013

Labels: -OS-All -Security_Severity-Medium OS-Windows Security_Severity-High Pri-0 Security_Impact-Beta Security_Impact-Stable M-27
Didn't realize this doesn't require user intervention. So, upping the severity and priority since it's an arbitrary sandbox escape on Windows.

On reflection, I should consider making CheckDuplicateHandle fire in official builds as well.

Comment 9 by, May 24 2013

FYI, it looks like MHTMLGenerationManager::CreateFile has the same pattern: GetFileHandleForProcess can potentially be called with INVALID_HANDLE_VALUE.
Labels: Security-Code28
Status: Assigned
Assigning to Justin for this part "I should consider making CheckDuplicateHandle fire in official builds as well." :)
Yeah, I don't think I'm gonna do that. As an immediate mitigation I'm just going to have GetFileHandleForProcess check for INVALID_HANDLE_VALUE. The rationale here is that only the file and pipe functions use INVALID_HANDLE_VALUE, so it's easy enough to catch this there.

More broadly, I'll have to think about longer term solutions to this pattern. CheckDuplicateHandle was supposed to make it easy to catch these early, but it relies on us having sufficient code coverage.
Labels: reward-topanel

Comment 13 by, May 24 2013

Can we make CheckDuplicateHandle explicitly check for -1 in release + debug ? And the rest of the check could stay DEBUG only.
Browser process hooks in official builds are almost always a bad idea.
Labels: Merge-Approved
Status: Fixed
This is a trivial merge that we'll want to pick up for the next release.
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: -Merge-Approved Merge-Merged Release-1
M27 is r202611
M28 is r202612
Labels: -reward-topanel reward-2000 reward-unpaid
@therealholden: great find. We'll reward this one at the $2000 level.
Labels: CVE-2013-2854
Labels: -reward-unpaid reward-inprocess
Labels: -Restrict-View-SecurityNotify
Bulk release of old security bug reports.

Labels: -reward-inprocess
Project Member

Comment 25 by ClusterFuzz, Feb 2 2016

Labels: -Security_Impact-Beta
Project Member

Comment 26 by, Oct 1 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit - Your friendly Sheriffbot
Project Member

Comment 27 by, Oct 2 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit - Your friendly Sheriffbot
Labels: allpublic

Sign in to add a comment