Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2306513002:
Reland "Filter out credentials with non-matching schemes" (Closed)
|
Created:
4 years, 3 months ago by vabr (Chromium) Modified:
4 years, 3 months ago Reviewers:
dvadym CC:
chromium-reviews, gcasto+watchlist_chromium.org, vabr+watchlistpasswordmanager_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReland "Filter out credentials with non-matching schemes"
The original CL: https://codereview.chromium.org/2298733002
The revert: https://codereview.chromium.org/2298263002
The original CL caused failures on Win7 debug bots, which are not
present in the CQ trybots set. The issue was genuine: the newly
introduced code modified a vector and invalidated some iterators
used later. The patch set 1 here is the reverted change, patch sets
2+ are fixes.
The fix here also replaces "non-relevant" with "irrelevant", to improve
the language, and does some further restructuring of the code based on reviewer
feedback from dvadym@.
Original description:
Filter out credentials with non-matching schemes
PasswordFormManager::ProcessMatches currently happily accepts credentials from
PasswordStore with a different PasswordForm::Scheme than the observed form has.
However, it still has a DCHECK against it later (in the Autofill* methods), so
it is clearly not expecting these, rather than mixing the schemes being by
design.
And it should not be by design. Especially, if the saved credential is a
non-HTML one, and should be filled in a HTML form. Mixing them makes the
non-HTML credential vulnerable against (injected attacker's) JavaScript
accessing them.
This CL filters out credentials with non-matching scheme from the batch coming
from the PasswordStore. Given the absence of DCHECKs in release builds, this
actually changes the behaviour for Chrome users, but the change is a desired
one.
BUG=640897
Committed: https://crrev.com/639b09a35f402f1e49e0a9ef7cb72705abd96421
Cr-Commit-Position: refs/heads/master@{#416536}
Patch Set 1 : Reverted patch #Patch Set 2 : Fix #Patch Set 3 : Just rebased #Patch Set 4 : Restructured + more tests #
Total comments: 5
Messages
Total messages: 24 (14 generated)
The CQ bit was checked by vabr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
vabr@chromium.org changed reviewers: + dvadym@chromium.org
Hi Vadym, Please review this patch. I was able to reproduce the failures on my Win Debug build, and verified that the fix works. I summarised my findings in the CL description.
Description was changed from ========== Reland "Filter out credentials with non-matching schemes" The original CL: https://codereview.chromium.org/2298733002 The revert: https://codereview.chromium.org/2298263002 The original CL caused failures on Win7 debug bots, which are not present in the CQ trybots set. The issue was genuine: the newly introduced code modified a vector and invalidated some iterators used later. The patch set 1 here is the reverted change, patch sets 2+ are fixes. Original description: Filter out credentials with non-matching schemes PasswordFormManager::ProcessMatches currently happily accepts credentials from PasswordStore with a different PasswordForm::Scheme than the observed form has. However, it still has a DCHECK against it later (in the Autofill* methods), so it is clearly not expecting these, rather than mixing the schemes being by design. And it should not be by design. Especially, if the saved credential is a non-HTML one, and should be filled in a HTML form. Mixing them makes the non-HTML credential vulnerable against (injected attacker's) JavaScript accessing them. This CL filters out credentials with non-matching scheme from the batch coming from the PasswordStore. Given the absence of DCHECKs in release builds, this actually changes the behaviour for Chrome users, but the change is a desired one. BUG=640897 ========== to ========== Reland "Filter out credentials with non-matching schemes" The original CL: https://codereview.chromium.org/2298733002 The revert: https://codereview.chromium.org/2298263002 The original CL caused failures on Win7 debug bots, which are not present in the CQ trybots set. The issue was genuine: the newly introduced code modified a vector and invalidated some iterators used later. The patch set 1 here is the reverted change, patch sets 2+ are fixes. The fix here also replaces "non-relevant" with "irrelevant", to improve the language. Original description: Filter out credentials with non-matching schemes PasswordFormManager::ProcessMatches currently happily accepts credentials from PasswordStore with a different PasswordForm::Scheme than the observed form has. However, it still has a DCHECK against it later (in the Autofill* methods), so it is clearly not expecting these, rather than mixing the schemes being by design. And it should not be by design. Especially, if the saved credential is a non-HTML one, and should be filled in a HTML form. Mixing them makes the non-HTML credential vulnerable against (injected attacker's) JavaScript accessing them. This CL filters out credentials with non-matching scheme from the batch coming from the PasswordStore. Given the absence of DCHECKs in release builds, this actually changes the behaviour for Chrome users, but the change is a desired one. BUG=640897 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi Vadym, A gentle ping: could you please review the diff between patch sets 2 and 1? Thanks! Vaclav
The CQ bit was checked by vabr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Reland "Filter out credentials with non-matching schemes" The original CL: https://codereview.chromium.org/2298733002 The revert: https://codereview.chromium.org/2298263002 The original CL caused failures on Win7 debug bots, which are not present in the CQ trybots set. The issue was genuine: the newly introduced code modified a vector and invalidated some iterators used later. The patch set 1 here is the reverted change, patch sets 2+ are fixes. The fix here also replaces "non-relevant" with "irrelevant", to improve the language. Original description: Filter out credentials with non-matching schemes PasswordFormManager::ProcessMatches currently happily accepts credentials from PasswordStore with a different PasswordForm::Scheme than the observed form has. However, it still has a DCHECK against it later (in the Autofill* methods), so it is clearly not expecting these, rather than mixing the schemes being by design. And it should not be by design. Especially, if the saved credential is a non-HTML one, and should be filled in a HTML form. Mixing them makes the non-HTML credential vulnerable against (injected attacker's) JavaScript accessing them. This CL filters out credentials with non-matching scheme from the batch coming from the PasswordStore. Given the absence of DCHECKs in release builds, this actually changes the behaviour for Chrome users, but the change is a desired one. BUG=640897 ========== to ========== Reland "Filter out credentials with non-matching schemes" The original CL: https://codereview.chromium.org/2298733002 The revert: https://codereview.chromium.org/2298263002 The original CL caused failures on Win7 debug bots, which are not present in the CQ trybots set. The issue was genuine: the newly introduced code modified a vector and invalidated some iterators used later. The patch set 1 here is the reverted change, patch sets 2+ are fixes. The fix here also replaces "non-relevant" with "irrelevant", to improve the language, and does some further restructuring of the code based on reviewer feedback from dvadym@. Original description: Filter out credentials with non-matching schemes PasswordFormManager::ProcessMatches currently happily accepts credentials from PasswordStore with a different PasswordForm::Scheme than the observed form has. However, it still has a DCHECK against it later (in the Autofill* methods), so it is clearly not expecting these, rather than mixing the schemes being by design. And it should not be by design. Especially, if the saved credential is a non-HTML one, and should be filled in a HTML form. Mixing them makes the non-HTML credential vulnerable against (injected attacker's) JavaScript accessing them. This CL filters out credentials with non-matching scheme from the batch coming from the PasswordStore. Given the absence of DCHECKs in release builds, this actually changes the behaviour for Chrome users, but the change is a desired one. BUG=640897 ==========
Hi Vadym, I tried to address your comments from our offline discussion. Please review the current patch set completely, not just as a diff, as the changes are now significant. Thanks! Vaclav
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM, thanks Vaclav for refactoring this, now it looks much better! https://codereview.chromium.org/2306513002/diff/60001/components/password_man... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/2306513002/diff/60001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:482: [this](const PasswordForm* match) { return ScoreResult(*match); }); Great, I like this STL style is better that it was previously with loops. https://codereview.chromium.org/2306513002/diff/60001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:1184: blacklisted_form.origin.GetOrigin() != Acknowledgement, removing comparison of submit/username/password elements, they are mostly garbage
Thanks, Vadym for the review. Please see my response to your second comment below, there might be a slight misunderstsanding. I'll wait with landing before we clarify that. Cheers, Vaclav https://codereview.chromium.org/2306513002/diff/60001/components/password_man... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/2306513002/diff/60001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:482: [this](const PasswordForm* match) { return ScoreResult(*match); }); On 2016/09/05 10:58:05, dvadym wrote: > Great, I like this STL style is better that it was previously with loops. Thanks, I also find it better in the way it lets the specific code stand out in the lambda at the end. https://codereview.chromium.org/2306513002/diff/60001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:1184: blacklisted_form.origin.GetOrigin() != On 2016/09/05 10:58:05, dvadym wrote: > Acknowledgement, removing comparison of submit/username/password elements, they > are mostly garbage There might be a misunderstanding here. I don't think I removed any checks about elements (they should be still below). What I did here was turn the DCHECK into a real condition, before I wanted this method to handle that case now. I also added the PasswordForm::Scheme check and squashed all these early exits into one if-return statement.
https://codereview.chromium.org/2306513002/diff/60001/components/password_man... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/2306513002/diff/60001/components/password_man... components/password_manager/core/browser/password_form_manager.cc:1184: blacklisted_form.origin.GetOrigin() != On 2016/09/05 11:39:10, vabr (Chromium) wrote: > On 2016/09/05 10:58:05, dvadym wrote: > > Acknowledgement, removing comparison of submit/username/password elements, > they > > are mostly garbage > > There might be a misunderstanding here. I don't think I removed any checks about > elements (they should be still below). What I did here was turn the DCHECK into > a real condition, before I wanted this method to handle that case now. I also > added the PasswordForm::Scheme check and squashed all these early exits into one > if-return statement. Ah, sorry, I misinterpered changes. Acknowledge your changes.
On 2016/09/05 11:48:37, dvadym wrote: > https://codereview.chromium.org/2306513002/diff/60001/components/password_man... > File components/password_manager/core/browser/password_form_manager.cc (right): > > https://codereview.chromium.org/2306513002/diff/60001/components/password_man... > components/password_manager/core/browser/password_form_manager.cc:1184: > blacklisted_form.origin.GetOrigin() != > On 2016/09/05 11:39:10, vabr (Chromium) wrote: > > On 2016/09/05 10:58:05, dvadym wrote: > > > Acknowledgement, removing comparison of submit/username/password elements, > > they > > > are mostly garbage > > > > There might be a misunderstanding here. I don't think I removed any checks > about > > elements (they should be still below). What I did here was turn the DCHECK > into > > a real condition, before I wanted this method to handle that case now. I also > > added the PasswordForm::Scheme check and squashed all these early exits into > one > > if-return statement. > > Ah, sorry, I misinterpered changes. Acknowledge your changes. Thanks! Sending to CQ now. Cheers, Vaclav
The CQ bit was checked by vabr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Reland "Filter out credentials with non-matching schemes" The original CL: https://codereview.chromium.org/2298733002 The revert: https://codereview.chromium.org/2298263002 The original CL caused failures on Win7 debug bots, which are not present in the CQ trybots set. The issue was genuine: the newly introduced code modified a vector and invalidated some iterators used later. The patch set 1 here is the reverted change, patch sets 2+ are fixes. The fix here also replaces "non-relevant" with "irrelevant", to improve the language, and does some further restructuring of the code based on reviewer feedback from dvadym@. Original description: Filter out credentials with non-matching schemes PasswordFormManager::ProcessMatches currently happily accepts credentials from PasswordStore with a different PasswordForm::Scheme than the observed form has. However, it still has a DCHECK against it later (in the Autofill* methods), so it is clearly not expecting these, rather than mixing the schemes being by design. And it should not be by design. Especially, if the saved credential is a non-HTML one, and should be filled in a HTML form. Mixing them makes the non-HTML credential vulnerable against (injected attacker's) JavaScript accessing them. This CL filters out credentials with non-matching scheme from the batch coming from the PasswordStore. Given the absence of DCHECKs in release builds, this actually changes the behaviour for Chrome users, but the change is a desired one. BUG=640897 ========== to ========== Reland "Filter out credentials with non-matching schemes" The original CL: https://codereview.chromium.org/2298733002 The revert: https://codereview.chromium.org/2298263002 The original CL caused failures on Win7 debug bots, which are not present in the CQ trybots set. The issue was genuine: the newly introduced code modified a vector and invalidated some iterators used later. The patch set 1 here is the reverted change, patch sets 2+ are fixes. The fix here also replaces "non-relevant" with "irrelevant", to improve the language, and does some further restructuring of the code based on reviewer feedback from dvadym@. Original description: Filter out credentials with non-matching schemes PasswordFormManager::ProcessMatches currently happily accepts credentials from PasswordStore with a different PasswordForm::Scheme than the observed form has. However, it still has a DCHECK against it later (in the Autofill* methods), so it is clearly not expecting these, rather than mixing the schemes being by design. And it should not be by design. Especially, if the saved credential is a non-HTML one, and should be filled in a HTML form. Mixing them makes the non-HTML credential vulnerable against (injected attacker's) JavaScript accessing them. This CL filters out credentials with non-matching scheme from the batch coming from the PasswordStore. Given the absence of DCHECKs in release builds, this actually changes the behaviour for Chrome users, but the change is a desired one. BUG=640897 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Reland "Filter out credentials with non-matching schemes" The original CL: https://codereview.chromium.org/2298733002 The revert: https://codereview.chromium.org/2298263002 The original CL caused failures on Win7 debug bots, which are not present in the CQ trybots set. The issue was genuine: the newly introduced code modified a vector and invalidated some iterators used later. The patch set 1 here is the reverted change, patch sets 2+ are fixes. The fix here also replaces "non-relevant" with "irrelevant", to improve the language, and does some further restructuring of the code based on reviewer feedback from dvadym@. Original description: Filter out credentials with non-matching schemes PasswordFormManager::ProcessMatches currently happily accepts credentials from PasswordStore with a different PasswordForm::Scheme than the observed form has. However, it still has a DCHECK against it later (in the Autofill* methods), so it is clearly not expecting these, rather than mixing the schemes being by design. And it should not be by design. Especially, if the saved credential is a non-HTML one, and should be filled in a HTML form. Mixing them makes the non-HTML credential vulnerable against (injected attacker's) JavaScript accessing them. This CL filters out credentials with non-matching scheme from the batch coming from the PasswordStore. Given the absence of DCHECKs in release builds, this actually changes the behaviour for Chrome users, but the change is a desired one. BUG=640897 ========== to ========== Reland "Filter out credentials with non-matching schemes" The original CL: https://codereview.chromium.org/2298733002 The revert: https://codereview.chromium.org/2298263002 The original CL caused failures on Win7 debug bots, which are not present in the CQ trybots set. The issue was genuine: the newly introduced code modified a vector and invalidated some iterators used later. The patch set 1 here is the reverted change, patch sets 2+ are fixes. The fix here also replaces "non-relevant" with "irrelevant", to improve the language, and does some further restructuring of the code based on reviewer feedback from dvadym@. Original description: Filter out credentials with non-matching schemes PasswordFormManager::ProcessMatches currently happily accepts credentials from PasswordStore with a different PasswordForm::Scheme than the observed form has. However, it still has a DCHECK against it later (in the Autofill* methods), so it is clearly not expecting these, rather than mixing the schemes being by design. And it should not be by design. Especially, if the saved credential is a non-HTML one, and should be filled in a HTML form. Mixing them makes the non-HTML credential vulnerable against (injected attacker's) JavaScript accessing them. This CL filters out credentials with non-matching scheme from the batch coming from the PasswordStore. Given the absence of DCHECKs in release builds, this actually changes the behaviour for Chrome users, but the change is a desired one. BUG=640897 Committed: https://crrev.com/639b09a35f402f1e49e0a9ef7cb72705abd96421 Cr-Commit-Position: refs/heads/master@{#416536} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/639b09a35f402f1e49e0a9ef7cb72705abd96421 Cr-Commit-Position: refs/heads/master@{#416536} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
