[Autofill] Bail on corrupt AutofillHostMsg_ShowPasswordSuggestions message

[Autofill] Bail on corrupt AutofillHostMsg_ShowPasswordSuggestions message

BUG=329978
TEST=none
R=aedla@chromium.org,palmer@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=242066

[palmer, 2013-12-20 00:08:07]

LGTM as-is, but maybe you can also assert more about the vectors. And, for that
matter, the string16s in the vectors.

https://codereview.chromium.org/115733009/diff/1/components/autofill/core/bro...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/115733009/diff/1/components/autofill/core/bro...
components/autofill/core/browser/autofill_manager.cc:613: if (suggestions.size()
!= realms.size())
Does it also make sense to set some minimum and/or maximum size for these
vectors? (And below.)

[Ilya Sherman, 2013-12-20 00:14:59]

https://codereview.chromium.org/115733009/diff/1/components/autofill/core/bro...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/115733009/diff/1/components/autofill/core/bro...
components/autofill/core/browser/autofill_manager.cc:613: if (suggestions.size()
!= realms.size())
On 2013/12/20 00:08:08, Chromium Palmer wrote:
> Does it also make sense to set some minimum and/or maximum size for these
> vectors? (And below.)

Hmm, we already check this in the renderer, so I guess this only applies if the
renderer is misbehaving.  If the renderer is misbehaving, is it worse to have
large sizes in the browser process than in the renderer process?

[palmer, 2013-12-20 00:31:21]

> Hmm, we already check this in the renderer, so I guess this only applies if
the
> renderer is misbehaving.  If the renderer is misbehaving, is it worse to have
> large sizes in the browser process than in the renderer process?

We always assume the renderer is not only misbehaving but is compromised and
spewing lies. :) Any sanity/reliability/security checks have meaning only on the
browser side; repeating the checks in (honest) renderers is merely a performance
optimization. (Often, a useful one, but that is up to you.)

[Ilya Sherman, 2013-12-20 01:40:54]

On 2013/12/20 00:31:21, Chromium Palmer wrote:
> > Hmm, we already check this in the renderer, so I guess this only applies if
> the
> > renderer is misbehaving.  If the renderer is misbehaving, is it worse to
have
> > large sizes in the browser process than in the renderer process?
> 
> We always assume the renderer is not only misbehaving but is compromised and
> spewing lies. :) Any sanity/reliability/security checks have meaning only on
the
> browser side; repeating the checks in (honest) renderers is merely a
performance
> optimization. (Often, a useful one, but that is up to you.)

My point is: What's the harm incurred by the browser process by having large
vectors or strings, and how is it worse than damage that an attacker could do
purely within the renderer process and by clogging the IPC channel?

[Ilya Sherman, 2013-12-20 01:42:32]

I'm going to go ahead and tick the CQ checkbox on this, so that there's
something small and easy to merge if we want to merge this fix.  Happy to
address further review comments in follow-up CLs.  (Or, feel free to cancel the
CQ/revert this CL if you'd rather have all the fixes in once place.)

[commit-bot: I haz the power, 2013-12-20 01:45:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/115733009/1

[commit-bot: I haz the power, 2013-12-20 02:51:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/isherman@chromium.org/115733009/1

[Ilya Sherman, 2013-12-20 06:43:54]

Follow-up posted at https://codereview.chromium.org/98753005

[commit-bot: I haz the power, 2013-12-20 12:07:10]

Change committed as 242066