PPAPI: Add PluginPrivateFileSystem

PPAPI: Add PluginPrivateFileSystem

This change introduces PluginPrivate fileSystem, a brand-new per-plugin
sandboxed-isolated filesystem, in PPB_IsolatedFileSystem_Private.

Key points in this CL:
- Adding new isolated filesystem type for PluginPrivate filesystem.
- Granting full access of the filesystem to renderer process.
- Generating plugin ID from plugin's MIME type.


BUG=286242
TEST=manual (see https://codereview.chromium.org/77813004/)
TEST=content_unittests --gtest_filter=PepperFileSystemBrowserHostTest.*
TBR=jochen@chromium.org

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

[nhiroki, 2013-10-29 11:31:06]

Hi Justin and Dave,

This would be ready for an early review.
Can you take a look?

Thanks,
nhiroki

[teravest, 2013-10-29 17:14:11]

https://codereview.chromium.org/26803004/diff/313001/content/renderer/pepper/...
File content/renderer/pepper/pepper_file_system_host.cc (right):

https://codereview.chromium.org/26803004/diff/313001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_system_host.cc:194: root_url_ =
GURL(fileapi::GetIsolatedFileSystemRootURIString(
There's a lot of duplication here with OnHostMsgInitIsolatedFileSystem.

Maybe we should change PpapiHostMsg_FileSystem_InitIsolatedFileSystem to have an
extra string argument "optional_root_name", and then you could reuse that code.

https://codereview.chromium.org/26803004/diff/313001/ppapi/api/private/ppb_pl...
File ppapi/api/private/ppb_plugin_private_file_system_private.idl (right):

https://codereview.chromium.org/26803004/diff/313001/ppapi/api/private/ppb_pl...
ppapi/api/private/ppb_plugin_private_file_system_private.idl:16: interface
PPB_PluginPrivateFileSystem_Private {
It would be nice to unify this with PPB_Ext_CrxFileSystem_Private; I don't like
that we're creating interfaces for every separate file system time. It seems
like the two interfaces should be unified. I'll follow up on that and CC you.

https://codereview.chromium.org/26803004/diff/313001/ppapi/proxy/file_system_...
File ppapi/proxy/file_system_resource.cc (right):

https://codereview.chromium.org/26803004/diff/313001/ppapi/proxy/file_system_...
ppapi/proxy/file_system_resource.cc:137:
Call<PpapiPluginMsg_FileSystem_InitPluginPrivateFileSystemReply>(RENDERER,
Why doesn't this call go to the BROWSER as well?

[nhiroki, 2013-11-12 13:36:15]

Hi,

I restarted to implement PluginPrivate filesystem based on IsolatedFileSystem
interface.
Can you take another look?

Thanks!

https://codereview.chromium.org/26803004/diff/313001/content/renderer/pepper/...
File content/renderer/pepper/pepper_file_system_host.cc (right):

https://codereview.chromium.org/26803004/diff/313001/content/renderer/pepper/...
content/renderer/pepper/pepper_file_system_host.cc:194: root_url_ =
GURL(fileapi::GetIsolatedFileSystemRootURIString(
On 2013/10/29 17:14:11, teravest wrote:
> There's a lot of duplication here with OnHostMsgInitIsolatedFileSystem.
> 
> Maybe we should change PpapiHostMsg_FileSystem_InitIsolatedFileSystem to have
an
> extra string argument "optional_root_name", and then you could reuse that
code.

Done.

https://codereview.chromium.org/26803004/diff/313001/ppapi/api/private/ppb_pl...
File ppapi/api/private/ppb_plugin_private_file_system_private.idl (right):

https://codereview.chromium.org/26803004/diff/313001/ppapi/api/private/ppb_pl...
ppapi/api/private/ppb_plugin_private_file_system_private.idl:16: interface
PPB_PluginPrivateFileSystem_Private {
On 2013/10/29 17:14:11, teravest wrote:
> It would be nice to unify this with PPB_Ext_CrxFileSystem_Private; I don't
like
> that we're creating interfaces for every separate file system time. It seems
> like the two interfaces should be unified. I'll follow up on that and CC you.

Done by http://crrev.com/233807

[teravest, 2013-11-12 18:04:53]

https://codereview.chromium.org/26803004/diff/453001/content/browser/renderer...
File content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc
(right):

https://codereview.chromium.org/26803004/diff/453001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc:209:
root_url_ = GURL(fileapi::GetIsolatedFileSystemRootURIString(
Should we do anything if root_url_.is_valid() is false? Seems like we'd want to
return some kind of error here.

https://codereview.chromium.org/26803004/diff/453001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc:214:
case PP_ISOLATEDFILESYSTEMTYPE_PRIVATE_INVALID:
Should this just be the default case for the switch statement?

https://codereview.chromium.org/26803004/diff/453001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc:234:
GURL origin = browser_ppapi_host_->GetDocumentURLForInstance(
Should we do a validity check on origin here?

https://codereview.chromium.org/26803004/diff/453001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc:312: //
Verify |output| contains only alphabets, digits, or "._-".
This is stricter than what mime types require, right? I think this is probably
ok, though I guess plugins will just silently fail if they have semicolons or
other things in their content-type?

https://codereview.chromium.org/26803004/diff/453001/ppapi/proxy/file_system_...
File ppapi/proxy/file_system_resource.cc (right):

https://codereview.chromium.org/26803004/diff/453001/ppapi/proxy/file_system_...
ppapi/proxy/file_system_resource.cc:106: callback_result_ = params.result();
Thanks for cleaning this up.

https://codereview.chromium.org/26803004/diff/453001/ppapi/shared_impl/file_s...
File ppapi/shared_impl/file_system_util.cc (right):

https://codereview.chromium.org/26803004/diff/453001/ppapi/shared_impl/file_s...
ppapi/shared_impl/file_system_util.cc:14: case
PP_ISOLATEDFILESYSTEMTYPE_PRIVATE_INVALID:
Why bother explicitly handling this case?

[nhiroki, 2013-11-14 11:34:38]

Thanks! Addressed your comments.
Please take another look.

https://codereview.chromium.org/26803004/diff/453001/content/browser/renderer...
File content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc
(right):

https://codereview.chromium.org/26803004/diff/453001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc:209:
root_url_ = GURL(fileapi::GetIsolatedFileSystemRootURIString(
On 2013/11/12 18:04:53, teravest wrote:
> Should we do anything if root_url_.is_valid() is false? Seems like we'd want
to
> return some kind of error here.

Done.

https://codereview.chromium.org/26803004/diff/453001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc:214:
case PP_ISOLATEDFILESYSTEMTYPE_PRIVATE_INVALID:
On 2013/11/12 18:04:53, teravest wrote:
> Should this just be the default case for the switch statement?

Done.

https://codereview.chromium.org/26803004/diff/453001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc:234:
GURL origin = browser_ppapi_host_->GetDocumentURLForInstance(
On 2013/11/12 18:04:53, teravest wrote:
> Should we do a validity check on origin here?

Done.

https://codereview.chromium.org/26803004/diff/453001/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc:312: //
Verify |output| contains only alphabets, digits, or "._-".
On 2013/11/12 18:04:53, teravest wrote:
> This is stricter than what mime types require, right?

That's right. MIME type requires at least one slash should be included, but file
path cannot accept it.

In addition, file path cannot accept ';', ':', '*'.... I guess MIME types for
pepper plugins might not contain those characters, so for now I adopt the
stricter way. If these characters should be acceptable, we need to consider ways
to escape them in consideration of file path limitation (ex. length).

> I think this is probably
> ok, though I guess plugins will just silently fail if they have semicolons or
> other things in their content-type?

Added LOG to show generation error when it contains unacceptable characters.

https://codereview.chromium.org/26803004/diff/453001/ppapi/shared_impl/file_s...
File ppapi/shared_impl/file_system_util.cc (right):

https://codereview.chromium.org/26803004/diff/453001/ppapi/shared_impl/file_s...
ppapi/shared_impl/file_system_util.cc:14: case
PP_ISOLATEDFILESYSTEMTYPE_PRIVATE_INVALID:
On 2013/11/12 18:04:53, teravest wrote:
> Why bother explicitly handling this case?

It's just my preference ('default' could hide newly added types on compile
time).

[teravest, 2013-11-14 17:48:39]

lgtm

[dmichael (off chromium), 2013-11-14 18:09:31]

https://codereview.chromium.org/26803004/diff/863002/chrome/browser/renderer_...
File
chrome/browser/renderer_host/pepper/pepper_isolated_file_system_message_filter.cc
(right):

https://codereview.chromium.org/26803004/diff/863002/chrome/browser/renderer_...
chrome/browser/renderer_host/pepper/pepper_isolated_file_system_message_filter.cc:174:
ppapi::host::HostMessageContext* context) {
I don't see where you check permissions. This should only be available if this
plugin has Private permission. I think the intent was also that it would only
work on ChromeOS.

Here's one example of checking Pepper permissions:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

https://codereview.chromium.org/26803004/diff/863002/chrome/browser/renderer_...
chrome/browser/renderer_host/pepper/pepper_isolated_file_system_message_filter.cc:177:
const std::string fsid =
nit/suggestion: these could be const& to possibly avoid a copy.

https://codereview.chromium.org/26803004/diff/863002/content/browser/renderer...
File content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc
(right):

https://codereview.chromium.org/26803004/diff/863002/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc:126:
reply_context_ = context->MakeReplyMessageContext();
Did you change this for any particular reason? Passing it along with the
callback below is more idiomatic. This probably works fine here, but the reply
context is unique per message. If you have to handle more than 1 message, each
has to have its own ReplyMessageContext. Since you should be handling only 1
message, I think it works okay, but I think it would be better to leave it the
way it was.

https://codereview.chromium.org/26803004/diff/863002/ppapi/proxy/file_system_...
File ppapi/proxy/file_system_resource.cc (right):

https://codereview.chromium.org/26803004/diff/863002/ppapi/proxy/file_system_...
ppapi/proxy/file_system_resource.cc:106: callback_result_ = params.result();
Please comment on why you need to save the callback_result_.

[nhiroki, 2013-11-15 12:24:01]

Thanks! Please take another look.

https://codereview.chromium.org/26803004/diff/863002/chrome/browser/renderer_...
File
chrome/browser/renderer_host/pepper/pepper_isolated_file_system_message_filter.cc
(right):

https://codereview.chromium.org/26803004/diff/863002/chrome/browser/renderer_...
chrome/browser/renderer_host/pepper/pepper_isolated_file_system_message_filter.cc:174:
ppapi::host::HostMessageContext* context) {
On 2013/11/14 18:09:31, dmichael wrote:
> I don't see where you check permissions. This should only be available if this
> plugin has Private permission. I think the intent was also that it would only
> work on ChromeOS.
> 
> Here's one example of checking Pepper permissions:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

Thanks for pointing out! Added permission check here, and OS_CHROMEOS check in
isolated_file_system_private_resource.cc.

By the way, should I move IsolatedFileSystem interface from
interfaces_ppb_private_no_permissions.h to interfaces_ppb_private.h? (CRX file
system does not require private permission though).

https://codereview.chromium.org/26803004/diff/863002/chrome/browser/renderer_...
chrome/browser/renderer_host/pepper/pepper_isolated_file_system_message_filter.cc:177:
const std::string fsid =
On 2013/11/14 18:09:31, dmichael wrote:
> nit/suggestion: these could be const& to possibly avoid a copy.

Done.

https://codereview.chromium.org/26803004/diff/863002/content/browser/renderer...
File content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc
(right):

https://codereview.chromium.org/26803004/diff/863002/content/browser/renderer...
content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc:126:
reply_context_ = context->MakeReplyMessageContext();
On 2013/11/14 18:09:31, dmichael wrote:
> Did you change this for any particular reason? Passing it along with the
> callback below is more idiomatic. This probably works fine here, but the reply
> context is unique per message. If you have to handle more than 1 message, each
> has to have its own ReplyMessageContext. Since you should be handling only 1
> message, I think it works okay, but I think it would be better to leave it the
> way it was.

I see. That was just clean-up purpose. Reverted them.

https://codereview.chromium.org/26803004/diff/863002/ppapi/proxy/file_system_...
File ppapi/proxy/file_system_resource.cc (right):

https://codereview.chromium.org/26803004/diff/863002/ppapi/proxy/file_system_...
ppapi/proxy/file_system_resource.cc:106: callback_result_ = params.result();
On 2013/11/14 18:09:31, dmichael wrote:
> Please comment on why you need to save the callback_result_.

Done.

[dmichael (off chromium), 2013-11-15 17:42:44]

lgtm % comments

https://codereview.chromium.org/26803004/diff/1293001/chrome/browser/renderer...
File
chrome/browser/renderer_host/pepper/pepper_isolated_file_system_message_filter.cc
(right):

https://codereview.chromium.org/26803004/diff/1293001/chrome/browser/renderer...
chrome/browser/renderer_host/pepper/pepper_isolated_file_system_message_filter.cc:178:
// Only plugins with private permission can open the filesystem.
nit: feels like this comment would go better after the DCHECK

https://codereview.chromium.org/26803004/diff/1293001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc
(right):

https://codereview.chromium.org/26803004/diff/1293001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc:296:
std::string PepperFileSystemBrowserHost::GeneratePluginId() const {
Can you please make a TODO here to indicate that this is very specialized, and
we would want to make it more general if we ever bring this API to stable? I
think for installed extensions and apps, we'd use their extension ID. (We'd
probably not open this up to PNaCl, so we would always have an extension ID for
untrusted use of the API).

https://codereview.chromium.org/26803004/diff/1293001/ppapi/proxy/isolated_fi...
File ppapi/proxy/isolated_file_system_private_resource.cc (right):

https://codereview.chromium.org/26803004/diff/1293001/ppapi/proxy/isolated_fi...
ppapi/proxy/isolated_file_system_private_resource.cc:51: // PluginPrivate
filesystem is supported only on ChromeOS.
Thanks. I think we want to actually check this on the host side, though.
Messages from the plugin can be forged (especially in NaCl). I don't think
there's a security concern with this particular API, but we shouldn't ever trust
the plugin code.

BTW: Does the backend stuff make any assumptions about it being ChromeOS only?
Justin and I were just talking, and we could arguably make this a stable API
some day. It won't be as useful on other platforms, but we still *could* do it
if the backend supports it.

[nhiroki, 2013-11-18 11:21:16]

Thanks for your comments!

https://codereview.chromium.org/26803004/diff/1293001/chrome/browser/renderer...
File
chrome/browser/renderer_host/pepper/pepper_isolated_file_system_message_filter.cc
(right):

https://codereview.chromium.org/26803004/diff/1293001/chrome/browser/renderer...
chrome/browser/renderer_host/pepper/pepper_isolated_file_system_message_filter.cc:178:
// Only plugins with private permission can open the filesystem.
On 2013/11/15 17:42:44, dmichael wrote:
> nit: feels like this comment would go better after the DCHECK

Done.

https://codereview.chromium.org/26803004/diff/1293001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc
(right):

https://codereview.chromium.org/26803004/diff/1293001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc:296:
std::string PepperFileSystemBrowserHost::GeneratePluginId() const {
On 2013/11/15 17:42:44, dmichael wrote:
> Can you please make a TODO here to indicate that this is very specialized, and
> we would want to make it more general if we ever bring this API to stable?

Added comment.

> I think for installed extensions and apps, we'd use their extension ID. (We'd
> probably not open this up to PNaCl, so we would always have an extension ID
for
> untrusted use of the API).

Hmm... can extensions or apps contain multiple plugins? If so, extension ID
might not be able to make per-plugin isolation.

https://codereview.chromium.org/26803004/diff/1293001/ppapi/proxy/isolated_fi...
File ppapi/proxy/isolated_file_system_private_resource.cc (right):

https://codereview.chromium.org/26803004/diff/1293001/ppapi/proxy/isolated_fi...
ppapi/proxy/isolated_file_system_private_resource.cc:51: // PluginPrivate
filesystem is supported only on ChromeOS.
On 2013/11/15 17:42:44, dmichael wrote:
> Thanks. I think we want to actually check this on the host side, though.
> Messages from the plugin can be forged (especially in NaCl). I don't think
> there's a security concern with this particular API, but we shouldn't ever
trust
> the plugin code.
> 
> BTW: Does the backend stuff make any assumptions about it being ChromeOS only?
> Justin and I were just talking, and we could arguably make this a stable API
> some day. It won't be as useful on other platforms, but we still *could* do it
> if the backend supports it.

Good to know. I considered this closely, and I think the backend does not make
any assumptions about ChromeOS, so we can remove this check (and removed).

[nhiroki, 2013-11-18 11:46:56]

+tsepez@

Hi Tom, can you review these files?

1) ppapi_messages.h
2) pepper_file_system_browser_host.cc (please see GeneratePluginId())

About 2), the function generates a plugin ID from plugin's MIME type string (ex.
"application/x-ppapi-example-plugin"), and the ID will be used for real
directory's name in storage partitions. Can you check this name generator is
safe?

I think this would be safe since the function rejects non-portable characters
(ex. ':', ';'). Additionally this is a private API, so probably an awkward MIME
type is not passed here.

[Tom Sepez, 2013-11-18 19:12:28]

https://codereview.chromium.org/26803004/diff/1613001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc
(right):

https://codereview.chromium.org/26803004/diff/1613001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc:317: for
(std::string::const_iterator it = output.begin();
What if the string is entirely dots, eg. "." or ".."? You probably ought to
check that it starts with an alpha.

[Tom Sepez, 2013-11-18 19:14:45]

content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc:317: for
> (std::string::const_iterator it = output.begin();
> What if the string is entirely dots, eg. "." or ".."? You probably ought to
> check that it starts with an alpha.
Alternatively, you may just want to copy alphanumerics, replacing all others
with an underscore.

[dmichael (off chromium), 2013-11-18 20:37:50]

I realized we should also have tests for this...

https://codereview.chromium.org/26803004/diff/1293001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc
(right):

https://codereview.chromium.org/26803004/diff/1293001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc:296:
std::string PepperFileSystemBrowserHost::GeneratePluginId() const {
> Hmm... can extensions or apps contain multiple plugins? If so, extension ID
> might not be able to make per-plugin isolation.
Oh, good point. I'm not sure; maybe that won't suffice. I'm still a bit fuzzy on
the MIME type solution as well, though. If we ever opened this up to NaCl or
PNaCl apps inside web store apps and extensions, then MIME type wouldn't really
work, since not all of them would even have a MIME type (unless you count
application/nacl and application/pnacl, but that's a useless distinction for
them).

Anyway, the comment seems right for now :-). I'm not sure what this might look
like if we tried to make it public/stable.

[nhiroki, 2013-11-19 12:15:58]

Thank you for reviewing!

I added test cases for the generator.
Can you take another look?

https://codereview.chromium.org/26803004/diff/1613001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc
(right):

https://codereview.chromium.org/26803004/diff/1613001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc:317: for
(std::string::const_iterator it = output.begin();
On 2013/11/18 19:12:29, Tom Sepez wrote:
> What if the string is entirely dots, eg. "." or ".."? You probably ought to
> check that it starts with an alpha.

These cases would be covered by IsMimeType(). It makes sure the given type
starts with "legal_top_level_types" (ex. "application/", "image/") defined in
"net/base/mime_util.cc".

[nhiroki, 2013-11-19 12:23:46]

On 2013/11/18 20:37:50, dmichael wrote:
> I realized we should also have tests for this...
> 
>
https://codereview.chromium.org/26803004/diff/1293001/content/browser/rendere...
> File content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc
> (right):
> 
>
https://codereview.chromium.org/26803004/diff/1293001/content/browser/rendere...
> content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc:296:
> std::string PepperFileSystemBrowserHost::GeneratePluginId() const {
> > Hmm... can extensions or apps contain multiple plugins? If so, extension ID
> > might not be able to make per-plugin isolation.
> Oh, good point. I'm not sure; maybe that won't suffice. I'm still a bit fuzzy
on
> the MIME type solution as well, though.

I think so, too...

> If we ever opened this up to NaCl or
> PNaCl apps inside web store apps and extensions, then MIME type wouldn't
really
> work, since not all of them would even have a MIME type (unless you count
> application/nacl and application/pnacl, but that's a useless distinction for
> them).
> 
> Anyway, the comment seems right for now :-). I'm not sure what this might look
> like if we tried to make it public/stable.

Okay. Let's revisit when we would really need to expose this to public/stable.

[Tom Sepez, 2013-11-19 18:20:38]

lgtm

https://codereview.chromium.org/26803004/diff/1613001/content/browser/rendere...
File content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc
(right):

https://codereview.chromium.org/26803004/diff/1613001/content/browser/rendere...
content/browser/renderer_host/pepper/pepper_file_system_browser_host.cc:317: for
(std::string::const_iterator it = output.begin();
On 2013/11/19 12:15:59, nhiroki wrote:
> On 2013/11/18 19:12:29, Tom Sepez wrote:
> > What if the string is entirely dots, eg. "." or ".."? You probably ought to
> > check that it starts with an alpha.
> 
> These cases would be covered by IsMimeType(). It makes sure the given type
> starts with "legal_top_level_types" (ex. "application/", "image/") defined in
> "net/base/mime_util.cc".
feels frail if this changes down the road, but OK.

[nhiroki, 2013-11-20 11:40:18]

TBRing jochen@ for content_tests.gypi (adding one test file)

[commit-bot: I haz the power, 2013-11-20 11:42:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nhiroki@chromium.org/26803004/1913001

[commit-bot: I haz the power, 2013-11-20 13:44:41]

Change committed as 236241