Kill renderers for bad password forms in --site-per-process.

Kill renderers for bad password forms in --site-per-process.

Validate the origin from password forms to make sure that the renderer that's sending the autofill requests are allowed to access the passwords.

BUG=467150
TBR=sky@chromium.org for chrome/browser/ui/login/login_prompt.cc

Committed: https://crrev.com/24d08379839a9c94d1c8aa10a8b04185dc6a9b19
Cr-Commit-Position: refs/heads/master@{#339322}

[lfg, 2015-07-07 21:50:34]

lfg@chromium.org changed reviewers:
+ nick@chromium.org

[lfg, 2015-07-07 21:50:34]

Hi Nick, please take a look.

Thanks!

[ncarter (slow), 2015-07-07 22:18:32]

Nice! Just a couple nits!

https://codereview.chromium.org/1212163007/diff/20001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/1212163007/diff/20001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2162:
iframe_observer.SetPathToWaitFor("/password/crossite_iframe_content.html");
Crossite is an inosilicate double chain sodic amphibole and is a rare silicate
mineral belonging to the riebeckite group. It is considered an intermediate
between the amphiboles glaucophane and magnesioriebeckite, which form a series.

Physical Properties
===================
Hand Specimen Colour: blue 
Transparency: translucent, opaque
Lustre: vitreous 
Streak: grey, grey-blue 
Hardness: 6
Density: 0
Habit: elongate bladed or prismatic laths
Cleavage: One: perfect [110]

Optical Properties
==================
PPL Colour: blue, light blue 
Pleochroism: moderate to strong 
Relief: None
Birefringence: 0.01-0.016 
2V: 0-180 
Optic Type: Biaxial (+) and (-)

https://codereview.chromium.org/1212163007/diff/20001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2170: // The
iframe on the http page should get its own process.
This comment looks like copypasta & needs reworking, "the http page" was in
contrast to the https page, in the cookies test?

https://codereview.chromium.org/1212163007/diff/20001/components/password_man...
File components/password_manager/content/browser/bad_message.cc (right):

https://codereview.chromium.org/1212163007/diff/20001/components/password_man...
components/password_manager/content/browser/bad_message.cc:20: reason,
BAD_MESSAGE_MAX + 1);
Do we really need the +1?

https://codereview.chromium.org/1212163007/diff/20001/components/password_man...
File
components/password_manager/content/browser/content_password_manager_driver.cc
(right):

https://codereview.chromium.org/1212163007/diff/20001/components/password_man...
components/password_manager/content/browser/content_password_manager_driver.cc:219:
bad_message::CPMD_BAD_ORIGIN);
Usually we've used a different bad_message reason for each incoming IPC message
type. The idea being, if we ever do see these fail in practice, it'll give us
more information to use to track down the underlying problem.

WDYT about passing a BadMessageReason enum (e.g. FORM_SUBMITTED_BAD_ORIGIN) to
this function?

[DaleCurtis, 2015-07-07 22:20:09]

https://codereview.chromium.org/1212163007/diff/20001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/1212163007/diff/20001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2162:
iframe_observer.SetPathToWaitFor("/password/crossite_iframe_content.html");
On 2015/07/07 22:18:32, ncarter wrote:
> Crossite is an inosilicate double chain sodic amphibole and is a rare silicate
> mineral belonging to the riebeckite group. It is considered an intermediate
> between the amphiboles glaucophane and magnesioriebeckite, which form a
series.
> 
> Physical Properties
> ===================
> Hand Specimen Colour: blue 
> Transparency: translucent, opaque
> Lustre: vitreous 
> Streak: grey, grey-blue 
> Hardness: 6
> Density: 0
> Habit: elongate bladed or prismatic laths
> Cleavage: One: perfect [110]
> 
> Optical Properties
> ==================
> PPL Colour: blue, light blue 
> Pleochroism: moderate to strong 
> Relief: None
> Birefringence: 0.01-0.016 
> 2V: 0-180 
> Optic Type: Biaxial (+) and (-) 

So many words that look like misspellings there -_-

[ncarter (slow), 2015-07-07 22:24:41]

nick@chromium.org changed reviewers:
+ xunlu@chromium.org

[ncarter (slow), 2015-07-07 22:24:41]

I'd like to get xunlu's opinion on this

[xunlu, 2015-07-07 22:57:53]

On 2015/07/07 22:24:41, ncarter wrote:
> I'd like to get xunlu's opinion on this

+gcasto
He is an owner of password manager related files.

[xunlu, 2015-07-07 22:58:10]

xunlu@chromium.org changed reviewers:
+ gcasto@chromium.org
- xunlu@chromium.org

[vabr (Chromium), 2015-07-08 08:26:27]

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

[vabr (Chromium), 2015-07-08 08:26:27]

*password_manager* LGTM with a nit and a question.

Cheers,
Vaclav

https://codereview.chromium.org/1212163007/diff/20001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/1212163007/diff/20001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2146:
switches::kSitePerProcess)) {
Are there trybots running tests with this flag?

https://codereview.chromium.org/1212163007/diff/20001/components/password_man...
File components/password_manager/content/browser/bad_message.cc (right):

https://codereview.chromium.org/1212163007/diff/20001/components/password_man...
components/password_manager/content/browser/bad_message.cc:16: LOG(ERROR)
nit: Please make this a VLOG at least, to allow modular filtering. Moreover,
please make this even DVLOG, unless both of the following is true:
* it would be hard to distinguish this type of renderer crashes otherwise
* we are likely to need this log from users with non-debug builds

(See also http://dev.chromium.org/developers/coding-style#TOC-Logging.)

[lfg, 2015-07-08 15:31:47]

Please, take another look.

https://codereview.chromium.org/1212163007/diff/20001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/1212163007/diff/20001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2146:
switches::kSitePerProcess)) {
On 2015/07/08 08:26:27, vabr (Chromium) wrote:
> Are there trybots running tests with this flag?

Yes, however it's currently only an FYI bot. The bot is linux_site_isolation.

https://codereview.chromium.org/1212163007/diff/20001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2162:
iframe_observer.SetPathToWaitFor("/password/crossite_iframe_content.html");
On 2015/07/07 22:18:32, ncarter wrote:
> Crossite is an inosilicate double chain sodic amphibole and is a rare silicate
> mineral belonging to the riebeckite group. It is considered an intermediate
> between the amphiboles glaucophane and magnesioriebeckite, which form a
series.
> 
> Physical Properties
> ===================
> Hand Specimen Colour: blue 
> Transparency: translucent, opaque
> Lustre: vitreous 
> Streak: grey, grey-blue 
> Hardness: 6
> Density: 0
> Habit: elongate bladed or prismatic laths
> Cleavage: One: perfect [110]
> 
> Optical Properties
> ==================
> PPL Colour: blue, light blue 
> Pleochroism: moderate to strong 
> Relief: None
> Birefringence: 0.01-0.016 
> 2V: 0-180 
> Optic Type: Biaxial (+) and (-) 

Hey! I'm just reusing what's already there ;)

https://codereview.chromium.org/1212163007/diff/20001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2170: // The
iframe on the http page should get its own process.
On 2015/07/07 22:18:32, ncarter wrote:
> This comment looks like copypasta & needs reworking, "the http page" was in
> contrast to the https page, in the cookies test?

Yup. Fixed. There's only a single iframe, so the qualifier is irrelevant.

https://codereview.chromium.org/1212163007/diff/20001/components/password_man...
File components/password_manager/content/browser/bad_message.cc (right):

https://codereview.chromium.org/1212163007/diff/20001/components/password_man...
components/password_manager/content/browser/bad_message.cc:16: LOG(ERROR)
On 2015/07/08 08:26:27, vabr (Chromium) wrote:
> nit: Please make this a VLOG at least, to allow modular filtering. Moreover,
> please make this even DVLOG, unless both of the following is true:
> * it would be hard to distinguish this type of renderer crashes otherwise
> * we are likely to need this log from users with non-debug builds
> 
> (See also http://dev.chromium.org/developers/coding-style#TOC-Logging.)

I believe this is one of the rare cases that we need to log this even on release
builds since the browser process has chosen to terminate the renderer. This is
not logged as a crash and it's otherwise impossible to tell the reason for the
renderer termination. This has been used in the past to track issues in
content/browser/bad_message.h.

Maybe nick@ has some other thoughts about this?

https://codereview.chromium.org/1212163007/diff/20001/components/password_man...
components/password_manager/content/browser/bad_message.cc:20: reason,
BAD_MESSAGE_MAX + 1);
On 2015/07/07 22:18:32, ncarter wrote:
> Do we really need the +1?

Yes, it was needed because histograms don't work when there's only a single
bucket (2 is the minimum). However, now that I've split the reasons, it's not
needed anymore.

https://codereview.chromium.org/1212163007/diff/20001/components/password_man...
File
components/password_manager/content/browser/content_password_manager_driver.cc
(right):

https://codereview.chromium.org/1212163007/diff/20001/components/password_man...
components/password_manager/content/browser/content_password_manager_driver.cc:219:
bad_message::CPMD_BAD_ORIGIN);
On 2015/07/07 22:18:32, ncarter wrote:
> Usually we've used a different bad_message reason for each incoming IPC
message
> type. The idea being, if we ever do see these fail in practice, it'll give us
> more information to use to track down the underlying problem.
> 
> WDYT about passing a BadMessageReason enum (e.g. FORM_SUBMITTED_BAD_ORIGIN) to
> this function?

Done.

[vabr (Chromium), 2015-07-09 06:29:32]

*password_manager* LGTM with a nit about the VLOG.

Thanks!
Vaclav

https://codereview.chromium.org/1212163007/diff/20001/chrome/browser/password...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/1212163007/diff/20001/chrome/browser/password...
chrome/browser/password_manager/password_manager_browsertest.cc:2146:
switches::kSitePerProcess)) {
On 2015/07/08 15:31:47, lfg wrote:
> On 2015/07/08 08:26:27, vabr (Chromium) wrote:
> > Are there trybots running tests with this flag?
> 
> Yes, however it's currently only an FYI bot. The bot is linux_site_isolation.

Acknowledged.

https://codereview.chromium.org/1212163007/diff/20001/components/password_man...
File components/password_manager/content/browser/bad_message.cc (right):

https://codereview.chromium.org/1212163007/diff/20001/components/password_man...
components/password_manager/content/browser/bad_message.cc:16: LOG(ERROR)
On 2015/07/08 15:31:47, lfg wrote:
> On 2015/07/08 08:26:27, vabr (Chromium) wrote:
> > nit: Please make this a VLOG at least, to allow modular filtering. Moreover,
> > please make this even DVLOG, unless both of the following is true:
> > * it would be hard to distinguish this type of renderer crashes otherwise
> > * we are likely to need this log from users with non-debug builds
> > 
> > (See also http://dev.chromium.org/developers/coding-style#TOC-Logging.)
> 
> I believe this is one of the rare cases that we need to log this even on
release
> builds since the browser process has chosen to terminate the renderer. This is
> not logged as a crash and it's otherwise impossible to tell the reason for the
> renderer termination. This has been used in the past to track issues in
> content/browser/bad_message.h.
> 
> Maybe nick@ has some other thoughts about this?

Fair enough about the release builds, but still, please why not make it a
VLOG(1) instead of LOG(ERROR)? They should output at the same time without
VLOG-specific flags, but VLOGs can also be filtered for modules.

[ncarter (slow), 2015-07-09 21:47:30]

lgtm

https://codereview.chromium.org/1212163007/diff/80001/components/password_man...
File components/password_manager/content/browser/bad_message.cc (right):

https://codereview.chromium.org/1212163007/diff/80001/components/password_man...
components/password_manager/content/browser/bad_message.cc:18: <<
static_cast<int>(reason);
Leave this as log error, to match all the other bad_message.cc files.

vabr: in the worst case this warning indicates an attacker trying to escape the
renderer sandbox.  I think it's best to let the logging behavior here match the
other bad_message.cc clauses (it's kind of a shame that we have to duplicate it
in the first place):
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ba...

https://codereview.chromium.org/1212163007/diff/80001/components/password_man...
File components/password_manager/content/browser/bad_message.h (right):

https://codereview.chromium.org/1212163007/diff/80001/components/password_man...
components/password_manager/content/browser/bad_message.h:25: enum class
BadMessageReason {
Since it's an enum class now (which provides a namespace-like scope), I'd be
fine moving BadMessageReason out of the bad_message namespace.

[lfg, 2015-07-16 17:01:12]

@vabr: Please, take another look. There was a large rebase and I had to fix
cross-origin redirects for HTTP auth.

@nick: Please take a look. I've added an exception for chrome:// schemes in
LockToOrigin.

https://codereview.chromium.org/1212163007/diff/80001/components/password_man...
File components/password_manager/content/browser/bad_message.h (right):

https://codereview.chromium.org/1212163007/diff/80001/components/password_man...
components/password_manager/content/browser/bad_message.h:25: enum class
BadMessageReason {
On 2015/07/09 21:47:30, ncarter wrote:
> Since it's an enum class now (which provides a namespace-like scope), I'd be
> fine moving BadMessageReason out of the bad_message namespace.
> 

Done.

[ncarter (slow), 2015-07-16 18:04:10]

lgtm

https://codereview.chromium.org/1212163007/diff/120001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/1212163007/diff/120001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:123:
IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase, PromptForNormalSubmit) {
Would it be better to just do:

using PasswordManagerBrowserTest = PasswordManagerBrowserTestBase

So that you're not renaming all the tests? (Renaming tests always carries some
risk of causing extra work, if you wind up confusing existing bug reports,
drmemory suppressions, flakiness dashboard, etc)

https://codereview.chromium.org/1212163007/diff/120001/components/password_ma...
File
components/password_manager/content/browser/content_password_manager_driver.h
(right):

https://codereview.chromium.org/1212163007/diff/120001/components/password_ma...
components/password_manager/content/browser/content_password_manager_driver.h:87:
bool CheckChildProcessSecurityPolicy(const GURL& url, BadMessageReason);
Give a name to the 2nd parameter.

[lfg, 2015-07-16 18:33:14]

https://codereview.chromium.org/1212163007/diff/120001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/1212163007/diff/120001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:123:
IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase, PromptForNormalSubmit) {
On 2015/07/16 18:04:10, ncarter wrote:
> Would it be better to just do:
> 
> using PasswordManagerBrowserTest = PasswordManagerBrowserTestBase
> 
> So that you're not renaming all the tests? (Renaming tests always carries some
> risk of causing extra work, if you wind up confusing existing bug reports,
> drmemory suppressions, flakiness dashboard, etc)

The tests are already renamed upstream. I should've rebased and made my changes
in different uploads, but forgot to do it.

https://codereview.chromium.org/1212163007/diff/120001/components/password_ma...
File
components/password_manager/content/browser/content_password_manager_driver.h
(right):

https://codereview.chromium.org/1212163007/diff/120001/components/password_ma...
components/password_manager/content/browser/content_password_manager_driver.h:87:
bool CheckChildProcessSecurityPolicy(const GURL& url, BadMessageReason);
On 2015/07/16 18:04:10, ncarter wrote:
> Give a name to the 2nd parameter.

Done.

[ncarter (slow), 2015-07-16 18:55:08]

https://codereview.chromium.org/1212163007/diff/120001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/1212163007/diff/120001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:123:
IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTestBase, PromptForNormalSubmit) {
On 2015/07/16 18:33:14, lfg wrote:
> On 2015/07/16 18:04:10, ncarter wrote:
> > Would it be better to just do:
> > 
> > using PasswordManagerBrowserTest = PasswordManagerBrowserTestBase
> > 
> > So that you're not renaming all the tests? (Renaming tests always carries
some
> > risk of causing extra work, if you wind up confusing existing bug reports,
> > drmemory suppressions, flakiness dashboard, etc)
> 
> The tests are already renamed upstream. I should've rebased and made my
changes
> in different uploads, but forgot to do it.

Ah, sorry!

[vabr (Chromium), 2015-07-17 07:25:10]

*password_manager* LGTM with a comment.
Thanks!
Vaclav

https://codereview.chromium.org/1212163007/diff/140001/components/password_ma...
File
components/password_manager/content/browser/content_password_manager_driver.h
(right):

https://codereview.chromium.org/1212163007/diff/140001/components/password_ma...
components/password_manager/content/browser/content_password_manager_driver.h:77:
void PasswordFormsParsed(const std::vector<autofill::PasswordForm>& forms);
Please make the name more informative. What about
OnPasswordFormsParsedNoRenderCheck?

[lfg, 2015-07-17 15:56:08]

https://codereview.chromium.org/1212163007/diff/140001/components/password_ma...
File
components/password_manager/content/browser/content_password_manager_driver.h
(right):

https://codereview.chromium.org/1212163007/diff/140001/components/password_ma...
components/password_manager/content/browser/content_password_manager_driver.h:77:
void PasswordFormsParsed(const std::vector<autofill::PasswordForm>& forms);
On 2015/07/17 07:25:10, vabr (Chromium) wrote:
> Please make the name more informative. What about
> OnPasswordFormsParsedNoRenderCheck?

Done.

[lfg, 2015-07-17 15:56:50]

lfg@chromium.org changed reviewers:
+ isherman@chromium.org

[lfg, 2015-07-17 15:56:51]

isherman@chromium.org: Please review changes in 

tools/metrics/histograms/

[Ilya Sherman, 2015-07-17 19:40:59]

histograms lgtm

[lfg, 2015-07-17 19:49:06]

The CQ bit was checked by lfg@chromium.org

[lfg, 2015-07-17 19:49:07]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/1212163007/#ps200001
(title: " ")

[commit-bot: I haz the power, 2015-07-17 19:51:02]

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

[commit-bot: I haz the power, 2015-07-17 20:45:43]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-07-17 20:46:45]

Patchset 11 (id:??) landed as
https://crrev.com/24d08379839a9c94d1c8aa10a8b04185dc6a9b19
Cr-Commit-Position: refs/heads/master@{#339322}