Ignore certain messages in plugin broker process if they are not sent by the

Ignore certain messages in plugin broker process if they are not sent by the
browser.

BUG=610600
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/92cad45212731b59e74c8f2a2913691db3e9d770
Cr-Commit-Position: refs/heads/master@{#400495}

[yzshen1, 2016-06-14 22:29:37]

Description was changed from

==========
Ignore certain messages to plugin broker process if they are not sent by the
browser.

BUG=610600
==========

to

==========
Ignore certain messages to plugin broker process if they are not sent by the
browser.

BUG=610600
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[yzshen1, 2016-06-14 22:57:18]

Description was changed from

==========
Ignore certain messages to plugin broker process if they are not sent by the
browser.

BUG=610600
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Ignore certain messages in plugin broker process if they are not sent by the
browser.

BUG=610600
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[yzshen1, 2016-06-14 22:57:18]

yzshen@chromium.org changed reviewers:
+ bbudge@chromium.org, jam@chromium.org, jschuh@chromium.org

[yzshen1, 2016-06-14 23:02:37]

Hi, reviewers.

Would you please take a look?

- bbudge: pepper logic
- jam: OWNER for content/ code.
- jschuh: security review

[bbudge, 2016-06-14 23:16:53]

LGTM

https://codereview.chromium.org/2069853002/diff/20001/content/ppapi_plugin/pp...
File content/ppapi_plugin/ppapi_thread.cc (right):

https://codereview.chromium.org/2069853002/diff/20001/content/ppapi_plugin/pp...
content/ppapi_plugin/ppapi_thread.cc:509: DCHECK_NE(base::kNullProcessId,
renderer_pid);
I'm not very familiar with this part of Pepper, but would a CHECK_NE here
prevent the plugin from crashing the browser process (where you CHECK_NE?)

[yzshen1, 2016-06-14 23:24:47]

https://codereview.chromium.org/2069853002/diff/20001/content/ppapi_plugin/pp...
File content/ppapi_plugin/ppapi_thread.cc (right):

https://codereview.chromium.org/2069853002/diff/20001/content/ppapi_plugin/pp...
content/ppapi_plugin/ppapi_thread.cc:509: DCHECK_NE(base::kNullProcessId,
renderer_pid);
On 2016/06/14 23:16:53, bbudge wrote:
> I'm not very familiar with this part of Pepper, but would a CHECK_NE here
> prevent the plugin from crashing the browser process (where you CHECK_NE?)

We get here because we get a create channel request from the browser, so we have
already passed the CHECK_NE in the browser process.

So I think it won't help to make this a CHECK_NE. WDYT? Thanks!

(I am not familiar with this part of code anymore, either.)

[bbudge, 2016-06-14 23:42:16]

https://codereview.chromium.org/2069853002/diff/20001/content/ppapi_plugin/pp...
File content/ppapi_plugin/ppapi_thread.cc (right):

https://codereview.chromium.org/2069853002/diff/20001/content/ppapi_plugin/pp...
content/ppapi_plugin/ppapi_thread.cc:509: DCHECK_NE(base::kNullProcessId,
renderer_pid);
On 2016/06/14 23:24:47, yzshen1 wrote:
> On 2016/06/14 23:16:53, bbudge wrote:
> > I'm not very familiar with this part of Pepper, but would a CHECK_NE here
> > prevent the plugin from crashing the browser process (where you CHECK_NE?)
> 
> We get here because we get a create channel request from the browser, so we
have
> already passed the CHECK_NE in the browser process.
> 
> So I think it won't help to make this a CHECK_NE. WDYT? Thanks!
> 
> (I am not familiar with this part of code anymore, either.)

Thanks for clarifying!

[jam, 2016-06-15 17:50:38]

lgtm with one change: please add yourself to content/ppapi_plugin/OWNERS

https://codereview.chromium.org/2069853002/diff/20001/content/ppapi_plugin/br...
File content/ppapi_plugin/broker_process_dispatcher.cc (right):

https://codereview.chromium.org/2069853002/diff/20001/content/ppapi_plugin/br...
content/ppapi_plugin/broker_process_dispatcher.cc:118: if (peer_is_browser_) {
nit: might be easier with indentation if you early out instead

[yzshen1, 2016-06-15 18:17:43]

Thanks John!

I also added myself as an OWNER of content/ppapi_plugin/

https://codereview.chromium.org/2069853002/diff/20001/content/ppapi_plugin/br...
File content/ppapi_plugin/broker_process_dispatcher.cc (right):

https://codereview.chromium.org/2069853002/diff/20001/content/ppapi_plugin/br...
content/ppapi_plugin/broker_process_dispatcher.cc:118: if (peer_is_browser_) {
On 2016/06/15 17:50:38, jam wrote:
> nit: might be easier with indentation if you early out instead

Done.

[yzshen1, 2016-06-16 17:59:39]

yzshen@chromium.org changed reviewers:
+ dcheng@chromium.org

[yzshen1, 2016-06-16 17:59:40]

+Daniel for security review.

WRT the change in ppapi_messges.h: This CL doesn't change the meaning of the
message or use it in a new way, it just makes it more explicit what the
parameter means.

[dcheng, 2016-06-16 20:09:26]

lgtm

[jschuh, 2016-06-16 21:08:36]

Almost good, but really think want to terminate the peer if we're getting these
messages, because it certainly implies it is either compromised or seriously
corrupt.

https://codereview.chromium.org/2069853002/diff/40001/content/ppapi_plugin/br...
File content/ppapi_plugin/broker_process_dispatcher.cc (right):

https://codereview.chromium.org/2069853002/diff/40001/content/ppapi_plugin/br...
content/ppapi_plugin/broker_process_dispatcher.cc:118: if (!peer_is_browser_)
This seems like a case where we want to terminate the peer.

[yzshen1, 2016-06-16 22:23:15]

https://codereview.chromium.org/2069853002/diff/40001/content/ppapi_plugin/br...
File content/ppapi_plugin/broker_process_dispatcher.cc (right):

https://codereview.chromium.org/2069853002/diff/40001/content/ppapi_plugin/br...
content/ppapi_plugin/broker_process_dispatcher.cc:118: if (!peer_is_browser_)
On 2016/06/16 21:08:36, jschuh (very slow) wrote:
> This seems like a case where we want to terminate the peer.

I agree that it would be nice if we can kill the peer.

The problem is that this process (the plugin) doesn't have the capability to do
that. Even if we add support for this plugin process to send a request to the
browser process to do that, the browser probably shouldn't trust the words of a
plugin process to terminate a renderer process.

Do you think I could land this CL without that? Thanks!

[jschuh, 2016-06-16 23:06:58]

On 2016/06/16 22:23:15, yzshen1 wrote:
>
https://codereview.chromium.org/2069853002/diff/40001/content/ppapi_plugin/br...
> File content/ppapi_plugin/broker_process_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/2069853002/diff/40001/content/ppapi_plugin/br...
> content/ppapi_plugin/broker_process_dispatcher.cc:118: if (!peer_is_browser_)
> On 2016/06/16 21:08:36, jschuh (very slow) wrote:
> > This seems like a case where we want to terminate the peer.
> 
> I agree that it would be nice if we can kill the peer.
> 
> The problem is that this process (the plugin) doesn't have the capability to
do
> that. Even if we add support for this plugin process to send a request to the
> browser process to do that, the browser probably shouldn't trust the words of
a
> plugin process to terminate a renderer process.
> 
> Do you think I could land this CL without that? Thanks!

Chatted in person, and we do have permission, but it would be a bit ugly to
plumb the process handle through. So, for now I think it's fine to just leave a
comment that we might want to consider killing the peer here if we see problems
in the future.

[yzshen1, 2016-06-17 16:45:08]

On 2016/06/16 23:06:58, jschuh (very slow) wrote:
> On 2016/06/16 22:23:15, yzshen1 wrote:
> >
>
https://codereview.chromium.org/2069853002/diff/40001/content/ppapi_plugin/br...
> > File content/ppapi_plugin/broker_process_dispatcher.cc (right):
> > 
> >
>
https://codereview.chromium.org/2069853002/diff/40001/content/ppapi_plugin/br...
> > content/ppapi_plugin/broker_process_dispatcher.cc:118: if
(!peer_is_browser_)
> > On 2016/06/16 21:08:36, jschuh (very slow) wrote:
> > > This seems like a case where we want to terminate the peer.
> > 
> > I agree that it would be nice if we can kill the peer.
> > 
> > The problem is that this process (the plugin) doesn't have the capability to
> do
> > that. Even if we add support for this plugin process to send a request to
the
> > browser process to do that, the browser probably shouldn't trust the words
of
> a
> > plugin process to terminate a renderer process.
> > 
> > Do you think I could land this CL without that? Thanks!
> 
> Chatted in person, and we do have permission, but it would be a bit ugly to
> plumb the process handle through. So, for now I think it's fine to just leave
a
> comment that we might want to consider killing the peer here if we see
problems
> in the future.

Thanks Justin!
I have added a comment and also a base::debug::DumpWithoutCrashing() so that we
can learn about issues in the field.
(I didn't use UMA stats as it is a bit more trouble to add and remove.)

[jschuh, 2016-06-17 18:46:05]

lgtm

[yzshen1, 2016-06-17 19:07:41]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2016-06-17 19:07:41]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org,
jam@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2069853002/#ps60001
(title: ".")

[commit-bot: I haz the power, 2016-06-17 19:08:02]

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

[commit-bot: I haz the power, 2016-06-17 20:40:20]

Description was changed from

==========
Ignore certain messages in plugin broker process if they are not sent by the
browser.

BUG=610600
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Ignore certain messages in plugin broker process if they are not sent by the
browser.

BUG=610600
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-06-17 20:40:21]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-17 20:43:17]

Description was changed from

==========
Ignore certain messages in plugin broker process if they are not sent by the
browser.

BUG=610600
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Ignore certain messages in plugin broker process if they are not sent by the
browser.

BUG=610600
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/92cad45212731b59e74c8f2a2913691db3e9d770
Cr-Commit-Position: refs/heads/master@{#400495}
==========

[commit-bot: I haz the power, 2016-06-17 20:43:19]

Patchset 4 (id:??) landed as
https://crrev.com/92cad45212731b59e74c8f2a2913691db3e9d770
Cr-Commit-Position: refs/heads/master@{#400495}