Restrict access to WebApkSandboxedProcessService to host browser only

Restrict access to WebApkSandboxedProcessService to host browser only

This CL restricts access to the WebApkSandboxedProcessService
IChildProcessService implementation to the host browser only. The host browser
is defined in the WebAPK's manifest. A RemoteException is thrown if an app other
than the host browser attempts to use WebApkSandboxedProcessService's IBinder
interface

BUG=625448
Committed: https://crrev.com/71de1748dc0a7c4058c8b3662862cda2fb69d8be
Cr-Commit-Position: refs/heads/master@{#405578}

[pkotwicz, 2016-07-03 02:15:12]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2016-07-03 02:27:10]

pkotwicz@chromium.org changed reviewers:
+ yfriedman@chromium.org

[pkotwicz, 2016-07-03 02:27:11]

Yaron, can you please take a look?

I did not find a nice way of sharing code between ChildProcessServiceImpl and
WebApkServiceImpl. I could make the "caller checking logic inherit from
java.util.concurrent.Callable" and pass the callable from
WebApkSandboxedProcessService to ChildProcessServiceImpl#onBind(). I do not like
this solution because it is not possible to pass the result of
Binder#getCallingUid() to Callable#call() because Callable#call() does not take
any parameters.

I am unsure what I should do for testing. An integration test which launches a
real WebAPK would be hard to write. I probably need to pull out
ChildProcessServiceImpl#mBinder to its own top level class in order to unit test
it

[Yaron, 2016-07-05 15:54:22]

lgtm

https://codereview.chromium.org/2114293003/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
(right):

https://codereview.chromium.org/2114293003/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:292:
* {@link authorizedCallerPackageName} calls the service's methods.
nit: indentation

[pkotwicz, 2016-07-05 18:08:23]

pkotwicz@chromium.org changed reviewers:
+ sievers@chromium.org

[pkotwicz, 2016-07-05 18:08:23]

sievers@ can you please take a look at the content/ changes?

[no sievers, 2016-07-06 20:03:00]

https://codereview.chromium.org/2114293003/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
(right):

https://codereview.chromium.org/2114293003/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:114:
int callingUid = Binder.getCallingUid();
Do we really have to do this on every transact, or can we somehow only do it
once when somebody binds to the Service?

[Yaron, 2016-07-06 20:10:06]

https://codereview.chromium.org/2114293003/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
(right):

https://codereview.chromium.org/2114293003/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:114:
int callingUid = Binder.getCallingUid();
On 2016/07/06 20:02:59, sievers wrote:
> Do we really have to do this on every transact, or can we somehow only do it
> once when somebody binds to the Service?

Unfortunately it's not set when someone binds, only while the transaction is
active during a binder call.

It's worth noting here that there are very few calls that actually go over the
binder interface anyway (actually, currently it's only setupConnection but this
is more future-proof). It doesn't affect normal browser<->renderer ipc.

[no sievers, 2016-07-06 20:20:36]

On 2016/07/06 20:10:06, Yaron wrote:
>
https://codereview.chromium.org/2114293003/diff/40001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
> (right):
> 
>
https://codereview.chromium.org/2114293003/diff/40001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:114:
> int callingUid = Binder.getCallingUid();
> On 2016/07/06 20:02:59, sievers wrote:
> > Do we really have to do this on every transact, or can we somehow only do it
> > once when somebody binds to the Service?
> 
> Unfortunately it's not set when someone binds, only while the transaction is
> active during a binder call.
> 
> It's worth noting here that there are very few calls that actually go over the
> binder interface anyway (actually, currently it's only setupConnection but
this
> is more future-proof). It doesn't affect normal browser<->renderer ipc.

Can we still track it as a variable if we had already verified the connection?

Binding to a service more than once (reusal) can't be allowed anyways for
security reasons, so we don't even have to reset it.

[Yaron, 2016-07-06 20:49:29]

On Wed, Jul 6, 2016 at 4:20 PM <sievers@chromium.org> wrote:

> On 2016/07/06 20:10:06, Yaron wrote:
> >
>
>
https://codereview.chromium.org/2114293003/diff/40001/content/public/android/...
> > File
> >
>
>
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
> > (right):
> >
> >
>
>
https://codereview.chromium.org/2114293003/diff/40001/content/public/android/...
> >
>
>
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:114:
> > int callingUid = Binder.getCallingUid();
> > On 2016/07/06 20:02:59, sievers wrote:
> > > Do we really have to do this on every transact, or can we somehow only
> do it
> > > once when somebody binds to the Service?
> >
> > Unfortunately it's not set when someone binds, only while the
> transaction is
> > active during a binder call.
> >
> > It's worth noting here that there are very few calls that actually go
> over the
> > binder interface anyway (actually, currently it's only setupConnection
> but
> this
> > is more future-proof). It doesn't affect normal browser<->renderer ipc.
>
> Can we still track it as a variable if we had already verified the
> connection?
>
> Sure, that ought to be doable.


> Binding to a service more than once (reusal) can't be allowed anyways for
> security reasons, so we don't even have to reset it.
>
> https://codereview.chromium.org/2114293003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[pkotwicz, 2016-07-07 20:36:59]

sievers@ I am confused. Unlike Chrome's SandboxedProcessServices which are
protected by a signature permission the WebAPK's WebApkSandboxedProcessServices
are not protected at all. This means that multiple non-Chrome apps can bind to
WebApkSandboxedProcessService0. My understanding is that if multiple apps
connect to the same WebAPK's WebApkSandboxedProcessService0 that they all use
the same instance of ChildProcessServiceImpl.

This means that we must check the calling uid on every onTransact() call.
Otherwise a non-Chrome app can call
IChildProcessService#crashIntentionallyForTesting() on a renderer used by a
WebAPK.

[no sievers, 2016-07-07 21:58:27]

On 2016/07/07 20:36:59, pkotwicz wrote:
> sievers@ I am confused. Unlike Chrome's SandboxedProcessServices which are
> protected by a signature permission the WebAPK's
WebApkSandboxedProcessServices
> are not protected at all. This means that multiple non-Chrome apps can bind to
> WebApkSandboxedProcessService0. My understanding is that if multiple apps
> connect to the same WebAPK's WebApkSandboxedProcessService0 that they all use
> the same instance of ChildProcessServiceImpl.
> 
> This means that we must check the calling uid on every onTransact() call.
> Otherwise a non-Chrome app can call
> IChildProcessService#crashIntentionallyForTesting() on a renderer used by a
> WebAPK.

But can we just remember the verified UIDs?

In fact, maybe we only have to remember one, since only one can probably be
valid for a given Service instance.

[pkotwicz, 2016-07-14 00:21:57]

Patchset #3 (id:60001) has been deleted

[pkotwicz, 2016-07-14 00:23:37]

Yaron and sievers@ can you please take another look? This CL now depends on
https://codereview.chromium.org/2133923002/

[no sievers, 2016-07-14 20:36:27]

lgtm

[pkotwicz, 2016-07-14 20:50:01]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-07-14 20:50:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
yfriedman@chromium.org
Link to the patchset: https://codereview.chromium.org/2114293003/#ps80001
(title: "Merge branch 'sandbox_on_transact1' into sandbox_on_transact")

[commit-bot: I haz the power, 2016-07-14 20:50:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 21:32:48]

Description was changed from

==========
Restrict access to WebApkSandboxedProcessService to host browser only

This CL restricts access to the WebApkSandboxedProcessService
IChildProcessService implementation to the host browser only. The host browser
is defined in the WebAPK's manifest. A RemoteException is thrown if an app other
than the host browser attempts to use WebApkSandboxedProcessService's IBinder
interface

BUG=625448
==========

to

==========
Restrict access to WebApkSandboxedProcessService to host browser only

This CL restricts access to the WebApkSandboxedProcessService
IChildProcessService implementation to the host browser only. The host browser
is defined in the WebAPK's manifest. A RemoteException is thrown if an app other
than the host browser attempts to use WebApkSandboxedProcessService's IBinder
interface

BUG=625448

Committed: https://crrev.com/71de1748dc0a7c4058c8b3662862cda2fb69d8be
Cr-Commit-Position: refs/heads/master@{#405578}
==========

[commit-bot: I haz the power, 2016-07-14 21:32:49]

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