New issue
Advanced search Search tips

Issue 757111 link

Starred by 1 user
Status: Fixed
Owner:
nparker@chromium.org
Closed: Oct 2017
Cc:
dvadym@chromium.org
vakh@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Password-matching catches shortest rather than all

Project Member Reported by nparker@chromium.org, Aug 18 2017 
Two related issues:
1) Common suffix: For password-reuse logging, it appears that if I have two passwords saved like this:

   mypassword
foomypassword

and I type "foomypassword", it only logs the first one.  We should find all matching passwords given the current character buffer.  I think this only affects the |password_matching_domains| proto field, and the count of matching passwords.

2) Common prefix: If I have these saved:

mypassword
mypassword123

and I type "mypassword123", it'll log "mypassword" since it only triggers a reuse event once per page -- the second event will be dropped.  This means if "mypassword123" is my sync password, we'll generate a saved-password ping first and Google won't know that the user typed their sync password.

+dvadym in case you've thought of solutions to these.

Comment 1 by vakh@chromium.org, Sep 1 2017

Labels: SafeBrowsing-Triaged

Comment 2 by nparker@chromium.org, Sep 1 2017

Status: Started (was: Assigned)
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 9 2017 

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

commit 2666acce35773870bf8d393a861d579b463584b6
Author: Nathan Parker <nparker@chromium.org>
Date: Sat Sep 09 00:19:41 2017

Log "sync && saved" password reuse events, and multiple passwords.

This will always check for sync-reuse AND saved-password reuse rather
that short-circuiting if sync-reuse is found. Also, for saved
passwords, it finds all matching passwords rather than just the
longest -- this makes the matching_origins field more complete.

Bug:  757111 
Change-Id: I6bfbca664277a4bed849bdf92bb82a687b874686
Reviewed-on: https://chromium-review.googlesource.com/653923
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Commit-Queue: Nathan Parker <nparker@chromium.org>
Cr-Commit-Position: refs/heads/master@{#500753}
[modify] https://crrev.com/2666acce35773870bf8d393a861d579b463584b6/components/password_manager/core/browser/password_manager_test_utils.h
[modify] https://crrev.com/2666acce35773870bf8d393a861d579b463584b6/components/password_manager/core/browser/password_reuse_detection_manager.cc
[modify] https://crrev.com/2666acce35773870bf8d393a861d579b463584b6/components/password_manager/core/browser/password_reuse_detection_manager.h
[modify] https://crrev.com/2666acce35773870bf8d393a861d579b463584b6/components/password_manager/core/browser/password_reuse_detection_manager_unittest.cc
[modify] https://crrev.com/2666acce35773870bf8d393a861d579b463584b6/components/password_manager/core/browser/password_reuse_detector.cc
[modify] https://crrev.com/2666acce35773870bf8d393a861d579b463584b6/components/password_manager/core/browser/password_reuse_detector.h
[modify] https://crrev.com/2666acce35773870bf8d393a861d579b463584b6/components/password_manager/core/browser/password_reuse_detector_consumer.h
[modify] https://crrev.com/2666acce35773870bf8d393a861d579b463584b6/components/password_manager/core/browser/password_reuse_detector_unittest.cc
[modify] https://crrev.com/2666acce35773870bf8d393a861d579b463584b6/components/password_manager/core/browser/password_store.cc
[modify] https://crrev.com/2666acce35773870bf8d393a861d579b463584b6/components/password_manager/core/browser/password_store.h
[modify] https://crrev.com/2666acce35773870bf8d393a861d579b463584b6/components/password_manager/core/browser/password_store_unittest.cc
[modify] https://crrev.com/2666acce35773870bf8d393a861d579b463584b6/tools/metrics/histograms/histograms.xml

Comment 4 by nparker@chromium.org, Sep 18 2017

Labels: Merge-Request-62
Requesting a merge -- this fixes a data quality issue on reports, and is not visible to users.
Project Member

Comment 5 by sheriffbot@chromium.org, Sep 18 2017

Labels: -Merge-Request-62 Merge-Review-62 Hotlist-Merge-Review
This bug requires manual review: M62 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

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

Comment 6 by abdulsyed@google.com, Sep 18 2017

Labels: -Merge-Review-62 Merge-Approved-62
Approving merge to M62. Branch:3202
Project Member

Comment 7 by bugdroid1@chromium.org, Sep 18 2017

Labels: -merge-approved-62 merge-merged-3202
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/31742905ff14ef69c81555969aedd62f2151eecd

commit 31742905ff14ef69c81555969aedd62f2151eecd
Author: Nathan Parker <nparker@chromium.org>
Date: Mon Sep 18 22:38:11 2017

[Merge M62] Log "sync && saved" password reuse events, and multiple passwords.

This will always check for sync-reuse AND saved-password reuse rather
that short-circuiting if sync-reuse is found. Also, for saved
passwords, it finds all matching passwords rather than just the
longest -- this makes the matching_origins field more complete.

TBR=nparker@chromium.org

(cherry picked from commit 2666acce35773870bf8d393a861d579b463584b6)

Bug:  757111 
Change-Id: I6bfbca664277a4bed849bdf92bb82a687b874686
Reviewed-on: https://chromium-review.googlesource.com/653923
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Commit-Queue: Nathan Parker <nparker@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#500753}
Reviewed-on: https://chromium-review.googlesource.com/671531
Reviewed-by: Nathan Parker <nparker@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#312}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/31742905ff14ef69c81555969aedd62f2151eecd/components/password_manager/core/browser/password_manager_test_utils.h
[modify] https://crrev.com/31742905ff14ef69c81555969aedd62f2151eecd/components/password_manager/core/browser/password_reuse_detection_manager.cc
[modify] https://crrev.com/31742905ff14ef69c81555969aedd62f2151eecd/components/password_manager/core/browser/password_reuse_detection_manager.h
[modify] https://crrev.com/31742905ff14ef69c81555969aedd62f2151eecd/components/password_manager/core/browser/password_reuse_detection_manager_unittest.cc
[modify] https://crrev.com/31742905ff14ef69c81555969aedd62f2151eecd/components/password_manager/core/browser/password_reuse_detector.cc
[modify] https://crrev.com/31742905ff14ef69c81555969aedd62f2151eecd/components/password_manager/core/browser/password_reuse_detector.h
[modify] https://crrev.com/31742905ff14ef69c81555969aedd62f2151eecd/components/password_manager/core/browser/password_reuse_detector_consumer.h
[modify] https://crrev.com/31742905ff14ef69c81555969aedd62f2151eecd/components/password_manager/core/browser/password_reuse_detector_unittest.cc
[modify] https://crrev.com/31742905ff14ef69c81555969aedd62f2151eecd/components/password_manager/core/browser/password_store.cc
[modify] https://crrev.com/31742905ff14ef69c81555969aedd62f2151eecd/components/password_manager/core/browser/password_store.h
[modify] https://crrev.com/31742905ff14ef69c81555969aedd62f2151eecd/components/password_manager/core/browser/password_store_unittest.cc
[modify] https://crrev.com/31742905ff14ef69c81555969aedd62f2151eecd/tools/metrics/histograms/histograms.xml

Comment 8 by nparker@chromium.org, Oct 2 2017

Status: Fixed (was: Started)
Some follow up work will happen in https://crbug.com/770725
►

Sign in to add a comment