Filter out credentials with non-matching schemes

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/da597b2f777ea4ac768164bee01f39a90fba3c65
Cr-Commit-Position: refs/heads/master@{#415622}

[vabr (Chromium), 2016-08-31 10:53:10]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-31 10:53:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2016-08-31 11:42:58]

vabr@chromium.org changed reviewers:
+ mkwst@chromium.org

[vabr (Chromium), 2016-08-31 11:42:59]

Hi Mike,

Does the security argument in the CL description about not mixing credential
schemes sound correct?

(If you are looking at this, feel free to also review, but I can find another
reviewer if you are too busy for that.)

Cheers,
Vaclav

[commit-bot: I haz the power, 2016-08-31 11:54:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-31 11:54:10]

Dry run: This issue passed the CQ dry run.

[Mike West, 2016-08-31 12:15:31]

On 2016/08/31 at 11:42:59, vabr wrote:
> Hi Mike,
> 
> Does the security argument in the CL description about not mixing credential
schemes sound correct?

When is this check happening? I thought we allowed HTTP passwords to be filled
into HTTPS forms with user interaction (e.g. click on the form element, then
click on the username), for example. We certainly shouldn't be going in the
opposite direction, of course.

> (If you are looking at this, feel free to also review, but I can find another
reviewer if you are too busy for that.)

Code change looks like it does what you say it does, but it would be helpful if
someone who knew whether what you say it does is what it should do took a look.
:)

[vabr (Chromium), 2016-08-31 13:02:26]

On 2016/08/31 12:15:31, Mike West (OOO until 29th) wrote:
> On 2016/08/31 at 11:42:59, vabr wrote:
> > Hi Mike,
> > 
> > Does the security argument in the CL description about not mixing credential
> schemes sound correct?
> 
> When is this check happening? I thought we allowed HTTP passwords to be filled
> into HTTPS forms with user interaction (e.g. click on the form element, then
> click on the username), for example. We certainly shouldn't be going in the
> opposite direction, of course.
Currently we don't cross the HTTP/HTTPS border in any way.
However, note that this is not about HTTP vs. HTTPS, it is about HTML forms vs.
HTTP basic/digest auth.
In other words the scheme in question is not the URL scheme, but the
PasswordForm::Scheme
(I acknowledge that the latter should have been named differently).

> 
> > (If you are looking at this, feel free to also review, but I can find
another
> reviewer if you are too busy for that.)
> 
> Code change looks like it does what you say it does, but it would be helpful
if
> someone who knew whether what you say it does is what it should do took a
look.
> :)

Sure, I'll ask Vadym for review.

Cheers,
Vaclav

[vabr (Chromium), 2016-08-31 13:03:05]

vabr@chromium.org changed reviewers:
+ dvadym@chromium.org

[vabr (Chromium), 2016-08-31 13:03:05]

Hi Vadym!

I would appreciate your review of this CL.

Thanks!
Vaclav

[Mike West, 2016-08-31 13:07:08]

On 2016/08/31 at 13:02:26, vabr wrote:
> On 2016/08/31 12:15:31, Mike West (OOO until 29th) wrote:
> > On 2016/08/31 at 11:42:59, vabr wrote:
> > > Hi Mike,
> > > 
> > > Does the security argument in the CL description about not mixing
credential
> > schemes sound correct?
> > 
> > When is this check happening? I thought we allowed HTTP passwords to be
filled
> > into HTTPS forms with user interaction (e.g. click on the form element, then
> > click on the username), for example. We certainly shouldn't be going in the
> > opposite direction, of course.
> Currently we don't cross the HTTP/HTTPS border in any way.
> However, note that this is not about HTTP vs. HTTPS, it is about HTML forms
vs.
> HTTP basic/digest auth.
> In other words the scheme in question is not the URL scheme, but the
PasswordForm::Scheme
> (I acknowledge that the latter should have been named differently).

In that case, LGTM. :)

[dvadym, 2016-08-31 13:27:30]

This code is LGTM.
Acctually this filtering is needed only when non-database store is used (i.e.
for Linux backends). In Login database only credentials with the same scheme are
fetched, since a regular expression in SQL for fetching credentials contain a
scheme of observed credentials.
https://cs.chromium.org/chromium/src/components/password_manager/core/browser...
 

So removing non-db backends will fix this issue automatically.

[vabr (Chromium), 2016-08-31 13:44:42]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2016-08-31 13:44:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2016-08-31 13:45:18]

Thanks both of you for the review, and also thank for pointing our that the
Login DB is safe against this!

Cheers,
Vaclav

[commit-bot: I haz the power, 2016-08-31 13:49:19]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-08-31 13:52:54]

Description was changed from

==========
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

==========
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/da597b2f777ea4ac768164bee01f39a90fba3c65
Cr-Commit-Position: refs/heads/master@{#415622}
==========

[commit-bot: I haz the power, 2016-08-31 13:52:55]

Patchset 1 (id:??) landed as
https://crrev.com/da597b2f777ea4ac768164bee01f39a90fba3c65
Cr-Commit-Position: refs/heads/master@{#415622}

[fgorski, 2016-08-31 16:17:05]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2298263002/ by fgorski@chromium.org.

The reason for reverting is: Patch is crashing tests on windows bots. 

https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2...

https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2....

[vabr (Chromium), 2016-09-01 07:53:21]

On 2016/08/31 13:27:30, dvadym wrote:
> This code is LGTM.
> Acctually this filtering is needed only when non-database store is used (i.e.
> for Linux backends). In Login database only credentials with the same scheme
are
> fetched, since a regular expression in SQL for fetching credentials contain a
> scheme of observed credentials.
>
https://cs.chromium.org/chromium/src/components/password_manager/core/browser...

BTW, this issue does affect Login Database. The scheme checked in the referenced
code is about the URL scheme (which is also checked in the backends), whereas
this issue is about the PasswordForm::Scheme. It led twice to confusion on this
CL, I start thinking we should rename PasswordForm::Scheme. It might be tricky,
though, because it might have entered some protobuf definitions used by sync.

Cheers,
Vaclav