ASan-win32 output.

Deleted: heap-use-after-free on address 0x1e48b080.txt 11.2 KB

heap-use-after-free on address 0x1e48b080.txt 11.2 KB View Download

I am unable to reproduce this crash on the same build (62.0.3165.0) on Windows 10.
Do you have more reliable reproduction steps?

I can repro the crash more easly on Canary (62.0.3166.0) on Windows 7 and Mac.

Crash/cd672e0140000000.

Deleted: Enregistrement #3.mp4 2.1 MB

	Enregistrement #3.mp4 2.1 MB View Download

Thank you for providing more feedback. Adding requester "vakh@chromium.org" to the cc list and removing "Needs-Feedback" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Note: This is a regression bug.

A crash reproduced for me in Chrome Canary 61.0.3161.0 on Mac OS X.
crash/4a54b379f8000000

crash/23fd51a268000000 shows the issue in Chrome 62.3166 with a slightly different stack. This one at least makes a bit more sense, with a disk cache entry callback method in the stack.

crash/c83c6f0988000000 shows the crash in Chrome 60.3112, with the crash seeming to occur long after the heap was trashed.

This is also easily hit on ChromeOS (61.3159) although it doesn't seem to generate a crash report.

I can also reproduce in ToT on Windows: 

INVALID_POINTER_READ_base!base::subtle::RefCountedThreadSafeBase::ReleaseImpl+22

base!base::subtle::RefCountedThreadSafeBase::ReleaseImpl+0x22:
10042be2 8a5005          mov     dl,byte ptr [eax+5]        ds:002b:dddddde2=??
0:024> ~k
 # ChildEBP RetAddr  
00 30bfdc78 10042baf base!base::subtle::RefCountedThreadSafeBase::ReleaseImpl+0x22
01 30bfdc84 10047f08 base!base::subtle::RefCountedThreadSafeBase::Release+0xf
02 30bfdc98 10052302 base!base::RefCountedThreadSafe<base::internal::BindStateBase,base::internal::BindStateBaseRefCountTraits>::Release+0x18
03 30bfdca4 10051c9c base!scoped_refptr<base::internal::BindStateBase>::Release+0x12
04 30bfdcc0 10051c2d base!scoped_refptr<base::internal::BindStateBase>::operator=+0x4c
*** WARNING: Unable to verify checksum for C:\src\c\src\out\default\net.dll
05 30bfdcd8 03f99189 base!base::internal::CallbackBase<0>::Reset+0x1d
06 30bfded8 03f99471 net!net::ViewCacheHelper::DoCallback+0x259
07 30bfe0d0 03f98c9e net!net::ViewCacheHelper::HandleResult+0x231
08 30bfe724 03f9aacd net!net::ViewCacheHelper::DoLoop+0x97e
09 30bfe73c 03f9b7a2 net!net::ViewCacheHelper::OnIOComplete+0x1d
0a 30bfe764 03f9b699 net!base::internal::FunctorTraits<void (__thiscall net::ViewCacheHelper::*)(int),void>::Invoke<net::ViewCacheHelper *,int>+0x42
0b 30bfe798 03f9b5ff net!base::internal::InvokeHelper<0,void>::MakeItSo<void (__thiscall net::ViewCacheHelper::*const &)(int),net::ViewCacheHelper *,int>+0x69
0c 30bfe7c8 03f9b4b3 net!base::internal::Invoker<base::internal::BindState<void (__thiscall net::ViewCacheHelper::*)(int),base::internal::UnretainedWrapper<net::ViewCacheHelper> >,void __cdecl(int)>::RunImpl<void (__thiscall net::ViewCacheHelper::*const &)(int),std::tuple<base::internal::UnretainedWrapper<net::ViewCacheHelper> > const &,0>+0x6f
0d 30bfe7fc 030f960d net!base::internal::Invoker<base::internal::BindState<void (__thiscall net::ViewCacheHelper::*)(int),base::internal::UnretainedWrapper<net::ViewCacheHelper> >,void __cdecl(int)>::Run+0x53
0e 30bfe82c 03428424 net!base::Callback<void __cdecl(int),1,1>::Run+0x6d
0f 30bfe860 0342a1c1 net!disk_cache::InFlightBackendIO::OnOperationComplete+0xd4
10 30bfea98 03429d9d net!disk_cache::InFlightIO::InvokeCallback+0x3c1
11 30bfeb3c 0342fa24 net!disk_cache::BackgroundIO::OnIOSignalled+0xdd

This seems easier to repro in 32bit on Windows.

Open https://bayden.com/sandbox/gifbust/flood.asp in tab #1
Open chrome://cache in tab #2 and mash the refresh key

You are probably looking for a change made after 470970 (known good), but no later than 470979 (first known bad). CHANGELOG URL: https://chromium.googlesource.com/chromium/src/+log/2f0e211470e22f96aebc5357317a67ee4bc062a2..1852efa6e4d9c3353cd43d1815929d46e0f19a70

This looks like the culprit:
https://chromium.googlesource.com/chromium/src/+/7f2980a6dacefd6a9458ab717f51634d51b00890

Reverting the addition of base::Unretained() inside view_http_cache_job_factory.cc resolves the crash.

https://chromium.googlesource.com/chromium/src/+/7f2980a6dacefd6a9458ab717f51634d51b00890%5E%21/#F0

Crashing scenarios are immediately preceded by a call to ViewHttpCacheJob::Kill() invoked by net::URLRequest::DoCancel invoked by content::MojoAsyncResourceHandler::Cancel.

jam@ -- can you please take or help find the right owner.
elawrence@ -- nice work!

Shouldn't be higher than low severity since it doesn't require too many user gestures?

Reasons for Low:
- It does require multiple user gestures.
- Does not happen consistently.
- Requires going to chrome:// page, which most users don't have any reason to go to.

Hmm sounds reasonable.

After a bunch of instrumentation, I think the issue is here:

 void ViewCacheHelper::DoCallback(int rv) {
  DCHECK_NE(ERR_IO_PENDING, rv);
  DCHECK(!callback_.is_null());
  callback_.Run(rv);         <--- Callback destroys this object
  callback_.Reset();	     <--- ... but it's still running
 }

ViewCacheHelper ::OnIOComplete() calls ::HandleResult() which calls ::DoCallback().

The DoCallback() method calls callback_.Run(). That invokes ViewHttpCacheJob::Core::OnIOComplete() which calls Release() on itself. Calling Release leads immediately to ViewHttpCacheJob::Core::~Destroy() in the case where Core::Orphan had been called previously. 

The destructor for the Core object results in invoking the destructor for the ViewCacheHelper, but (oops!) its DoCallback() method is still running! The Destructor finishes and the DoCallback() method, still running on the now destructed object, then calls callback_.Reset. This hits an AV in base!base::subtle::RefCountedThreadSafeBase::ReleaseImpl+0x22.

sorry just saw this, looking..

btw this is in M60. I think this is pretty low risk, so no need to merge it to stable. But we should probably merge it to M61.

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

commit 7055d46ff097dedbb75bafc5e8880f87e3fcfb7a
Author: John Abd-El-Malek <jam@chromium.org>
Date: Fri Jul 28 16:05:39 2017

Fix UaF in ViewHttpCacheJob::Core.

This was introduced in r470978 which fixed the memory leak of the object never being deleted. Now
that it can be destructed, a UaF was exposed when ViewHttpCacheJob::Core::OnIOComplete deletes the
last reference to the object. It would delete the net::ViewCacheHelper member variable which is
what was calling OnIOComplete. The fix is to release the reference asynchronously.

BUG= 748362 

Change-Id: I40c160661eac24c607ac58448429ac2444875a06
Reviewed-on: https://chromium-review.googlesource.com/590709
Reviewed-by: Eric Lawrence <elawrence@chromium.org>
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#490415}
[modify] https://crrev.com/7055d46ff097dedbb75bafc5e8880f87e3fcfb7a/content/browser/net/view_http_cache_job_factory.cc

Approving merge to M61 Chrome OS.

+ awhalley@ (Security TPM) who mostly handles Security merge reviews and approvals.

ketakid@, could you please let us know for approving this merge to M61 for Chrome OS at #25?

Confirming merge is good for 61.

Ok to confirm, this affects all OSs. I'll merge this to M61

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

commit 1c24bdb489689cbd0adff00d8b9e24654c9c6770
Author: John Abd-El-Malek <jam@chromium.org>
Date: Mon Jul 31 21:24:33 2017

Fix UaF in ViewHttpCacheJob::Core.

This was introduced in r470978 which fixed the memory leak of the object never being deleted. Now
that it can be destructed, a UaF was exposed when ViewHttpCacheJob::Core::OnIOComplete deletes the
last reference to the object. It would delete the net::ViewCacheHelper member variable which is
what was calling OnIOComplete. The fix is to release the reference asynchronously.

BUG= 748362 
TBR=jam@chromium.org

(cherry picked from commit 7055d46ff097dedbb75bafc5e8880f87e3fcfb7a)

Change-Id: I40c160661eac24c607ac58448429ac2444875a06
Reviewed-on: https://chromium-review.googlesource.com/590709
Reviewed-by: Eric Lawrence <elawrence@chromium.org>
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#490415}
Reviewed-on: https://chromium-review.googlesource.com/594940
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#184}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/1c24bdb489689cbd0adff00d8b9e24654c9c6770/content/browser/net/view_http_cache_job_factory.cc

Verified on 62.0.3173.0 on Mac and Windows, this seems like fixed. Thanks John for the quick fix.

No reward for this one?

The panel will have a look.

On the plus side: It's a UAF in the browser process (!!!)
On the minus side: It requires an unlikely set of manual user actions.

Hi - the VRP panel look at look at this and declined to award, I'm afraid, but they said they'd reconsider if it can be demonstrated how this could be exploited by an attacker.

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