Pepper: Whitelist dev channel APIs for some apps.

Pepper: Whitelist dev channel APIs for some apps.

Some applications need access to dev channel APIs before they will
become stable. As written, this change only performs whitelisting for NaCl
apps; I don't think we have any need for enabling this in the non-NaCl case.

I need to find a good way to test this.

BUG=383937
R=dmichael@chromium.org, jln@chromium.org, piman@chromium.org

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

[dmichael (off chromium), 2014-06-13 21:07:26]

lgtm

https://codereview.chromium.org/338523007/diff/20001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/338523007/diff/20001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:282:
"4EB74897CB187C7633357C2FE832E0AD6A44883A"   // see crbug.com/234789
I think you might want to reference 383937 instead here?

https://codereview.chromium.org/338523007/diff/20001/components/nacl/browser/...
File components/nacl/browser/nacl_process_host.h (right):

https://codereview.chromium.org/338523007/diff/20001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.h:46: // document_url: The URL of the
document of the Native Client plugin being
document containing the...?

https://codereview.chromium.org/338523007/diff/20001/components/nacl/renderer...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/338523007/diff/20001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:307:
static_cast<int32_t>(PP_ERROR_FAILED)));
suggestion: Looks like you could use PostPPCompletionCallback like below.

https://codereview.chromium.org/338523007/diff/20001/content/browser/ppapi_pl...
File content/browser/ppapi_plugin_process_host.cc (right):

https://codereview.chromium.org/338523007/diff/20001/content/browser/ppapi_pl...
content/browser/ppapi_plugin_process_host.cc:236: // We don't have to do any
whitelisting for APIs this process host, so don't
APIs +in+ this process host?

[teravest, 2014-06-13 21:13:00]

Thanks for the quick review!

https://codereview.chromium.org/338523007/diff/20001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/338523007/diff/20001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:282:
"4EB74897CB187C7633357C2FE832E0AD6A44883A"   // see crbug.com/234789
On 2014/06/13 21:07:26, dmichael wrote:
> I think you might want to reference 383937 instead here?

Done.

https://codereview.chromium.org/338523007/diff/20001/components/nacl/browser/...
File components/nacl/browser/nacl_process_host.h (right):

https://codereview.chromium.org/338523007/diff/20001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.h:46: // document_url: The URL of the
document of the Native Client plugin being
On 2014/06/13 21:07:26, dmichael wrote:
> document containing the...?

Done.

https://codereview.chromium.org/338523007/diff/20001/components/nacl/renderer...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/338523007/diff/20001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:307:
static_cast<int32_t>(PP_ERROR_FAILED)));
On 2014/06/13 21:07:26, dmichael wrote:
> suggestion: Looks like you could use PostPPCompletionCallback like below.

Done.

https://codereview.chromium.org/338523007/diff/20001/content/browser/ppapi_pl...
File content/browser/ppapi_plugin_process_host.cc (right):

https://codereview.chromium.org/338523007/diff/20001/content/browser/ppapi_pl...
content/browser/ppapi_plugin_process_host.cc:236: // We don't have to do any
whitelisting for APIs this process host, so don't
On 2014/06/13 21:07:26, dmichael wrote:
> APIs +in+ this process host?

Done.

[teravest, 2014-06-13 21:14:05]

+jln for components/nacl/common/nacl_host_messages.h
+piman for content/

[piman, 2014-06-13 21:15:27]

o.O

[teravest, 2014-06-16 17:33:24]

Antoine,

I've altered this change to get the document URL on the browser side instead of
retrieving it from the renderer.

The value given by WebContents is a little different-- it gives the URL for the
view instead of the URL for the document that contains the <embed> for the
plugin. These may differ if the plugin is embedded in a frame, but I don't think
applications from the Chrome Web Store are used that way, so we should be fine.

What do you think?

[piman, 2014-06-16 19:29:14]

On 2014/06/16 17:33:24, teravest wrote:
> Antoine,
> 
> I've altered this change to get the document URL on the browser side instead
of
> retrieving it from the renderer.
> 
> The value given by WebContents is a little different-- it gives the URL for
the
> view instead of the URL for the document that contains the <embed> for the
> plugin. These may differ if the plugin is embedded in a frame, but I don't
think
> applications from the Chrome Web Store are used that way, so we should be
fine.
> 
> What do you think?

I'll need to let security folks think about this one... Julien?

[teravest, 2014-06-17 19:24:12]

Julien,

If you're busy, do you have any suggestions for anyone else who can look at
this? There aren't any new messages that need review, but Antoine's concerned
about how URL parsing is happening here.

Since the "dev channel" restriction is purely advisory, malicious plugins could
already use interfaces that have this restriction, and we keep that in mind when
implementing those interfaces. I think the mechanism used here for parsing the
view URL should be fine, and even if that URL is spoofed somehow, it could have
used "dev channel" APIs before anyway.

[teravest, 2014-06-18 02:32:58]

ccing joshwoodward

[jln (very slow on Chromium), 2014-06-19 00:58:20]

Adding felt@ and meacer@. Adrienne, Mustafa, are you familiar with these APIs
and what they grant?

Does anyone have a pointer to a good description of the security model for
something such as RequestOSFileHandle? I didn't find useful comments in the code
after a quick look.

Were these APIs restricted on security grounds at any time, or are they private
only until stabilization?

I would like them to be perceived "safe", for both security reasons (The
whitelisting logic is to complicated to trust blindly for security) and so that
we have an easier path to making them public.

[felt, 2014-06-19 01:24:37]

On 2014/06/19 00:58:20, jln wrote:
> Adding felt@ and meacer@. Adrienne, Mustafa, are you familiar with these APIs
> and what they grant?
> 
> Does anyone have a pointer to a good description of the security model for
> something such as RequestOSFileHandle? I didn't find useful comments in the
code
> after a quick look.
> 
> Were these APIs restricted on security grounds at any time, or are they
private
> only until stabilization?
> 
> I would like them to be perceived "safe", for both security reasons (The
> whitelisting logic is to complicated to trust blindly for security) and so
that
> we have an easier path to making them public.

Neither Mustafa nor I are familiar with the Pepper APIs. Justin, do you know who
has done Pepper API security reviews in the past?

Regardless, I recommend that the whitelisting should be limited specifically to
the APIs that you think Alloy apps need, instead of whitelisting all dev channel
APIs. That way we can check that each of the whitelisted APIs is safe to expose.

[jschuh, 2014-06-19 08:52:52]

On 2014/06/19 01:24:37, felt wrote:
> Regardless, I recommend that the whitelisting should be limited specifically
to
> the APIs that you think Alloy apps need, instead of whitelisting all dev
channel
> APIs. That way we can check that each of the whitelisted APIs is safe to
expose.

Agreed. hese API whitelists are generally speaking not a good practice and have
bitten us in the past. If at all possible we should avoid them, and when we have
no other choice but to use them they must be as narrow as possible.

[joshwoodward1, 2014-06-19 08:57:29]

So should @teravest narrow his CL to just cover the 3 dev APIs covered in the
bug (below) and try to land that patch ASAP?

1) compositor
2) hardware decode
3) trace events

[jschuh, 2014-06-19 09:06:35]

On 2014/06/19 08:57:29, joshwoodward1 wrote:
> So should @teravest narrow his CL to just cover the 3 dev APIs covered in the
> bug (below) and try to land that patch ASAP?
> 
> 1) compositor
> 2) hardware decode
> 3) trace events

Yeah, at a minimum they need to be scoped to the 3 APIs ASAP. At first glance,
the first two don't scare me. Abusing #1 means brute forcing a 64-byte (yes,
that's *byte*) cryptographically random value and #2 is exposed to e.g. Flash
and needs to be well hardened anyway. I haven't looked at #3 in any depth.

[teravest, 2014-06-19 14:12:04]

On 2014/06/19 00:58:20, jln wrote:
> Adding felt@ and meacer@. Adrienne, Mustafa, are you familiar with these APIs
> and what they grant?
> 
> Does anyone have a pointer to a good description of the security model for
> something such as RequestOSFileHandle? I didn't find useful comments in the
code
> after a quick look.
> 
> Were these APIs restricted on security grounds at any time, or are they
private
> only until stabilization?

None of the APIs were ever restricted to dev channel on security grounds.
We now stage APIs there before making them stable, to give us a chance to test
out the API before committing to support it.

> 
> I would like them to be perceived "safe", for both security reasons (The
> whitelisting logic is to complicated to trust blindly for security) and so
that
> we have an easier path to making them public.

[teravest, 2014-06-19 14:24:42]

On 2014/06/19 01:24:37, felt wrote:
> On 2014/06/19 00:58:20, jln wrote:
> > Adding felt@ and meacer@. Adrienne, Mustafa, are you familiar with these
APIs
> > and what they grant?
> > 
> > Does anyone have a pointer to a good description of the security model for
> > something such as RequestOSFileHandle? I didn't find useful comments in the
> code
> > after a quick look.
> > 
> > Were these APIs restricted on security grounds at any time, or are they
> private
> > only until stabilization?
> > 
> > I would like them to be perceived "safe", for both security reasons (The
> > whitelisting logic is to complicated to trust blindly for security) and so
> that
> > we have an easier path to making them public.
> 
> Neither Mustafa nor I are familiar with the Pepper APIs. Justin, do you know
who
> has done Pepper API security reviews in the past?

Not sure. I suppose jschuh has done a lot of the ppapi_messages.h reviews,
but I don't think we have a security review process when new APIs are
introduced.
Presumably reviewing the messages is the right time to do that, since that's
the available footprint of anything newly introduced?

> 
> Regardless, I recommend that the whitelisting should be limited specifically
to
> the APIs that you think Alloy apps need, instead of whitelisting all dev
channel
> APIs. That way we can check that each of the whitelisted APIs is safe to
expose.

I'll look at whitelisting just the specified APIs, but it will require adding
host-side checking to all the existing dev channel APIs, including making the
channel restriction and whitelist availble in the renderer.

I'm not confident that I'll be able to get all of that done by the M-37 cutoff.

[teravest, 2014-06-19 17:07:09]

After doing some investigation today, we don't have many dev channel APIs with
an implementation in the host, so I think the risk is small.

As far as I can tell, there are:
  Compositor
  Video Decoder
two new methods in PPB_Messaging_API:
  RegisterMessageHandler
  UnregisterMessageHandler
and two new methods in PPB_MediaStreamVideoTrack:
  GetEmptyFrame
  PutFrame


I'm concerned about trying to hack something together using narrower permissions
by tomorrow, which has proved a bit tricky this morning. Is it okay to get this
in for now, and have a follow up patch to narrow the whitelisting?

[dmichael (off chromium), 2014-06-19 18:03:02]

On Thu, Jun 19, 2014 at 11:07 AM, <teravest@chromium.org> wrote:

> After doing some investigation today, we don't have many dev channel APIs
> with
> an implementation in the host, so I think the risk is small.
>
> As far as I can tell, there are:
>   Compositor
>   Video Decoder
> two new methods in PPB_Messaging_API:
>   RegisterMessageHandler
>   UnregisterMessageHandler
>
^^^ The renderer side of this API is actually already checked in the
renderer. It's in fact more restrictive than necessary at present. (It's
only available with the --enable-pepper-testing flag).


> and two new methods in PPB_MediaStreamVideoTrack:
>   GetEmptyFrame
>   PutFrame
>
Note that these are Pepper adaptions of things that JavaScript could do
today (MediaStreamTrack video output); there's no real threat model. The
only risk associated with them is that if a NaCl app tries really hard,
they can use these before we make them stable, and so they might get broken
if we make changes before promoting them to stable.

It seems to me we could ship this way in 37, but we definitely should add
the renderer-side checking early in 38, in case we have other APIs come
along that are more risky. We can meanwhile valiantly try to get the
renderer-side check in to 37, but it doesn't seem like a release blocker to
me.


>
>
> I'm concerned about trying to hack something together using narrower
> permissions
> by tomorrow, which has proved a bit tricky this morning. Is it okay to get
> this
> in for now, and have a follow up patch to narrow the whitelisting?
>
> https://codereview.chromium.org/338523007/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jln (very slow on Chromium), 2014-06-19 18:22:52]

On 2014/06/19 09:06:35, Justin Schuh wrote:
> On 2014/06/19 08:57:29, joshwoodward1 wrote:
> > So should @teravest narrow his CL to just cover the 3 dev APIs covered in
the
> > bug (below) and try to land that patch ASAP?
> > 
> > 1) compositor
> > 2) hardware decode
> > 3) trace events
> 
> Yeah, at a minimum they need to be scoped to the 3 APIs ASAP. At first glance,
> the first two don't scare me. Abusing #1 means brute forcing a 64-byte (yes,
> that's *byte*) cryptographically random value and #2 is exposed to e.g. Flash
> and needs to be well hardened anyway. I haven't looked at #3 in any depth.

A little bit of a detail here, but the security model for (1) and its large
random value is not as straightforward in the NaCl case, as Mark pointed out.
Untrusted code needs to generate these random values and it can go wrong on a
couple of levels.

[jln (very slow on Chromium), 2014-06-19 18:29:17]

On 2014/06/19 18:03:02, dmichael wrote:
> On Thu, Jun 19, 2014 at 11:07 AM, <mailto:teravest@chromium.org> wrote:
> 
> > After doing some investigation today, we don't have many dev channel APIs
> > with
> > an implementation in the host, so I think the risk is small.
> >
> > As far as I can tell, there are:
> >   Compositor
> >   Video Decoder
> > two new methods in PPB_Messaging_API:
> >   RegisterMessageHandler
> >   UnregisterMessageHandler
> >
> ^^^ The renderer side of this API is actually already checked in the
> renderer. It's in fact more restrictive than necessary at present. (It's
> only available with the --enable-pepper-testing flag).
> 
> 
> > and two new methods in PPB_MediaStreamVideoTrack:
> >   GetEmptyFrame
> >   PutFrame
> >
> Note that these are Pepper adaptions of things that JavaScript could do
> today (MediaStreamTrack video output); there's no real threat model. The
> only risk associated with them is that if a NaCl app tries really hard,
> they can use these before we make them stable, and so they might get broken
> if we make changes before promoting them to stable.
> 
> It seems to me we could ship this way in 37, but we definitely should add
> the renderer-side checking early in 38, in case we have other APIs come
> along that are more risky. We can meanwhile valiantly try to get the
> renderer-side check in to 37, but it doesn't seem like a release blocker to
> me.

This seems reasonable, but I'm wary of tasks like this falling into the void.

If we decided to do this, could we:
- Have an open and assigned bug to do this for M-38
- A comment in a security-gated _message.h file at a spot that would be
necessary to modify to add a new private API. Or some kind of unit test
enumerating (or at least counting) the private API, with a comment that adding
anything requires a security review until the above bug is fixed.

[teravest, 2014-06-19 19:53:38]

On 2014/06/19 18:29:17, jln wrote:
> On 2014/06/19 18:03:02, dmichael wrote:
> > On Thu, Jun 19, 2014 at 11:07 AM, <mailto:teravest@chromium.org> wrote:
> > 
> > > After doing some investigation today, we don't have many dev channel APIs
> > > with
> > > an implementation in the host, so I think the risk is small.
> > >
> > > As far as I can tell, there are:
> > >   Compositor
> > >   Video Decoder
> > > two new methods in PPB_Messaging_API:
> > >   RegisterMessageHandler
> > >   UnregisterMessageHandler
> > >
> > ^^^ The renderer side of this API is actually already checked in the
> > renderer. It's in fact more restrictive than necessary at present. (It's
> > only available with the --enable-pepper-testing flag).
> > 
> > 
> > > and two new methods in PPB_MediaStreamVideoTrack:
> > >   GetEmptyFrame
> > >   PutFrame
> > >
> > Note that these are Pepper adaptions of things that JavaScript could do
> > today (MediaStreamTrack video output); there's no real threat model. The
> > only risk associated with them is that if a NaCl app tries really hard,
> > they can use these before we make them stable, and so they might get broken
> > if we make changes before promoting them to stable.
> > 
> > It seems to me we could ship this way in 37, but we definitely should add
> > the renderer-side checking early in 38, in case we have other APIs come
> > along that are more risky. We can meanwhile valiantly try to get the
> > renderer-side check in to 37, but it doesn't seem like a release blocker to
> > me.
> 
> This seems reasonable, but I'm wary of tasks like this falling into the void.
> 
> If we decided to do this, could we:
> - Have an open and assigned bug to do this for M-38
> - A comment in a security-gated _message.h file at a spot that would be
> necessary to modify to add a new private API. Or some kind of unit test
> enumerating (or at least counting) the private API, with a comment that adding
> anything requires a security review until the above bug is fixed.

Sounds reasonable to me.

I filed a bug (crbug.com/386743), added a comment where the whitelist is added,
and security gated ppapi/thunk/interfaces_ppb_public_dev_channel.h,
which is the file where any new dev channel APIs would be added.

[piman, 2014-06-19 20:06:02]

lgtm if Justin/Julien are ok with this.

[jln (very slow on Chromium), 2014-06-19 22:27:56]

On 2014/06/19 19:53:38, teravest wrote:
> On 2014/06/19 18:29:17, jln wrote:
> > On 2014/06/19 18:03:02, dmichael wrote:
> > > On Thu, Jun 19, 2014 at 11:07 AM, <mailto:teravest@chromium.org> wrote:
> > > 
> > > > After doing some investigation today, we don't have many dev channel
APIs
> > > > with
> > > > an implementation in the host, so I think the risk is small.
> > > >
> > > > As far as I can tell, there are:
> > > >   Compositor
> > > >   Video Decoder
> > > > two new methods in PPB_Messaging_API:
> > > >   RegisterMessageHandler
> > > >   UnregisterMessageHandler
> > > >
> > > ^^^ The renderer side of this API is actually already checked in the
> > > renderer. It's in fact more restrictive than necessary at present. (It's
> > > only available with the --enable-pepper-testing flag).
> > > 
> > > 
> > > > and two new methods in PPB_MediaStreamVideoTrack:
> > > >   GetEmptyFrame
> > > >   PutFrame
> > > >
> > > Note that these are Pepper adaptions of things that JavaScript could do
> > > today (MediaStreamTrack video output); there's no real threat model. The
> > > only risk associated with them is that if a NaCl app tries really hard,
> > > they can use these before we make them stable, and so they might get
broken
> > > if we make changes before promoting them to stable.
> > > 
> > > It seems to me we could ship this way in 37, but we definitely should add
> > > the renderer-side checking early in 38, in case we have other APIs come
> > > along that are more risky. We can meanwhile valiantly try to get the
> > > renderer-side check in to 37, but it doesn't seem like a release blocker
to
> > > me.
> > 
> > This seems reasonable, but I'm wary of tasks like this falling into the
void.
> > 
> > If we decided to do this, could we:
> > - Have an open and assigned bug to do this for M-38
> > - A comment in a security-gated _message.h file at a spot that would be
> > necessary to modify to add a new private API. Or some kind of unit test
> > enumerating (or at least counting) the private API, with a comment that
adding
> > anything requires a security review until the above bug is fixed.
> 
> Sounds reasonable to me.
> 
> I filed a bug (crbug.com/386743), added a comment where the whitelist is
added,
> and security gated ppapi/thunk/interfaces_ppb_public_dev_channel.h,
> which is the file where any new dev channel APIs would be added.

Thanks, lgtm

[igorc, 2014-06-20 05:18:40]

The CQ bit was checked by igorc@chromium.org

[commit-bot: I haz the power, 2014-06-20 05:19:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/teravest@chromium.org/338523007/160001

[hamaji, 2014-06-20 07:51:54]

Committed patchset #9 manually as r278645 (presubmit successful).