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

Issue 817754 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 21
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Blacklisted password entries contain PII and returned via CM API

Project Member Reported by vasi...@chromium.org, Mar 1 2018

Issue description

Chrome Version: 66
OS: all

Summary from another reporter:

>>>>>
1) Generally, it's been my intention to not allow Chrome to store passwords. But I'm lazy, so I haven't disabled it.

2) Despite this general intention, Chrome somehow stored a password for kayak. It also stored it with the blacklisted bit, so it's not clear to me if a) I enabled password saving for kayak and it recorded my "username" and "password" and then I disabled it, or b) I told it to "not remember this login" and it correctly set the blacklisted bit, but also stored my login info.

(b) sounds more likely to me based on the fact that I generally disable login storing.

3) Despite being listed as blacklisted, Chrome tries to use my login info when I visit Kayak, saying "now logging in with [login]..."

4) That login just so happens to be my credit card number. There are two instances of chrome storing my credit card number as my username.

5) This login info is not being displayed correctly in chrome://settings/passwords or passwords.google.com. The sites are displayed as blacklisted despite having login info.
<<<<<<

Basically there are two bugs here
- A blacklisted entry appeared with non-empty username/password.
- The Credential Manager API uses the blacklisted entry as a normal credential
 
Cc: jshannon@google.com
Project Member

Comment 2 by bugdroid1@chromium.org, Mar 1 2018

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

commit 662c896bfeb4ccb094d9f93b69288b84e3cf4e9a
Author: Vasilii Sukhanov <vasilii@chromium.org>
Date: Thu Mar 01 13:19:30 2018

Don't return blacklisted credentials via CM API.

Previously navigator.credentials.get() would return a blacklisted credential like a normal password. It's a problem if due to another bug the username isn't empty.

Bug:  817754 
Change-Id: I51f79e8399f0566438ce203cb9338bd5e4dc5f0d
Reviewed-on: https://chromium-review.googlesource.com/942921
Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Maxim Kolosovskiy <kolos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540143}
[modify] https://crrev.com/662c896bfeb4ccb094d9f93b69288b84e3cf4e9a/components/password_manager/core/browser/credential_manager_impl_unittest.cc
[modify] https://crrev.com/662c896bfeb4ccb094d9f93b69288b84e3cf4e9a/components/password_manager/core/browser/credential_manager_pending_request_task.cc

I reproduced saving a blacklisted entry with username/password. It happens when it's prefilled when we see the form.
Project Member

Comment 4 by bugdroid1@chromium.org, Mar 1 2018

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

commit a3c2603f8a3af07b29b115c22c6d16f4bcb603a6
Author: Vasilii Sukhanov <vasilii@chromium.org>
Date: Thu Mar 01 17:38:49 2018

Don't save username/password when blacklisting a site.

It happens when the form is prefilled when detected by the password manager.

Bug:  817754 
Change-Id: Ia6c8ecd2257950aa34b3fedf796a9696c71cf4fb
Reviewed-on: https://chromium-review.googlesource.com/943503
Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540202}
[modify] https://crrev.com/a3c2603f8a3af07b29b115c22c6d16f4bcb603a6/components/password_manager/core/browser/form_saver_impl.cc
[modify] https://crrev.com/a3c2603f8a3af07b29b115c22c6d16f4bcb603a6/components/password_manager/core/browser/form_saver_impl_unittest.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Mar 8 2018

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

commit 10f23e8bdecef522d92505fd0b122ece45498949
Author: Vasilii Sukhanov <vasilii@chromium.org>
Date: Thu Mar 08 19:11:59 2018

Clean up username/password values in the blacklisted credentials.

When a user clicks "Never save password" no username/password should be recorded. Due to a bug when the page prefills some values there were stored. Now let's iterate the blacklisted credentials and clean those values.

Bug:  817754 
Change-Id: I68d811ef26d7d17ffd99031897bb01750b86f14b
Reviewed-on: https://chromium-review.googlesource.com/951607
Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541855}
[modify] https://crrev.com/10f23e8bdecef522d92505fd0b122ece45498949/chrome/browser/password_manager/password_store_factory.cc
[modify] https://crrev.com/10f23e8bdecef522d92505fd0b122ece45498949/components/password_manager/core/browser/password_manager.cc
[modify] https://crrev.com/10f23e8bdecef522d92505fd0b122ece45498949/components/password_manager/core/browser/password_manager_util.cc
[modify] https://crrev.com/10f23e8bdecef522d92505fd0b122ece45498949/components/password_manager/core/browser/password_manager_util.h
[modify] https://crrev.com/10f23e8bdecef522d92505fd0b122ece45498949/components/password_manager/core/browser/password_manager_util_unittest.cc
[modify] https://crrev.com/10f23e8bdecef522d92505fd0b122ece45498949/components/password_manager/core/common/password_manager_pref_names.cc
[modify] https://crrev.com/10f23e8bdecef522d92505fd0b122ece45498949/components/password_manager/core/common/password_manager_pref_names.h
[modify] https://crrev.com/10f23e8bdecef522d92505fd0b122ece45498949/tools/metrics/histograms/histograms.xml

I keep the bug open because we need to remove r541855 after couple of releases.
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 21

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

commit 94dc43bf01e33143ac1737bfcd2d7304f22687a2
Author: Gemene Narcis <gemene@google.com>
Date: Tue Aug 21 07:29:06 2018

Remove cleaning username/password on the blacklisted credentials

The code is executed once per profile. As it was introduced 5 months ago,
and acording to metric reported users don't need it anymore.
It's a manual revert of https://crrev.com/c/951607/

Bug:  817754 
Change-Id: Iab2c5dfdb9effb4e2347f174faebeb67f9dd6a3e
Reviewed-on: https://chromium-review.googlesource.com/1179151
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Commit-Queue: Narcis Gemene <gemene@google.com>
Cr-Commit-Position: refs/heads/master@{#584675}
[modify] https://crrev.com/94dc43bf01e33143ac1737bfcd2d7304f22687a2/chrome/browser/password_manager/password_store_factory.cc
[modify] https://crrev.com/94dc43bf01e33143ac1737bfcd2d7304f22687a2/components/password_manager/core/browser/password_manager.cc
[modify] https://crrev.com/94dc43bf01e33143ac1737bfcd2d7304f22687a2/components/password_manager/core/browser/password_manager_util.cc
[modify] https://crrev.com/94dc43bf01e33143ac1737bfcd2d7304f22687a2/components/password_manager/core/browser/password_manager_util.h
[modify] https://crrev.com/94dc43bf01e33143ac1737bfcd2d7304f22687a2/components/password_manager/core/browser/password_manager_util_unittest.cc
[modify] https://crrev.com/94dc43bf01e33143ac1737bfcd2d7304f22687a2/components/password_manager/core/common/password_manager_pref_names.cc
[modify] https://crrev.com/94dc43bf01e33143ac1737bfcd2d7304f22687a2/components/password_manager/core/common/password_manager_pref_names.h
[modify] https://crrev.com/94dc43bf01e33143ac1737bfcd2d7304f22687a2/tools/metrics/histograms/histograms.xml

Status: Fixed (was: Assigned)

Sign in to add a comment