Add a whitelist check for nacl-nonsfi mode

Add a whitelist check for nacl-nonsfi mode

Also add an explicit check for whitelisting pepper/nacl
permissions based on importing a shared module, so a shared
module can potentially limit allowing import of its resources
and permissions its allowed.

You can still use nonsfi mode for Linux or ChromeOS by
passing in --enable-nacl-nonsfi-mode, but it is on for a
whitelisted set of extensions for ChromeOS without flags.

BUG=355141
TEST=deploy_chrome to daisy, test app which includes whitelisted module

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

[elijahtaylor1, 2014-05-03 01:04:09]

Depends on recently landed: https://codereview.chromium.org/264923011

PTAL

[Mark Seaborn, 2014-05-03 01:08:06]

On 2 May 2014 18:04, <elijahtaylor@chromium.org> wrote:

> Reviewers: jln, Mark Seaborn, hamaji,
>
> Message:
> Depends on recently landed: https://codereview.chromium.org/264923011


That's a link to the same change.  Did you mean to paste a different URL?

Cheers,
Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[Mark Seaborn, 2014-05-03 01:08:07]

On 2 May 2014 18:04, <elijahtaylor@chromium.org> wrote:

> Reviewers: jln, Mark Seaborn, hamaji,
>
> Message:
> Depends on recently landed: https://codereview.chromium.org/264923011


That's a link to the same change.  Did you mean to paste a different URL?

Cheers,
Mark

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

[elijahtaylor1, 2014-05-03 01:19:04]

Ha, I did but now I'm on my mobile. Its my last landed change, can you grab
the link?
On May 2, 2014 6:08 PM, "Mark Seaborn" <mseaborn@chromium.org> wrote:

> On 2 May 2014 18:04, <elijahtaylor@chromium.org> wrote:
>
>> Reviewers: jln, Mark Seaborn, hamaji,
>>
>> Message:
>> Depends on recently landed: https://codereview.chromium.org/264923011
>
>
> That's a link to the same change.  Did you mean to paste a different URL?
>
> Cheers,
> Mark
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[elijahtaylor1, 2014-05-03 01:19:05]

Ha, I did but now I'm on my mobile. Its my last landed change, can you grab
the link?
On May 2, 2014 6:08 PM, "Mark Seaborn" <mseaborn@chromium.org> wrote:

> On 2 May 2014 18:04, <elijahtaylor@chromium.org> wrote:
>
>> Reviewers: jln, Mark Seaborn, hamaji,
>>
>> Message:
>> Depends on recently landed: https://codereview.chromium.org/264923011
>
>
> That's a link to the same change.  Did you mean to paste a different URL?
>
> Cheers,
> Mark
>
>

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

[elijahtaylor1, 2014-05-03 01:33:23]

Ah never mind here it is: https://codereview.chromium.org/263703002/
On May 2, 2014 6:19 PM, "Elijah Taylor" <elijahtaylor@chromium.org> wrote:

> Ha, I did but now I'm on my mobile. Its my last landed change, can you
> grab the link?
> On May 2, 2014 6:08 PM, "Mark Seaborn" <mseaborn@chromium.org> wrote:
>
>> On 2 May 2014 18:04, <elijahtaylor@chromium.org> wrote:
>>
>>> Reviewers: jln, Mark Seaborn, hamaji,
>>>
>>> Message:
>>> Depends on recently landed: https://codereview.chromium.org/264923011
>>
>>
>> That's a link to the same change.  Did you mean to paste a different URL?
>>
>> Cheers,
>> Mark
>>
>>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[elijahtaylor1, 2014-05-03 01:33:24]

Ah never mind here it is: https://codereview.chromium.org/263703002/
On May 2, 2014 6:19 PM, "Elijah Taylor" <elijahtaylor@chromium.org> wrote:

> Ha, I did but now I'm on my mobile. Its my last landed change, can you
> grab the link?
> On May 2, 2014 6:08 PM, "Mark Seaborn" <mseaborn@chromium.org> wrote:
>
>> On 2 May 2014 18:04, <elijahtaylor@chromium.org> wrote:
>>
>>> Reviewers: jln, Mark Seaborn, hamaji,
>>>
>>> Message:
>>> Depends on recently landed: https://codereview.chromium.org/264923011
>>
>>
>> That's a link to the same change.  Did you mean to paste a different URL?
>>
>> Cheers,
>> Mark
>>
>>

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

[Mark Seaborn, 2014-05-03 01:37:59]

Note that we shouldn't commit this until more security hardening of Non-SFI Mode
has been done.

https://codereview.chromium.org/264923011/diff/1/chrome/browser/nacl_host/nac...
File chrome/browser/nacl_host/nacl_browser_delegate_impl.cc (right):

https://codereview.chromium.org/264923011/diff/1/chrome/browser/nacl_host/nac...
chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:38:
"6EAED1924DB611B6EEF2A664BD077BE7EAD33B8F",  // see crbug.com/355141
Is there a way I can check these IDs?

https://codereview.chromium.org/264923011/diff/1/chrome/common/pepper_permiss...
File chrome/common/pepper_permission_util.cc (right):

https://codereview.chromium.org/264923011/diff/1/chrome/common/pepper_permiss...
chrome/common/pepper_permission_util.cc:62: // We check the whitelist explicitly
even though the extension should
I'm not familiar with this part of the code, so I'm not sure what this change
does.  Can it be unit-tested?

https://codereview.chromium.org/264923011/diff/1/chrome/common/pepper_permiss...
chrome/common/pepper_permission_util.cc:63: // never have been allowed installed
in the first place if this fails.
"allowed to be installed"?

https://codereview.chromium.org/264923011/diff/1/components/nacl/browser/nacl...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/264923011/diff/1/components/nacl/browser/nacl...
components/nacl/browser/nacl_process_host.cc:433: if (uses_nonsfi_mode_) {
Note that this code path was originally just meant to be a sanity check that
prevents the renderer from requesting a nonsfi child process when it shouldn't.

This code path doesn't control whether the trusted plugin (in the renderer)
selects "arm-nonsfi" from the manifest file.  That's controlled by
components/nacl/renderer/ppb_nacl_private_impl.cc currently.  You might need to
add an IPC to plumb the info through from renderer to browser -- Hidehiko
started doing that when he changed ppb_nacl_private_impl.cc (but didn't commit
it because it turned out not to be necessary).

Oh, I see you've seen ppb_nacl_private_impl.cc already...

https://codereview.chromium.org/264923011/diff/1/components/nacl/browser/nacl...
components/nacl/browser/nacl_process_host.cc:434: const bool
kNonSFIModeSwitchEnabled =
Nit: I think the "k" naming style is only for values that are constant at
compile time.

https://codereview.chromium.org/264923011/diff/1/components/nacl/browser/nacl...
components/nacl/browser/nacl_process_host.cc:436: #if defined(OS_CHROMEOS)
We also need to make this ARM-only.

https://codereview.chromium.org/264923011/diff/1/components/nacl/renderer/ppb...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/264923011/diff/1/components/nacl/renderer/ppb...
components/nacl/renderer/ppb_nacl_private_impl.cc:509: return PP_TRUE;
Hmm, really the browser should tell the renderer whether a given app is allowed
to run under Non-SFI mode.  (Or is that difficult to plumb through?)

Otherwise, if a non-whitelisted app contains SFI and Non-SFI entries in its NMF,
it would break on Chrome OS.  The renderer would request a Non-SFI process
launch, only to have it rejected by the browser.

Or would you say that it's OK for this to break, because non-whitelisted apps
shouldn't list Non-SFI nexes in their NMFs?  (That might be OK, unless the
whitelisting status of an app changes.)

[elijahtaylor1, 2014-05-06 05:56:15]

Re: not submitting this until nonsfi mode is hardened more, I don't agree. 
Either we trust this mechanism to limit nonsfi mode to a subset of apps and
platforms or we don't.  And even on the platforms it's allowed to run on (just
in case there is a mistake that lets any CWS app run) there is still a sandbox
in place, the hardening is just not 100% complete.  I don't see this as a reason
not to allow this whitelisting to go in.

https://codereview.chromium.org/264923011/diff/1/chrome/browser/nacl_host/nac...
File chrome/browser/nacl_host/nacl_browser_delegate_impl.cc (right):

https://codereview.chromium.org/264923011/diff/1/chrome/browser/nacl_host/nac...
chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:38:
"6EAED1924DB611B6EEF2A664BD077BE7EAD33B8F",  // see crbug.com/355141
On 2014/05/03 01:37:59, Mark Seaborn wrote:
> Is there a way I can check these IDs?

Yes, sorry, I meant to update the linked bug with those IDs.  This is done now.

https://codereview.chromium.org/264923011/diff/1/chrome/common/pepper_permiss...
File chrome/common/pepper_permission_util.cc (right):

https://codereview.chromium.org/264923011/diff/1/chrome/common/pepper_permiss...
chrome/common/pepper_permission_util.cc:62: // We check the whitelist explicitly
even though the extension should
On 2014/05/03 01:37:59, Mark Seaborn wrote:
> I'm not familiar with this part of the code, so I'm not sure what this change
> does.  Can it be unit-tested?

Added some unit tests for this function.

https://codereview.chromium.org/264923011/diff/1/chrome/common/pepper_permiss...
chrome/common/pepper_permission_util.cc:63: // never have been allowed installed
in the first place if this fails.
On 2014/05/03 01:37:59, Mark Seaborn wrote:
> "allowed to be installed"?

Done.

https://codereview.chromium.org/264923011/diff/1/components/nacl/browser/nacl...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/264923011/diff/1/components/nacl/browser/nacl...
components/nacl/browser/nacl_process_host.cc:433: if (uses_nonsfi_mode_) {
On 2014/05/03 01:37:59, Mark Seaborn wrote:
> Note that this code path was originally just meant to be a sanity check that
> prevents the renderer from requesting a nonsfi child process when it
shouldn't.
> 
> This code path doesn't control whether the trusted plugin (in the renderer)
> selects "arm-nonsfi" from the manifest file.  That's controlled by
> components/nacl/renderer/ppb_nacl_private_impl.cc currently.  You might need
to
> add an IPC to plumb the info through from renderer to browser -- Hidehiko
> started doing that when he changed ppb_nacl_private_impl.cc (but didn't commit
> it because it turned out not to be necessary).
> 
> Oh, I see you've seen ppb_nacl_private_impl.cc already...

I'll address this in ppb_nacl_private_impl.cc

https://codereview.chromium.org/264923011/diff/1/components/nacl/browser/nacl...
components/nacl/browser/nacl_process_host.cc:434: const bool
kNonSFIModeSwitchEnabled =
On 2014/05/03 01:37:59, Mark Seaborn wrote:
> Nit: I think the "k" naming style is only for values that are constant at
> compile time.

Done.

https://codereview.chromium.org/264923011/diff/1/components/nacl/browser/nacl...
components/nacl/browser/nacl_process_host.cc:436: #if defined(OS_CHROMEOS)
On 2014/05/03 01:37:59, Mark Seaborn wrote:
> We also need to make this ARM-only.

Done.

https://codereview.chromium.org/264923011/diff/1/components/nacl/renderer/ppb...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/264923011/diff/1/components/nacl/renderer/ppb...
components/nacl/renderer/ppb_nacl_private_impl.cc:509: return PP_TRUE;
On 2014/05/03 01:37:59, Mark Seaborn wrote:
> Hmm, really the browser should tell the renderer whether a given app is
allowed
> to run under Non-SFI mode.  (Or is that difficult to plumb through?)
> 
> Otherwise, if a non-whitelisted app contains SFI and Non-SFI entries in its
NMF,
> it would break on Chrome OS.  The renderer would request a Non-SFI process
> launch, only to have it rejected by the browser.
> 
> Or would you say that it's OK for this to break, because non-whitelisted apps
> shouldn't list Non-SFI nexes in their NMFs?  (That might be OK, unless the
> whitelisting status of an app changes.)

It's not incredibly difficult to plumb this IPC, but it does require changing
the NaCl Private interface (to add an instance parameter so we can look up the
manifest location), and I'm not sure it's worth it.  I would argue what you
wrote last, that non-whitelisted apps shouldn't have nonsfi entries.

Let me know what you think, I could be convinced either way.

[Mark Seaborn, 2014-05-08 01:06:01]

On 2014/05/06 05:56:15, elijahtaylor1 wrote:
> Re: not submitting this until nonsfi mode is hardened more, I
> don't agree.  Either we trust this mechanism to limit nonsfi
> mode to a subset of apps and platforms or we don't.  And even
> on the platforms it's allowed to run on (just in case there is
> a mistake that lets any CWS app run) there is still a sandbox
> in place, the hardening is just not 100% complete.  I don't see
> this as a reason not to allow this whitelisting to go in.

OK, now that I've committed
https://src.chromium.org/viewvc/chrome?revision=268999&view=revision, I am OK
with committing this in principle.

I'd like to get Julien to sign off on this, plus someone who is familiar with
the Chrome extensions code.  (You probably need an owners review for
pepper_permission_util.cc anyway.)

Can you add a TEST= line to say how you tested this, please?  Also, you should
run trybots (including the non-default linux_rel_precise32) if you want to land
this soon. :-)

https://codereview.chromium.org/264923011/diff/1/components/nacl/renderer/ppb...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/264923011/diff/1/components/nacl/renderer/ppb...
components/nacl/renderer/ppb_nacl_private_impl.cc:509: return PP_TRUE;
On 2014/05/06 05:56:15, elijahtaylor1 wrote:
> On 2014/05/03 01:37:59, Mark Seaborn wrote:
> > Hmm, really the browser should tell the renderer whether a given app is
> allowed
> > to run under Non-SFI mode.  (Or is that difficult to plumb through?)
> > 
> > Otherwise, if a non-whitelisted app contains SFI and Non-SFI entries in its
> NMF,
> > it would break on Chrome OS.  The renderer would request a Non-SFI process
> > launch, only to have it rejected by the browser.
> > 
> > Or would you say that it's OK for this to break, because non-whitelisted
apps
> > shouldn't list Non-SFI nexes in their NMFs?  (That might be OK, unless the
> > whitelisting status of an app changes.)
> 
> It's not incredibly difficult to plumb this IPC, but it does require changing
> the NaCl Private interface (to add an instance parameter so we can look up the
> manifest location), and I'm not sure it's worth it.  I would argue what you
> wrote last, that non-whitelisted apps shouldn't have nonsfi entries.

OK.  Can you comment this limitation somewhere appropriate in the code, please?

https://codereview.chromium.org/264923011/diff/10001/chrome/browser/nacl_host...
File chrome/browser/nacl_host/nacl_browser_delegate_impl.cc (right):

https://codereview.chromium.org/264923011/diff/10001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:37: const char*
kAllowedNonSfiOrigins[] = {
Nit: This array should be const (const char *const)

Maybe comment here that these are temporarily allowed to use non-sfi mode for
testing purposes?

https://codereview.chromium.org/264923011/diff/10001/components/nacl/browser/...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/264923011/diff/10001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.cc:436: #if defined(OS_CHROMEOS) &&
defined(ARCH_CPU_ARM_FAMILY)
This should be ARCH_CPU_ARMEL, otherwise it would get enabled for ARM64 too.

https://codereview.chromium.org/264923011/diff/10001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.cc:438: #elif defined(OS_LINUX) ||
defined(OS_CHROMEOS)
You only need OS_LINUX.  OS_CHROMEOS implies OS_LINUX.

https://codereview.chromium.org/264923011/diff/10001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.cc:443: bool is_enabled =
nonsfi_switch_enabled ||
I find this logic hard to understand because nonsfi_switch_enabled is checked
multiple times.

How about this:

  bool nonsfi_mode_enabled = false;
#if defined(OS_LINUX)
  nonsfi_mode_enabled ||= cmd->HasSwitch(switches::kEnableNaClNonSfiMode);
# if defined(OS_CHROMEOS) && defined(ARCH_CPU_ARMEL)
  nonsfi_mode_enabled ||=
NaClBrowser::GetDelegate()->IsNonSfiModeAllowed(manifest_url_);
# endif
#endif
  if (!nonsfi_mode_enabled) {
    ...
  }

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

https://codereview.chromium.org/264923011/diff/10001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:508: #if defined(OS_CHROMEOS)
&& defined(ARCH_CPU_ARM_FAMILY)
Same comment as in other file

https://codereview.chromium.org/264923011/diff/10001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:510: #elif defined(OS_LINUX)
|| defined(OS_CHROMEOS)
Same here too

[asargent_no_longer_on_chrome, 2014-05-08 18:30:09]

extensions related code lgtm

[elijahtaylor1, 2014-05-08 20:00:24]

PTAL, feedback addressed and rebased to latest

https://codereview.chromium.org/264923011/diff/10001/chrome/browser/nacl_host...
File chrome/browser/nacl_host/nacl_browser_delegate_impl.cc (right):

https://codereview.chromium.org/264923011/diff/10001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:37: const char*
kAllowedNonSfiOrigins[] = {
On 2014/05/08 01:06:02, Mark Seaborn wrote:
> Nit: This array should be const (const char *const)
> 
> Maybe comment here that these are temporarily allowed to use non-sfi mode for
> testing purposes?

Done.

https://codereview.chromium.org/264923011/diff/10001/components/nacl/browser/...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/264923011/diff/10001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.cc:436: #if defined(OS_CHROMEOS) &&
defined(ARCH_CPU_ARM_FAMILY)
On 2014/05/08 01:06:02, Mark Seaborn wrote:
> This should be ARCH_CPU_ARMEL, otherwise it would get enabled for ARM64 too.

Thanks, I didn't know there was this distinction

https://codereview.chromium.org/264923011/diff/10001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.cc:438: #elif defined(OS_LINUX) ||
defined(OS_CHROMEOS)
On 2014/05/08 01:06:02, Mark Seaborn wrote:
> You only need OS_LINUX.  OS_CHROMEOS implies OS_LINUX.

Done.

https://codereview.chromium.org/264923011/diff/10001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.cc:443: bool is_enabled =
nonsfi_switch_enabled ||
On 2014/05/08 01:06:02, Mark Seaborn wrote:
> I find this logic hard to understand because nonsfi_switch_enabled is checked
> multiple times.
> 
> How about this:
> 
>   bool nonsfi_mode_enabled = false;
> #if defined(OS_LINUX)
>   nonsfi_mode_enabled ||= cmd->HasSwitch(switches::kEnableNaClNonSfiMode);
> # if defined(OS_CHROMEOS) && defined(ARCH_CPU_ARMEL)
>   nonsfi_mode_enabled ||=
> NaClBrowser::GetDelegate()->IsNonSfiModeAllowed(manifest_url_);
> # endif
> #endif
>   if (!nonsfi_mode_enabled) {
>     ...
>   }

Yeah, I wasn't in love with how this turned out.  I like that your version just
uses one flag that's additive.

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

https://codereview.chromium.org/264923011/diff/10001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:508: #if defined(OS_CHROMEOS)
&& defined(ARCH_CPU_ARM_FAMILY)
On 2014/05/08 01:06:02, Mark Seaborn wrote:
> Same comment as in other file

Done.

https://codereview.chromium.org/264923011/diff/10001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:510: #elif defined(OS_LINUX)
|| defined(OS_CHROMEOS)
On 2014/05/08 01:06:02, Mark Seaborn wrote:
> Same here too

Done.

[jln (very slow on Chromium), 2014-05-08 23:22:41]

This looks good in general. I will still do another pass and discuss a few
details with Mark.

https://chromiumcodereview.appspot.com/264923011/diff/30001/chrome/browser/na...
File chrome/browser/nacl_host/nacl_browser_delegate_impl.cc (right):

https://chromiumcodereview.appspot.com/264923011/diff/30001/chrome/browser/na...
chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:37: // These are
tempoararily needed for testing non-sfi mode on ChromeOS without
nit: temporarily

https://chromiumcodereview.appspot.com/264923011/diff/30001/chrome/common/pep...
File chrome/common/pepper_permission_util.cc (right):

https://chromiumcodereview.appspot.com/264923011/diff/30001/chrome/common/pep...
chrome/common/pepper_permission_util.cc:38: bool
IsExtensionOrSharedModuleWhitelisted(
I didn't find pepper_permission_util.h very clear.

Could you be a little more verbose in the header? For instance:
- Mention that the extension is identified by |url|.
- What should |extension_set| represent precisely?
- The transitivity level is 1, correct? i.e. if a module itself imports a
whitelisted module, this will not pass the check.

https://chromiumcodereview.appspot.com/264923011/diff/30001/chrome/common/pep...
chrome/common/pepper_permission_util.cc:53: if (extension) {
Nit: I would find "if (!extension) { return false; } more readable, and this
allows you do de-indent the rest of the code one level up.

https://chromiumcodereview.appspot.com/264923011/diff/30001/chrome/common/pep...
File chrome/common/pepper_permission_util_unittest.cc (right):

https://chromiumcodereview.appspot.com/264923011/diff/30001/chrome/common/pep...
chrome/common/pepper_permission_util_unittest.cc:47: std::string id =
id_util::GenerateId("whitelisted_extension");
Do you want to rename to whitelisted_id?

https://chromiumcodereview.appspot.com/264923011/diff/30001/chrome/common/pep...
chrome/common/pepper_permission_util_unittest.cc:85: std::string id =
id_util::GenerateId("extension_id");
s/id/whitelisted_id/ ?

https://chromiumcodereview.appspot.com/264923011/diff/30001/chrome/common/pep...
chrome/common/pepper_permission_util_unittest.cc:97: ListBuilder().Append(id)))
Maybe add a comment that this is where the magic happens?

https://chromiumcodereview.appspot.com/264923011/diff/30001/chrome/common/pep...
chrome/common/pepper_permission_util_unittest.cc:121: 
Could you add another EXPECT_FALSE() after removing whitelisted_id from
|whitelist|?

https://chromiumcodereview.appspot.com/264923011/diff/30001/components/nacl/b...
File components/nacl/browser/nacl_process_host.cc (right):

https://chromiumcodereview.appspot.com/264923011/diff/30001/components/nacl/b...
components/nacl/browser/nacl_process_host.cc:436: nonsfi_mode_enabled =
nonsfi_mode_enabled ||
This is fine, but if you don't mind havint different bools, I think it would
make this more readable for security:

#if... OS_LINUX
const bool non_sfi_mode_forced_by_command_line = cmd->..
# if defined(OS_CHROMEOS) && .. ARMEL)
const bool non_sfi_mode_allowed = ...->IsNonSfiModeAllowed);
#endif

nonsfi_mode_enabled = !non_sfi_mode_allowed &&
!non_sfi_mode_forced_by_command_line;

[jln (very slow on Chromium), 2014-05-08 23:44:56]

https://chromiumcodereview.appspot.com/264923011/diff/30001/components/nacl/b...
File components/nacl/browser/nacl_process_host.cc (right):

https://chromiumcodereview.appspot.com/264923011/diff/30001/components/nacl/b...
components/nacl/browser/nacl_process_host.cc:436: nonsfi_mode_enabled =
nonsfi_mode_enabled ||
> nonsfi_mode_enabled = !non_sfi_mode_allowed &&
> !non_sfi_mode_forced_by_command_line;

err, enabled, not disabled:

nonsfi_mode_enabled = non_sfi_mode_allowed ||
non_sfi_mode_forced_by_command_line;

[elijahtaylor1, 2014-05-09 00:33:27]

I will need to rebase when https://codereview.chromium.org/277463003/ lands as
my change depends on it

https://codereview.chromium.org/264923011/diff/30001/chrome/browser/nacl_host...
File chrome/browser/nacl_host/nacl_browser_delegate_impl.cc (right):

https://codereview.chromium.org/264923011/diff/30001/chrome/browser/nacl_host...
chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:37: // These are
tempoararily needed for testing non-sfi mode on ChromeOS without
On 2014/05/08 23:22:41, jln wrote:
> nit: temporarily

Done.

https://codereview.chromium.org/264923011/diff/30001/chrome/common/pepper_per...
File chrome/common/pepper_permission_util.cc (right):

https://codereview.chromium.org/264923011/diff/30001/chrome/common/pepper_per...
chrome/common/pepper_permission_util.cc:38: bool
IsExtensionOrSharedModuleWhitelisted(
On 2014/05/08 23:22:41, jln wrote:
> I didn't find pepper_permission_util.h very clear.
> 
> Could you be a little more verbose in the header? For instance:
> - Mention that the extension is identified by |url|.
> - What should |extension_set| represent precisely?
> - The transitivity level is 1, correct? i.e. if a module itself imports a
> whitelisted module, this will not pass the check.

Done.

https://codereview.chromium.org/264923011/diff/30001/chrome/common/pepper_per...
chrome/common/pepper_permission_util.cc:53: if (extension) {
On 2014/05/08 23:22:41, jln wrote:
> Nit: I would find "if (!extension) { return false; } more readable, and this
> allows you do de-indent the rest of the code one level up.

Done.

https://codereview.chromium.org/264923011/diff/30001/chrome/common/pepper_per...
File chrome/common/pepper_permission_util_unittest.cc (right):

https://codereview.chromium.org/264923011/diff/30001/chrome/common/pepper_per...
chrome/common/pepper_permission_util_unittest.cc:47: std::string id =
id_util::GenerateId("whitelisted_extension");
On 2014/05/08 23:22:41, jln wrote:
> Do you want to rename to whitelisted_id?

Done.

https://codereview.chromium.org/264923011/diff/30001/chrome/common/pepper_per...
chrome/common/pepper_permission_util_unittest.cc:85: std::string id =
id_util::GenerateId("extension_id");
On 2014/05/08 23:22:41, jln wrote:
> s/id/whitelisted_id/ ?

Done.

https://codereview.chromium.org/264923011/diff/30001/chrome/common/pepper_per...
chrome/common/pepper_permission_util_unittest.cc:97: ListBuilder().Append(id)))
On 2014/05/08 23:22:41, jln wrote:
> Maybe add a comment that this is where the magic happens?

Done.

https://codereview.chromium.org/264923011/diff/30001/chrome/common/pepper_per...
chrome/common/pepper_permission_util_unittest.cc:121: 
On 2014/05/08 23:22:41, jln wrote:
> Could you add another EXPECT_FALSE() after removing whitelisted_id from
> |whitelist|?

Done.

https://codereview.chromium.org/264923011/diff/30001/components/nacl/browser/...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/264923011/diff/30001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.cc:436: nonsfi_mode_enabled =
nonsfi_mode_enabled ||
On 2014/05/08 23:44:57, jln wrote:
> > nonsfi_mode_enabled = !non_sfi_mode_allowed &&
> > !non_sfi_mode_forced_by_command_line;
> 
> err, enabled, not disabled:
> 
> nonsfi_mode_enabled = non_sfi_mode_allowed ||
> non_sfi_mode_forced_by_command_line;

Done.

[jln (very slow on Chromium), 2014-05-09 00:39:04]

https://codereview.chromium.org/264923011/diff/50001/chrome/common/pepper_per...
File chrome/common/pepper_permission_util_unittest.cc (right):

https://codereview.chromium.org/264923011/diff/50001/chrome/common/pepper_per...
chrome/common/pepper_permission_util_unittest.cc:128: 
You need to re-add shared_module for the next test to be meaningful, no?

[elijahtaylor1, 2014-05-09 00:43:43]

https://codereview.chromium.org/264923011/diff/50001/chrome/common/pepper_per...
File chrome/common/pepper_permission_util_unittest.cc (right):

https://codereview.chromium.org/264923011/diff/50001/chrome/common/pepper_per...
chrome/common/pepper_permission_util_unittest.cc:128: 
On 2014/05/09 00:39:05, jln wrote:
> You need to re-add shared_module for the next test to be meaningful, no?

yes you're right, I will add the erase and added expect_false last

[elijahtaylor1, 2014-05-09 00:46:18]

+sky@chromium.org for OWNERS:

    chrome/common/pepper_permission_util.cc
    chrome/common/pepper_permission_util.h
    chrome/common/pepper_permission_util_unittest.cc

[jln (very slow on Chromium), 2014-05-09 00:55:57]

lgtm

[Mark Seaborn, 2014-05-09 02:12:31]

LGTM

https://codereview.chromium.org/264923011/diff/70001/chrome/common/pepper_per...
File chrome/common/pepper_permission_util.cc (right):

https://codereview.chromium.org/264923011/diff/70001/chrome/common/pepper_per...
chrome/common/pepper_permission_util.cc:64: // We check the whitelist explicitly
even though the extension should
Could you possibly explain what this change does in the commit message, and
explain the reason?

i.e. Before the change, this implicitly whitelists extension A if it depends on
extension B which was explicitly whitelisted.  With this change, you're adding a
sanity check to ensure that B whitelisted A as being allowed to import B.

When you say "the extension should never have been allowed to be installed in
the first place", could you also say in the comment where the check for that is
implemented, for context?

https://codereview.chromium.org/264923011/diff/70001/chrome/common/pepper_per...
File chrome/common/pepper_permission_util_unittest.cc (right):

https://codereview.chromium.org/264923011/diff/70001/chrome/common/pepper_per...
chrome/common/pepper_permission_util_unittest.cc:142: 
Nit: should just be 1 empty line here, not 2

[elijahtaylor1, 2014-05-09 05:37:57]

https://codereview.chromium.org/264923011/diff/70001/chrome/common/pepper_per...
File chrome/common/pepper_permission_util.cc (right):

https://codereview.chromium.org/264923011/diff/70001/chrome/common/pepper_per...
chrome/common/pepper_permission_util.cc:64: // We check the whitelist explicitly
even though the extension should
On 2014/05/09 02:12:31, Mark Seaborn wrote:
> Could you possibly explain what this change does in the commit message, and
> explain the reason?
> 
> i.e. Before the change, this implicitly whitelists extension A if it depends
on
> extension B which was explicitly whitelisted.  With this change, you're adding
a
> sanity check to ensure that B whitelisted A as being allowed to import B.
> 
> When you say "the extension should never have been allowed to be installed in
> the first place", could you also say in the comment where the check for that
is
> implemented, for context?

Done.  While adjusting this comment, I thought of another additional sanity
check to do for shared modules' whitelists, so I added that and a comment to
this CL.

https://codereview.chromium.org/264923011/diff/70001/chrome/common/pepper_per...
File chrome/common/pepper_permission_util_unittest.cc (right):

https://codereview.chromium.org/264923011/diff/70001/chrome/common/pepper_per...
chrome/common/pepper_permission_util_unittest.cc:142: 
On 2014/05/09 02:12:31, Mark Seaborn wrote:
> Nit: should just be 1 empty line here, not 2

Done.

[elijahtaylor1, 2014-05-09 06:13:05]

@sky: please CQ if you LGTM

[elijahtaylor1, 2014-05-09 06:31:39]

On 2014/05/09 06:13:05, elijahtaylor1 wrote:
> @sky: please CQ if you LGTM

Actually, disregard this, do not CQ. I still need to rebase after
https://codereview.chromium.org/277463003/ lands, tree is closed so it's not
submitting yet.

[sky, 2014-05-09 14:20:23]

LGTM

[elijahtaylor1, 2014-05-09 18:39:54]

rebased to include https://codereview.chromium.org/277463003/, added tsepez
(security reviewer for that CL) to review to please look over further changes
based on that CL

https://codereview.chromium.org/264923011/diff/110001/chrome/browser/nacl_hos...
File chrome/browser/nacl_host/nacl_browser_delegate_impl.cc (right):

https://codereview.chromium.org/264923011/diff/110001/chrome/browser/nacl_hos...
chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:299: DCHECK(profile);
I would like confirmation that this is ok to DCHECK on (before it was a test and
'return false' in MapUrlToLocalFilePath)

[Tom Sepez, 2014-05-09 19:27:17]

LGTM given that profile_directory appears to be controlled by the browser.

[Tom Sepez, 2014-05-09 19:27:31]

https://codereview.chromium.org/264923011/diff/130001/chrome/browser/nacl_hos...
File chrome/browser/nacl_host/nacl_browser_delegate_impl.cc (right):

https://codereview.chromium.org/264923011/diff/130001/chrome/browser/nacl_hos...
chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:41:
"6EAED1924DB611B6EEF2A664BD077BE7EAD33B8F",  // see crbug.com/355141
nit: add http:// so various code review tools may make it linkable.

https://codereview.chromium.org/264923011/diff/130001/chrome/common/pepper_pe...
File chrome/common/pepper_permission_util.cc (right):

https://codereview.chromium.org/264923011/diff/130001/chrome/common/pepper_pe...
chrome/common/pepper_permission_util.cc:56: typedef
std::vector<SharedModuleInfo::ImportInfo> ImportInfoVector;
nit: pity this typedef isn't in shared_module_info.h

[elijahtaylor1, 2014-05-09 19:31:24]

https://codereview.chromium.org/264923011/diff/130001/chrome/browser/nacl_hos...
File chrome/browser/nacl_host/nacl_browser_delegate_impl.cc (right):

https://codereview.chromium.org/264923011/diff/130001/chrome/browser/nacl_hos...
chrome/browser/nacl_host/nacl_browser_delegate_impl.cc:41:
"6EAED1924DB611B6EEF2A664BD077BE7EAD33B8F",  // see crbug.com/355141
On 2014/05/09 19:27:32, Tom Sepez wrote:
> nit: add http:// so various code review tools may make it linkable.

Done.

https://codereview.chromium.org/264923011/diff/130001/chrome/common/pepper_pe...
File chrome/common/pepper_permission_util.cc (right):

https://codereview.chromium.org/264923011/diff/130001/chrome/common/pepper_pe...
chrome/common/pepper_permission_util.cc:56: typedef
std::vector<SharedModuleInfo::ImportInfo> ImportInfoVector;
On 2014/05/09 19:27:32, Tom Sepez wrote:
> nit: pity this typedef isn't in shared_module_info.h

I agree.  I will save this for a future change (as there are more callers of
this to clean up, and I don't want this CL to snowball any more).

[elijahtaylor1, 2014-05-09 19:32:27]

The CQ bit was checked by elijahtaylor@chromium.org

[commit-bot: I haz the power, 2014-05-09 19:35:53]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/elijahtaylor@chromium.org/264923011/15...

[elijahtaylor1, 2014-05-09 22:15:44]

The CQ bit was checked by elijahtaylor@chromium.org

[commit-bot: I haz the power, 2014-05-09 22:20:28]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/elijahtaylor@chromium.org/264923011/17...

[commit-bot: I haz the power, 2014-05-10 19:58:32]

Change committed as 269626