This issue is a security regression. If you are not able to fix this quickly, please revert the change that introduced it.

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

vabr: Could you take a look? Potentially related to https://chromium.googlesource.com/chromium/src/+/daecd125374ae8d6526b58e56e0429550f78aca4

Sanitizers, I love you! I was just trying to figure out what is causing bug 675587, when LSAN has already done the analysis for me. These tools are awesome! Jumping right on this.

This is easily reproducible on my GNU/Linux machine with ASAN/LSAN using the minimised testcase from clusterfuzz; just save test.html and open in the browser. ASAN output attached. Will start fixing this as soon as I grab breakfast.

Deleted: test.html 112 bytes

test.html 112 bytes View Download

Deleted: result.txt 12.7 KB

result.txt 12.7 KB View Download

Users experienced this crash on the following builds:

Android Dev 57.0.2950.3 -  0.58 CPM, 22 reports, 6 clients (signature password_manager::FormFetcherImpl::OnGetPasswordStoreResults)

If this update was incorrect, please add "Fracas-Wrong" label to prevent future updates.

- Go/Fracas

The cause of the crash:
When FormFetcherImpl passes the obtained credentials from the store to CredentialManagerPasswordFormManager (CMPFM, a child of PasswordFormManager), the CMPFM hands over to CredentialManagerImpl, which takes the CMPFM and passes it to the UI code, which destroys it (because it decides the UI should not be shown). This destruction destroys the FormFetcherImpl, to which the control is returned nevertheless. It tries to continue iterating over the set which is its data-member, and hits the use-after-free by doing that.

It seems to me that the direct call to delegate_->OnProvisionalSaveComplete(); in CredentialManagerPasswordFormManager::ProcessMatches is a bad idea, as long as the delegate is capable of destroying the CMPFM (and the FormFetcherImpl it owns). So I plan to defer that call by posting a task instead.

https://codereview.chromium.org/2592653003/ is the fix. I'll send it out for  review soon.

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

commit 7d5047855d221120c1e79e0524f9b369327350d2
Author: vabr <vabr@chromium.org>
Date: Wed Dec 21 15:24:52 2016

Avoid use-after-free in FormFetcherImpl

Currently, FormFetcherImpl can get deleted during processing results from the
PasswordStore, because the credential manager core code which owns it can hand
itself off to UI code and get dropped. Detailed description of the
use-after-free is in  https://crbug.com/675178#c8 .

This CL fixes this issue inside CredentialManagerPasswordFormManager by not
letting itself call a potentially self-deleting method inside
CredentialManagerPasswordFormManager::ProcessMatches.

BUG= 675178 , 621355 

Review-Url: https://codereview.chromium.org/2592653003
Cr-Commit-Position: refs/heads/master@{#440107}

[modify] https://crrev.com/7d5047855d221120c1e79e0524f9b369327350d2/components/password_manager/content/browser/credential_manager_impl.cc
[modify] https://crrev.com/7d5047855d221120c1e79e0524f9b369327350d2/components/password_manager/core/browser/BUILD.gn
[modify] https://crrev.com/7d5047855d221120c1e79e0524f9b369327350d2/components/password_manager/core/browser/credential_manager_password_form_manager.cc
[modify] https://crrev.com/7d5047855d221120c1e79e0524f9b369327350d2/components/password_manager/core/browser/credential_manager_password_form_manager.h
[add] https://crrev.com/7d5047855d221120c1e79e0524f9b369327350d2/components/password_manager/core/browser/credential_manager_password_form_manager_unittest.cc
[modify] https://crrev.com/7d5047855d221120c1e79e0524f9b369327350d2/ios/chrome/browser/passwords/credential_manager.mm

Marking this as fixed, because I could reproduce the ASAN testcase before the above r440107 and not after it.
Both the culprit r436571 and the fix r440107 landed in 57, so no merge needed.

ClusterFuzz has detected this issue as fixed in range 440099:440242.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=6111806453186560

Fuzzer: inferno_layout_test_unmodified
Job Type: linux_lsan_chrome_mp
Platform Id: linux

Crash Type: Heap-use-after-free READ 8
Crash Address: 0x6040004572d8
Crash State:
  password_manager::FormFetcherImpl::OnGetPasswordStoreResults
  void base::internal::FunctorTraits<void
  MakeItSo<void
  
Recommended Security Severity: Critical

Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_lsan_chrome_mp&range=436564:436579
Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_lsan_chrome_mp&range=440099:440242

Minimized Testcase (0.11 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv956Y6tF6wNZLDQUfOsLkrEniwueIwoDZdTsmUq0aBeW89RtBaDhcr8Es5pmaskoVC4vj4wCNC8tFQ_vevEx6MsnFCrz-mDplpBjxPHLs1fsjSvrFGjCrcjgJkNeZcuVmfWfBNLjgwJF8-t7lokOpXkjYz57HA?testcase_id=6111806453186560
<script>
navigator.credentials.store(new PasswordCredential({'id': 'name', 'password': 'password' }))
</script>


See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

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