Don't use FormFieldData only as a key in a map

Don't use FormFieldData only as a key in a map

The FormFieldData was used in PasswordAutofillManagera::LoginToPasswordInfoMap merely as a map key, without needing the information in it. This CL changes the key type to int, and removes one IPC message completely in result.

Note: this CL is also relevant to http://crbug.com/377422 and http://crbug.com/118601, but does not directly contribute to their goals.

BUG=400186
Committed: https://crrev.com/d73e4141cbcb4663862c02f11935b5cda79d6af0
Cr-Commit-Position: refs/heads/master@{#302790}

[vabr (Chromium), 2014-10-31 12:32:04]

Patchset #1 (id:1) has been deleted

[vabr (Chromium), 2014-10-31 12:32:20]

vabr@chromium.org changed reviewers:
+ gcasto@chromium.org, tsepez@chromium.org

[vabr (Chromium), 2014-10-31 12:33:33]

Hi Garret,

Could you please take a look at the whole CL, a follow-up to yesterday's
https://codereview.chromium.org/688633004/?

Hi Tom,

Could you please check components/autofill/content/common/autofill_messages.h
for this CL as well?

Thanks!
Vaclav

[Tom Sepez, 2014-10-31 16:47:17]

My only concern when making a change like this is whether the integer key, being
sequential and hence easily guessable, might allow some other principal (in this
case, lets say some unrelated renderer) access to information it wouldn't
already have.

I don't think this is the case, but please confirm.
Otherwise LGTM.

[vabr (Chromium), 2014-10-31 17:48:20]

On 2014/10/31 16:47:17, Tom Sepez wrote:
> My only concern when making a change like this is whether the integer key,
being
> sequential and hence easily guessable, might allow some other principal (in
this
> case, lets say some unrelated renderer) access to information it wouldn't
> already have.
> 
> I don't think this is the case, but please confirm.
> Otherwise LGTM.

Thanks, Tom.

As for AutofillHostMsg_ShowPasswordSuggestions, sent from the renderer to the
browse: If the sent key is invalid, i.e., there is no corresponding password
information for that key in the browser, the browser will crash by dereferencing
an end() iterator of a map. If the key is valid, the browser will show the user
a pop-up with the credential suggestions corresponding to the key, and if the
user accepts them, fill them in the renderer corresponding to the tab where the
user accepted the suggestion. In particular, sending this IPC from a different
renderer should not bring the sender any information.

As for AutofillMsg_FillPasswordForm, sent from the browser to the renderer:
sending a different key than which is associated with the password information
(also sent in this message) in PasswordAutofillManager will cause the
(uncompromised) renderer send a wrong key back via the above message later,
resulting in a crash or wrong credentials displayed.

Let me know if you are concerned and want me to change something.

Cheers,
Vaclav

[vabr (Chromium), 2014-10-31 17:57:50]

On 2014/10/31 17:48:20, vabr (Chromium) wrote:
> On 2014/10/31 16:47:17, Tom Sepez wrote:
> > My only concern when making a change like this is whether the integer key,
> being
> > sequential and hence easily guessable, might allow some other principal (in
> this
> > case, lets say some unrelated renderer) access to information it wouldn't
> > already have.
> > 
> > I don't think this is the case, but please confirm.
> > Otherwise LGTM.
> 
> Thanks, Tom.
> 
> As for AutofillHostMsg_ShowPasswordSuggestions, sent from the renderer to the
> browse: If the sent key is invalid, i.e., there is no corresponding password
> information for that key in the browser, the browser will crash by
dereferencing
> an end() iterator of a map. If the key is valid, the browser will show the
user
> a pop-up with the credential suggestions corresponding to the key, and if the
> user accepts them, fill them in the renderer corresponding to the tab where
the
> user accepted the suggestion. In particular, sending this IPC from a different
> renderer should not bring the sender any information.

I should also add, that this situation is not new, and the same was possible by
spoofing the autofill::FormFieldData argument before.

> 
> As for AutofillMsg_FillPasswordForm, sent from the browser to the renderer:
> sending a different key than which is associated with the password information
> (also sent in this message) in PasswordAutofillManager will cause the
> (uncompromised) renderer send a wrong key back via the above message later,
> resulting in a crash or wrong credentials displayed.
> 
> Let me know if you are concerned and want me to change something.
> 
> Cheers,
> Vaclav

[Tom Sepez, 2014-10-31 23:40:59]

Doesn't sound like there's a problem then. LGTM.

[Garrett Casto, 2014-11-04 07:22:09]

lgtm

https://codereview.chromium.org/686653003/diff/20001/components/autofill/cont...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/686653003/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_autofill_agent.cc:1078:
username_element, &form, &field, REQUIRE_NONE);
This is unneeded, right?

[vabr (Chromium), 2014-11-04 13:43:51]

On 2014/11/04 07:22:09, Garrett Casto wrote:
> lgtm
> 
>
https://codereview.chromium.org/686653003/diff/20001/components/autofill/cont...
> File components/autofill/content/renderer/password_autofill_agent.cc (right):
> 
>
https://codereview.chromium.org/686653003/diff/20001/components/autofill/cont...
> components/autofill/content/renderer/password_autofill_agent.cc:1078:
> username_element, &form, &field, REQUIRE_NONE);
> This is unneeded, right?

Thanks!

I will address the comment and land the CL soon, currently I'm waiting for
re-landing https://codereview.chromium.org/688633004 (to avoid rebasing), which
had some crashing troubles http://crbug.com/429607#c8.

Cheers,
Vaclav

[vabr (Chromium), 2014-11-05 11:56:19]

Thanks, Garrett.

I addressed your comment.
Once https://codereview.chromium.org/701073002/ lands, I will rebase this CL on
top of it, and submit.

Cheers,
Vaclav

https://codereview.chromium.org/686653003/diff/20001/components/autofill/cont...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/686653003/diff/20001/components/autofill/cont...
components/autofill/content/renderer/password_autofill_agent.cc:1078:
username_element, &form, &field, REQUIRE_NONE);
On 2014/11/04 07:22:09, Garrett Casto wrote:
> This is unneeded, right?

Correct, thanks for checking it!
Done.

[vabr (Chromium), 2014-11-05 12:00:57]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2014-11-05 12:02:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/686653003/60001

[commit-bot: I haz the power, 2014-11-05 13:00:33]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2014-11-05 13:01:06]

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