New issue
Advanced search Search tips

Issue 871522 link

Starred by 2 users
Status: Fixed
Owner:
brucedaw...@chromium.org
Closed: Aug 14
Cc:
robliao@chromium.org
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug



Sign in to add a comment

Crash Using The Chromebook Recovery Extension

Project Member Reported by robliao@chromium.org, Aug 6 
Chrome Windows 70.0.3514.0 (Official Build) canary (64-bit) (cohort: Clang-64)

crash/708e3004887cdbd5

1. Fire up the Chromebook Recovery Utility (Available on the Chrome Web Store)
2. Search for a Chromebook (Like "Link Wisteria")
3. Start the recovery process.
4. Wait for the download and the UAC prompt.
5. Allow the UAC prompt to proceed.

Observe a crash.

Suspected CL:
https://chromium.googlesource.com/chromium/src.git/+/02f07de804a9c247c5b6405f0d7dde88da6bee3c

0:002> !gle
LastErrorValue: (Win32) 0x5 (5) - Access is denied.
LastStatusValue: (NTSTATUS) 0xc0000022 - {Access Denied}  A process has requested access to an object, but has not been granted those access rights.

0:002> kn20
 # Child-SP          RetAddr           Call Site
00 0000002d`aa5fef20 00007ff9`b2a96fbd chrome!content::ChildProcessData::SetHandle+0xc3 [C:\b\c\b\win64_clang\src\content\public\browser\child_process_data.cc @ 23] 
01 0000002d`aa5fefa0 00007ff9`b2a96efb chrome!content::BrowserChildProcessHostImpl::OnProcessLaunched+0x87 [C:\b\c\b\win64_clang\src\content\browser\browser_child_process_host_impl.cc @ 607] 
02 0000002d`aa5ff080 00007ff9`b2a96ea6 chrome!content::ChildProcessLauncher::Notify+0x35 [C:\b\c\b\win64_clang\src\content\browser\child_process_launcher.cc @ 81] 
03 0000002d`aa5ff0d0 00007ff9`b2a96df5 chrome!content::internal::ChildProcessLauncherHelper::PostLaunchOnClientThread+0x98 [C:\b\c\b\win64_clang\src\content\browser\child_process_launcher_helper.cc @ 169] 
04 0000002d`aa5ff140 00007ff9`b29a3b4c chrome!base::internal::Invoker<base::internal::BindState<void (content::internal::ChildProcessLauncherHelper::*)(content::internal::ChildProcessLauncherHelper::Process, int),scoped_refptr<content::internal::ChildProcessLauncherHelper>,content::internal::ChildProcessLauncherHelper::Process,int>,void ()>::RunOnce+0x3f [C:\b\c\b\win64_clang\src\base\bind_internal.h @ 658] 
05 0000002d`aa5ff1a0 00007ff9`b29a3547 chrome!base::debug::TaskAnnotator::RunTask+0x12c [C:\b\c\b\win64_clang\src\base\debug\task_annotator.cc @ 101] 
06 0000002d`aa5ff2c0 00007ff9`b299ca85 chrome!base::MessageLoop::RunTask+0x247 [C:\b\c\b\win64_clang\src\base\message_loop\message_loop.cc @ 433] 
07 0000002d`aa5ff420 00007ff9`b299c8ea chrome!base::MessageLoop::DoWork+0x185 [C:\b\c\b\win64_clang\src\base\message_loop\message_loop.cc @ 514] 
08 0000002d`aa5ff650 00007ff9`b299c77e chrome!base::MessagePumpForIO::DoRunLoop+0x14a [C:\b\c\b\win64_clang\src\base\message_loop\message_pump_win.cc @ 482] 
09 0000002d`aa5ff6f0 00007ff9`b299c4e1 chrome!base::MessagePumpWin::Run+0x4e [C:\b\c\b\win64_clang\src\base\message_loop\message_pump_win.cc @ 54] 
0a 0000002d`aa5ff740 00007ff9`b299c476 chrome!base::RunLoop::Run+0x31 [C:\b\c\b\win64_clang\src\base\run_loop.cc @ 108] 
0b 0000002d`aa5ff770 00007ff9`b2999fdb chrome!content::BrowserProcessSubThread::IOThreadRun+0x24 [C:\b\c\b\win64_clang\src\content\browser\browser_process_sub_thread.cc @ 179] 
0c 0000002d`aa5ff7b0 00007ff9`b3b8e614 chrome!base::Thread::ThreadMain+0x19b [C:\b\c\b\win64_clang\src\base\threading\thread.cc @ 360] 
0d 0000002d`aa5ff840 00007ff9`f6bd1fe4 chrome!base::`anonymous namespace'::ThreadFunc+0xf4 [C:\b\c\b\win64_clang\src\base\threading\platform_thread_win.cc @ 94] 
0e 0000002d`aa5ff8c0 00007ff9`f6f3cb31 KERNEL32!BaseThreadInitThunk+0x14
0f 0000002d`aa5ff8f0 00000000`00000000 ntdll!RtlUserThreadStart+0x21

Comment 1 by robliao@chromium.org, Aug 6

Description: Show this description

Comment 2 by robliao@chromium.org, Aug 8

Labels: Target-70 ReleaseBlock-Stable M-70

Comment 3 by brucedaw...@chromium.org, Aug 13

Status: Started (was: Assigned)
I just saw this now for some reason. This is definitely related to my change but I think my change merely revealed an existing bug. Prior to my change BrowserChildProcessHostImpl::OnProcessLaunched would just assign the HANDLE to a variable. Now it duplicates the handle which means the handle has to be valid and accessible.

Random question: when I load the crash dump and type !gle it says that the LastErrorValue is 0 instead of 5. Any idea why?

What process is being launched when the crash occurs?
I assume that the access is denied failure is because it is launched as admin. Any thoughts on what the correct fix is?

I cannot find the Chromebook Recovery Utility. I see results like EditThisCookie, and various CPH extensions. My search query ends up looking like this:

https://chrome.google.com/webstore/search/chromebook%20recovery%20utility?utm_source=chrome-ntp-icon

Comment 4 by brucedaw...@chromium.org, Aug 13 

Oh wait, found it. A regular Google search finds it. I don't know why searching the Chrome Web Store doesn't. I can reproduce the crash.

Comment 5 by robliao@chromium.org, Aug 13 

> when I load the crash dump and type !gle it says that the LastErrorValue is 0 instead of 5. Any idea why?
When do we capture the last error? I've always suspected our crash handlers may occasionally call into Win32 and blow away the last error.

> I assume that the access is denied failure is because it is launched as admin. Any thoughts on what the correct fix is?
Yeah, I suspect this has something to do with it. I haven't looked deeply enough to draw a conclusion on the right fix.

> I cannot find the Chromebook Recovery Utility.
They don't make it easy to find it appears. Here's a direct link:
https://chrome.google.com/webstore/detail/chromebook-recovery-utili/jndclpdbaamdhonoechobihbbiimdgai

Comment 6 by brucedaw...@chromium.org, Aug 13 

And... easy fix. Thanks for the report. I believe that crrev.com/c/1173499 will correct this (it works in local testing).
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 14 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/24b4ca38b52a72ee531d30128bd3186700a4e0a1

commit 24b4ca38b52a72ee531d30128bd3186700a4e0a1
Author: Bruce Dawson <brucedawson@chromium.org>
Date: Tue Aug 14 19:48:45 2018

Use DUPLICATE_SAME_ACCESS to duplicate privileges

When launching a process with admin rights we can't duplicate the
process handle with dwDesiredAccess == PROCESS_QUERY_INFORMATION.
But we don't need to - we just need the same rights we have to the
original handle. So we should instead set dwOptions to
DUPLICATE_SAME_ACCESS.

This is an update to correct the flaw added in crrev.com/c/1155498

Bug:  871522 
Change-Id: Ib5e526c1a1dfef07e6bb5e6d4d64e12c2d4c3eed
Reviewed-on: https://chromium-review.googlesource.com/1173499
Reviewed-by: Nick Carter <nick@chromium.org>
Reviewed-by: Robert Liao <robliao@chromium.org>
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583000}
[modify] https://crrev.com/24b4ca38b52a72ee531d30128bd3186700a4e0a1/content/public/browser/child_process_data.cc

Comment 8 by brucedaw...@chromium.org, Aug 14

Status: Fixed (was: Started)
This is fixed. The fix should show up in tomorrow's canary build. Thanks for finding and reporting.

Comment 9 by viswa.karala@chromium.org, Aug 16

Labels: TE-NeedsTriageFromHYD
As per comment# 0, the issue seems to be related to Chromebook, hence routing this issue to Inhouse team for verifying the fix, adding TE-NeedsTriageFromHYD label to it.

Thanks!

Comment 10 by brucedaw...@chromium.org, Aug 16 

The issue is not related to Chromebooks. It is entirely a Windows issue. The only Chromebook relevance is that this utility requires elevation *on Windows* and therefore exposes a bug.

I just verified in today's canary that the bug is fixed. The fix can *only* be verified on a Windows machine. Testing on a ChromeBook would be meaningless.
►

Sign in to add a comment