Move socket API restriction check to browser process

Move socket API restriction check to browser process

BUG=112961
TEST=ui_tests --gtest_filter=*TCPSocket*


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=122326

[Dmitry Polukhin, 2012-02-10 13:24:12]

https://chromiumcodereview.appspot.com/9379002/diff/3003/content/browser/rend...
File content/browser/renderer_host/pepper_message_filter.cc (right):

https://chromiumcodereview.appspot.com/9379002/diff/3003/content/browser/rend...
content/browser/renderer_host/pepper_message_filter.cc:17: #include
"chrome/common/chrome_switches.h"
It looks like header from chrome/... can't be included to content/... What place
would you recommend to place checks that work with chrome command line options?

[Dmitry Polukhin, 2012-02-10 14:05:44]

https://chromiumcodereview.appspot.com/9379002/diff/3003/content/browser/rend...
File content/browser/renderer_host/pepper_message_filter.cc (right):

https://chromiumcodereview.appspot.com/9379002/diff/3003/content/browser/rend...
content/browser/renderer_host/pepper_message_filter.cc:17: #include
"chrome/common/chrome_switches.h"
On 2012/02/10 13:24:12, Dmitry Polukhin wrote:
> It looks like header from chrome/... can't be included to content/... What
place
> would you recommend to place checks that work with chrome command line
options?

It looks like from architectural point of check for socket API should be part of
ContentBrowserClient interface and Chrome implementation should do actual check.
But I'm not sure that I fully understand architecture so please give me
directions.

[jam, 2012-02-10 18:23:54]

http://codereview.chromium.org/9379002/diff/3003/content/browser/renderer_hos...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/9379002/diff/3003/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:17: #include
"chrome/common/chrome_switches.h"
On 2012/02/10 14:05:45, Dmitry Polukhin wrote:
> On 2012/02/10 13:24:12, Dmitry Polukhin wrote:
> > It looks like header from chrome/... can't be included to content/... What
> place
> > would you recommend to place checks that work with chrome command line
> options?
> 
> It looks like from architectural point of check for socket API should be part
of
> ContentBrowserClient interface and Chrome implementation should do actual
check.
> But I'm not sure that I fully understand architecture so please give me
> directions.

see my comment below. the switch and origin checking should be done in the
embedder

http://codereview.chromium.org/9379002/diff/3003/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:53:
"okddffdblfhhnmhodogpojmfkjmhinfp",  // Test SSH Client
these are chrome extension IDs right? if so, extensions is a chrome feature and
doesn't belong in the content module. you will have to add a call out to the
embedder (i.e. through ContentBrowserClient) to ask it if a given socket origin
should be allowed

stepping back a bit. if i'm understanding correctly, what you want is that if a
renderer is hosting one of two known extensions, it can be allowed to use this
sockets APIs. if that's the case, then on construction of the
PepperMessageFilter (so on the UI thread) it can call the embedder with the
render_process_id. i dont know all the extension code, but it seems that we
should be able to get ExtensionService or some other class to tell us which
extension is being hosted in that process. then you can keep a boolean in
PepperMessageFilter about whether that API is allowed or not. then you don't
need the static maps etc.

http://codereview.chromium.org/9379002/diff/3003/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/9379002/diff/3003/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.cc:152:
PepperMessageFilter::SetRenderSite(process()->GetID(), this->routing_id(),
it's a bit odd that RenderViewHost (UI thread) is calling PepperMessageFilter
(IO thread) to give it this data. it also breaks the abstraction layer between
the message filters which are supposed to be self-contained places where a
feature is implemented.

[jam, 2012-02-10 18:33:42]

http://codereview.chromium.org/9379002/diff/3003/content/browser/renderer_hos...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/9379002/diff/3003/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:53:
"okddffdblfhhnmhodogpojmfkjmhinfp",  // Test SSH Client
On 2012/02/10 18:23:54, John Abd-El-Malek wrote:
> these are chrome extension IDs right? if so, extensions is a chrome feature
and
> doesn't belong in the content module. you will have to add a call out to the
> embedder (i.e. through ContentBrowserClient) to ask it if a given socket
origin
> should be allowed
> 
> stepping back a bit. if i'm understanding correctly, what you want is that if
a
> renderer is hosting one of two known extensions, it can be allowed to use this
> sockets APIs. if that's the case, then on construction of the
> PepperMessageFilter (so on the UI thread) it can call the embedder with the
> render_process_id. i dont know all the extension code, but it seems that we
> should be able to get ExtensionService or some other class to tell us which
> extension is being hosted in that process. then you can keep a boolean in
> PepperMessageFilter about whether that API is allowed or not. then you don't
> need the static maps etc.

for completeness, here's how you can do this in the chrome code: 
BrowserContext* browser_context =
RenderProcessHost::FromID(process_id)->GetBrowserContext;
Profile* profile = Profile::FromBrowserContext(browser_context);
std::set<std::string> extensions =
profile->GetExtensionService()->process_map()->GetExtensionsInProcess();

and check if extensions has the two extension IDs that you want to whitelist
this API for

[Dmitry Polukhin, 2012-02-13 14:54:35]

http://codereview.chromium.org/9379002/diff/3003/content/browser/renderer_hos...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/9379002/diff/3003/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:53:
"okddffdblfhhnmhodogpojmfkjmhinfp",  // Test SSH Client
On 2012/02/10 18:23:54, John Abd-El-Malek wrote:
> these are chrome extension IDs right? if so, extensions is a chrome feature
and
> doesn't belong in the content module. you will have to add a call out to the
> embedder (i.e. through ContentBrowserClient) to ask it if a given socket
origin
> should be allowed
> 
> stepping back a bit. if i'm understanding correctly, what you want is that if
a
> renderer is hosting one of two known extensions, it can be allowed to use this
> sockets APIs. if that's the case, then on construction of the
> PepperMessageFilter (so on the UI thread) it can call the embedder with the
> render_process_id. i dont know all the extension code, but it seems that we
> should be able to get ExtensionService or some other class to tell us which
> extension is being hosted in that process. then you can keep a boolean in
> PepperMessageFilter about whether that API is allowed or not. then you don't
> need the static maps etc.


Now this white-list contains only extensions but I don't want to limit it by
extensions only. Because it will complicate development process and all test for
TCP/UDP will have to be rewritten. I would like to keep ability to white-list
other hosts too (like local host or something more complicated).

Moreover as far as I understand render process may render multiple sites. I
would like to be precise and in case of uncompromised render allow to use
TCP/UDP sockets per-site (RenderView) not per-render process.

http://codereview.chromium.org/9379002/diff/3003/content/browser/renderer_hos...
content/browser/renderer_host/pepper_message_filter.cc:53:
"okddffdblfhhnmhodogpojmfkjmhinfp",  // Test SSH Client
On 2012/02/10 18:33:42, John Abd-El-Malek wrote:
> On 2012/02/10 18:23:54, John Abd-El-Malek wrote:
> > these are chrome extension IDs right? if so, extensions is a chrome feature
> and
> > doesn't belong in the content module. you will have to add a call out to the
> > embedder (i.e. through ContentBrowserClient) to ask it if a given socket
> origin
> > should be allowed
> > 
> > stepping back a bit. if i'm understanding correctly, what you want is that
if
> a
> > renderer is hosting one of two known extensions, it can be allowed to use
this
> > sockets APIs. if that's the case, then on construction of the
> > PepperMessageFilter (so on the UI thread) it can call the embedder with the
> > render_process_id. i dont know all the extension code, but it seems that we
> > should be able to get ExtensionService or some other class to tell us which
> > extension is being hosted in that process. then you can keep a boolean in
> > PepperMessageFilter about whether that API is allowed or not. then you don't
> > need the static maps etc.
> 
> for completeness, here's how you can do this in the chrome code: 
> BrowserContext* browser_context =
> RenderProcessHost::FromID(process_id)->GetBrowserContext;
> Profile* profile = Profile::FromBrowserContext(browser_context);
> std::set<std::string> extensions =
> profile->GetExtensionService()->process_map()->GetExtensionsInProcess();
> 
> and check if extensions has the two extension IDs that you want to whitelist
> this API for

My initial version used RenderProcessHost::FromID but it works only on UI thread
but I need to make check on IO thread and check must be sync (I can't post task
on UI thread). Therefore I have to pre-calculate which render can use sockets.

[jam, 2012-02-13 17:12:50]

On 2012/02/13 14:54:35, Dmitry Polukhin wrote:
>
http://codereview.chromium.org/9379002/diff/3003/content/browser/renderer_hos...
> File content/browser/renderer_host/pepper_message_filter.cc (right):
> 
>
http://codereview.chromium.org/9379002/diff/3003/content/browser/renderer_hos...
> content/browser/renderer_host/pepper_message_filter.cc:53:
> "okddffdblfhhnmhodogpojmfkjmhinfp",  // Test SSH Client
> On 2012/02/10 18:23:54, John Abd-El-Malek wrote:
> > these are chrome extension IDs right? if so, extensions is a chrome feature
> and
> > doesn't belong in the content module. you will have to add a call out to the
> > embedder (i.e. through ContentBrowserClient) to ask it if a given socket
> origin
> > should be allowed
> > 
> > stepping back a bit. if i'm understanding correctly, what you want is that
if
> a
> > renderer is hosting one of two known extensions, it can be allowed to use
this
> > sockets APIs. if that's the case, then on construction of the
> > PepperMessageFilter (so on the UI thread) it can call the embedder with the
> > render_process_id. i dont know all the extension code, but it seems that we
> > should be able to get ExtensionService or some other class to tell us which
> > extension is being hosted in that process. then you can keep a boolean in
> > PepperMessageFilter about whether that API is allowed or not. then you don't
> > need the static maps etc.
> 
> 
> Now this white-list contains only extensions but I don't want to limit it by
> extensions only. Because it will complicate development process and all test
for
> TCP/UDP will have to be rewritten. I would like to keep ability to white-list
> other hosts too (like local host or something more complicated).

The simplicity of the shipping code is more important than having to rewrite
tests. But I don't think you have to rewrite them. You can still have a
ContentBrowserClient call to check if a given host is fine for this feature. For
you tests you can allow localhost/127.0.0.1. As for the future, I don't think we
should complicate the code in case in the future we want something more than
extensions. If that day comes, we can think about the best solution then. 

The reason I'm not excited about the current implementation is this thread safe
object, and the fact that RenderViewHost has to know about PepperMessageFilter
implementation. This is an anti-pattern from the chrome design, see
http://dev.chromium.org/developers/design-documents/threading:

"Our approach is to use message passing as the way of communicating between
threads. We discourage locking and threadsafe objects. Instead, objects live on
only one thread, we pass messages between threads for communication, and we use
callback interfaces (implemented by message passing) for most cross-thread
requests."



> 
> Moreover as far as I understand render process may render multiple sites. I
> would like to be precise and in case of uncompromised render allow to use
> TCP/UDP sockets per-site (RenderView) not per-render process.
> 
>
http://codereview.chromium.org/9379002/diff/3003/content/browser/renderer_hos...
> content/browser/renderer_host/pepper_message_filter.cc:53:
> "okddffdblfhhnmhodogpojmfkjmhinfp",  // Test SSH Client
> On 2012/02/10 18:33:42, John Abd-El-Malek wrote:
> > On 2012/02/10 18:23:54, John Abd-El-Malek wrote:
> > > these are chrome extension IDs right? if so, extensions is a chrome
feature
> > and
> > > doesn't belong in the content module. you will have to add a call out to
the
> > > embedder (i.e. through ContentBrowserClient) to ask it if a given socket
> > origin
> > > should be allowed
> > > 
> > > stepping back a bit. if i'm understanding correctly, what you want is that
> if
> > a
> > > renderer is hosting one of two known extensions, it can be allowed to use
> this
> > > sockets APIs. if that's the case, then on construction of the
> > > PepperMessageFilter (so on the UI thread) it can call the embedder with
the
> > > render_process_id. i dont know all the extension code, but it seems that
we
> > > should be able to get ExtensionService or some other class to tell us
which
> > > extension is being hosted in that process. then you can keep a boolean in
> > > PepperMessageFilter about whether that API is allowed or not. then you
don't
> > > need the static maps etc.
> > 
> > for completeness, here's how you can do this in the chrome code: 
> > BrowserContext* browser_context =
> > RenderProcessHost::FromID(process_id)->GetBrowserContext;
> > Profile* profile = Profile::FromBrowserContext(browser_context);
> > std::set<std::string> extensions =
> > profile->GetExtensionService()->process_map()->GetExtensionsInProcess();
> > 
> > and check if extensions has the two extension IDs that you want to whitelist
> > this API for
> 
> My initial version used RenderProcessHost::FromID but it works only on UI
thread
> but I need to make check on IO thread and check must be sync (I can't post
task
> on UI thread). Therefore I have to pre-calculate which render can use sockets.

this solution i outlied above should work for extensions since you know that
when the renderer process is created.

[Dmitry Polukhin, 2012-02-13 17:58:31]

See below.

On Mon, Feb 13, 2012 at 9:12 PM, <jam@chromium.org> wrote:

> The simplicity of the shipping code is more important than having to
> rewrite
> tests. But I don't think you have to rewrite them. You can still have a
> ContentBrowserClient call to check if a given host is fine for this
> feature. For
> you tests you can allow localhost/127.0.0.1. As for the future, I don't
> think we
> should complicate the code in case in the future we want something more
> than
> extensions. If that day comes, we can think about the best solution then.
>

My main concern was that render process can be used for multiple sites and
more site could be added to existing render process in case of high memory
pressure. Am I right that render process can be used for multiple different
sites? If yes, proposed solution with making decision
during PepperMessageFilter construction doesn't work.


>
> The reason I'm not excited about the current implementation is this thread
> safe
> object, and the fact that RenderViewHost has to know about
> PepperMessageFilter
> implementation. This is an anti-pattern from the chrome design, see
>
http://dev.chromium.org/**developers/design-documents/**threading<http://dev....
> :
>
> "Our approach is to use message passing as the way of communicating between
> threads. We discourage locking and threadsafe objects. Instead, objects
> live on
> only one thread, we pass messages between threads for communication, and
> we use
> callback interfaces (implemented by message passing) for most cross-thread
> requests."


I know about it but I have to make decision during sync call on IO thread.
Alternative solution is to postponed check until connect happens that is
async so I will be able to send message to UI thread for the check.

Have you looked to patch #5 that removes dependency between RenderViewHost
and PepperMessageFilter?

[jam, 2012-02-13 19:25:30]

On Mon, Feb 13, 2012 at 9:58 AM, Dmitry Polukhin <dpolukhin@chromium.org>wrote:

> See below.
>
> On Mon, Feb 13, 2012 at 9:12 PM, <jam@chromium.org> wrote:
>
>> The simplicity of the shipping code is more important than having to
>> rewrite
>>  tests. But I don't think you have to rewrite them. You can still have a
>> ContentBrowserClient call to check if a given host is fine for this
>> feature. For
>> you tests you can allow localhost/127.0.0.1. As for the future, I don't
>> think we
>> should complicate the code in case in the future we want something more
>> than
>> extensions. If that day comes, we can think about the best solution then.
>>
>
> My main concern was that render process can be used for multiple sites and
> more site could be added to existing render process in case of high memory
> pressure. Am I right that render process can be used for multiple different
> sites?
>

Extension processes ignore the memory pressure stuff and always have a
separate process per extension. Since we only care about extensions now,
sharing should be a non-issue.


> If yes, proposed solution with making decision during PepperMessageFilter
> construction doesn't work.
>
>
>>
>> The reason I'm not excited about the current implementation is this
>> thread safe
>> object, and the fact that RenderViewHost has to know about
>> PepperMessageFilter
>> implementation. This is an anti-pattern from the chrome design, see
>>
http://dev.chromium.org/**developers/design-documents/**threading<http://dev....
>> :
>>
>> "Our approach is to use message passing as the way of communicating
>> between
>> threads. We discourage locking and threadsafe objects. Instead, objects
>> live on
>> only one thread, we pass messages between threads for communication, and
>> we use
>> callback interfaces (implemented by message passing) for most cross-thread
>> requests."
>
>
> I know about it but I have to make decision during sync call on IO thread.
> Alternative solution is to postponed check until connect happens that is
> async so I will be able to send message to UI thread for the check.
>

If you're able to make this async, then you can also send async messages
from the renderer or posttasks from the IO thread which get responded to on
the UI thread and check this data.


>
> Have you looked to patch #5 that removes dependency between RenderViewHost
> and PepperMessageFilter?
>

I did, the issues I have with the original change still exist.
RenderViewHost still has to know about PepperMessageFilter, but now
indirectly through RenderProcessHost. There's still the locking and
communication between an object that lives on the UI thread and one that
lives on the IO thread.

[Dmitry Polukhin, 2012-02-14 14:22:21]

PTAL, I removed explicit synchronization, static var etc. Now I post task to UI
thread to check render rights in connect/bind methods. I still don't like idea
of making this check extension specific because in tests we use URL so I would
like to use the same check in test and in prod. Also we will need similar
technique for server sockets anyway because they won't have sync create method
at all. Server sockets uses async create&listen.

[jam, 2012-02-14 16:11:26]

On 2012/02/14 14:22:21, Dmitry Polukhin wrote:
> PTAL, I removed explicit synchronization, static var etc. Now I post task to
UI
> thread to check render rights in connect/bind methods. I still don't like idea
> of making this check extension specific because in tests we use URL so I would
> like to use the same check in test and in prod.

I'm not sure I understand what you mean. Now you're passing the SiteInstance to
the embedder, so it's not extension specific right? i.e. in a test you can
either change the ContentBrowserClient implementation, or pass a command line
with a new host to allow.

> Also we will need similar
> technique for server sockets anyway because they won't have sync create method
> at all. Server sockets uses async create&listen.

I don't see what the issue here would be? Async IPCs are much better since we
can handle them on the UI thread and check the RVH there. With things like
OverrideThreadForMessage, this is pretty painless to do.

http://codereview.chromium.org/9379002/diff/13002/chrome/test/ui/ppapi_uitest.cc
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/9379002/diff/13002/chrome/test/ui/ppapi_uitest...
chrome/test/ui/ppapi_uitest.cc:413: // ability to create PP_NetAddress_Private
without ability to crate use sockets.
nit: crate use

http://codereview.chromium.org/9379002/diff/13002/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/9379002/diff/13002/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:371:
iter->second->routing_id(), socket_id, host, port));
when dispatching in a filter in a thread other than IO, BrowserMessageFilter has
a mechanism to automatically do this, see OverrideThreadForMessage:
http://code.google.com/codesearch#OAMlx_jo-ck/src/content/public/browser/brow...

The main benefit, other than having to write less code, is that it has checks to
ensure that no sync messages are dispatched on the UI thread on Windows, which
can cause deadlocks. This guards against someone in the future coming and making
this IPC sync and not noticing.

It looks like the only thing you need from the IO thread is the routing_id, but
you can just send that from the renderer/ppapi process.

[Dmitry Polukhin, 2012-02-15 10:19:22]

PTAL

On 2012/02/14 16:11:26, John Abd-El-Malek wrote:
> I'm not sure I understand what you mean. Now you're passing the SiteInstance
to
> the embedder, so it's not extension specific right? i.e. in a test you can
> either change the ContentBrowserClient implementation, or pass a command line
> with a new host to allow.

Now there is no problem. I was talking another possible approach that makes the
check during PepperMessageFilter construction on UI thread for extensions only.
Now tests and real chrome use absolutely the same code path to check socket
restrictions.

> I don't see what the issue here would be? Async IPCs are much better since we
> can handle them on the UI thread and check the RVH there. With things like
> OverrideThreadForMessage, this is pretty painless to do.

There is nothing wrong with async creation. But I don't want to change TCP/UDP
socket creation to async in this CL because it needs to be merge to M18 so I
would like to make it as small and simple as possible.

http://codereview.chromium.org/9379002/diff/13002/chrome/test/ui/ppapi_uitest.cc
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/9379002/diff/13002/chrome/test/ui/ppapi_uitest...
chrome/test/ui/ppapi_uitest.cc:413: // ability to create PP_NetAddress_Private
without ability to crate use sockets.
On 2012/02/14 16:11:26, John Abd-El-Malek wrote:
> nit: crate use

Done.

http://codereview.chromium.org/9379002/diff/13002/content/browser/renderer_ho...
File content/browser/renderer_host/pepper_message_filter.cc (right):

http://codereview.chromium.org/9379002/diff/13002/content/browser/renderer_ho...
content/browser/renderer_host/pepper_message_filter.cc:371:
iter->second->routing_id(), socket_id, host, port));
On 2012/02/14 16:11:26, John Abd-El-Malek wrote:
> when dispatching in a filter in a thread other than IO, BrowserMessageFilter
has
> a mechanism to automatically do this, see OverrideThreadForMessage:
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/content/public/browser/brow...
> 
> The main benefit, other than having to write less code, is that it has checks
to
> ensure that no sync messages are dispatched on the UI thread on Windows, which
> can cause deadlocks. This guards against someone in the future coming and
making
> this IPC sync and not noticing.
> 
> It looks like the only thing you need from the IO thread is the routing_id,
but
> you can just send that from the renderer/ppapi process.

Done.

[jam, 2012-02-15 16:02:38]

lgtm

On 2012/02/15 10:19:22, Dmitry Polukhin wrote:
> PTAL
> 
> On 2012/02/14 16:11:26, John Abd-El-Malek wrote:
> > I'm not sure I understand what you mean. Now you're passing the SiteInstance
> to
> > the embedder, so it's not extension specific right? i.e. in a test you can
> > either change the ContentBrowserClient implementation, or pass a command
line
> > with a new host to allow.
> 
> Now there is no problem. I was talking another possible approach that makes
the
> check during PepperMessageFilter construction on UI thread for extensions
only.
> Now tests and real chrome use absolutely the same code path to check socket
> restrictions.

ah, got it. Apologies if I wasn't clear, I had sketched that approach as one
method of avoiding the map because I didn't know that you can delay the checking
to later when it's an async IPC. My main issue was the map, anything that solves
that is good.

[commit-bot: I haz the power, 2012-02-16 07:15:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dpolukhin@chromium.org/9379002/29001

[commit-bot: I haz the power, 2012-02-16 07:15:47]

Presubmit check for 9379002-29001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
ppapi/proxy/ppapi_messages.h,ppapi/proxy/ppb_udp_socket_private_proxy.cc,ppapi/proxy/ppb_tcp_socket_private_proxy.cc,ppapi/tests/test_tcp_socket_private_disallowed.h,ppapi/tests/test_tcp_socket_private_disallowed.cc,ppapi/tests/test_udp_socket_private_disallowed.h,ppapi/tests/test_udp_socket_private_disallowed.cc

Presubmit checks took 2.8s to calculate.

[brettw, 2012-02-16 17:41:03]

PPAPI LGTM

[commit-bot: I haz the power, 2012-02-16 17:41:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dpolukhin@chromium.org/9379002/29001

[commit-bot: I haz the power, 2012-02-16 19:06:50]

Change committed as 122326