CRX FileSystem Pepper private API

ppapi/proxy/ppapi_messages.h

Index: ppapi/proxy/ppapi_messages.h
diff --git a/ppapi/proxy/ppapi_messages.h b/ppapi/proxy/ppapi_messages.h
index 78f9edab6f79c1422c7516682c56bd159242ea95..d93ac0f1b94e54cc271949370483ab6b8933c219 100644
--- a/ppapi/proxy/ppapi_messages.h
+++ b/ppapi/proxy/ppapi_messages.h
@@ -1344,6 +1344,17 @@ IPC_MESSAGE_CONTROL2(PpapiHostMsg_ExtensionsCommon_Call,
 IPC_MESSAGE_CONTROL1(PpapiPluginMsg_ExtensionsCommon_CallReply,
                      base::ListValue /* output */)
 
+// Ext_CrxFileSystem
+IPC_MESSAGE_CONTROL0(PpapiHostMsg_Ext_CrxFileSystem_Create)
+IPC_MESSAGE_CONTROL0(PpapiBrowserHostMsg_Ext_CrxFileSystem_Open)
+IPC_MESSAGE_CONTROL2(PpapiPluginMsg_Ext_CrxFileSystem_BrowserOpenReply,
+                     std::string /* extension_id */,
+                     std::string /* fsid */)

[palmer, 2013/04/25 00:01:23]

I don't feel great about this arbitrary string |fsid|. Can we make it come from
a limited set of static strings, or get rid of it? It is ultimately passed down
to the isolated filesystem API, where it is an optional parameter anyway.

[victorhsieh, 2013/04/25 02:21:51]

I guess you're talking about the case that a malicious plugin sends a fake fsid
to renderer host (instead of forwarding what the browser host gets from isolated
filesystem).

kinuko@, the filesystem:chrome-extension:// passed to fileapi can be manipulated
in current implementatoin.  Do you have any suggestion on making this secure
from fileapi's perspective?

[kinuko, 2013/04/25 09:27:16]

Sending an arbitrary fsid doesn't work unless the fsid is registered for the
renderer process ID in the browser process upfront. Also fsid is 128-bit string
(it's like GUID) and I don't think picking an arbitrary fsid could make any
meaningful attack. I'm not an expert in security area though, so please
enlighten me if we seem to need reconsider. Thanks!

[victorhsieh, 2013/04/25 15:41:10]

Here are two cases that I can think of:

1. attacker can brute-forcely scanning other isolated fsid.  In this case, I
think it's not a big problem because when the filesystem is created, it should
always be granted the limited permission in the same renderer.  I'd say the risk
is very low.

2. attacker can give fsid like "../../../../persistent" or something.  But I
guess this will be caught by "cracking"? 

[palmer, 2013/04/25 23:35:51]

First, a question: Isn't the |extension_id| the true separator of extensions
from each other?

What purpose does the |fsid| serve?
kinuko said that |fsid| is a 128-bit GUID. If it is generated cryptographically
randomly, brute-forcing it is infeasible. But I don't see where |fsid| is
generated. For that matter, I don't see code enforcing that the std::string is
GUID-shaped — from what I can see here, it could be anything, and that's why I
don't like it. :) strings are weak, blob types, and for IPC security we prefer
strong, specific types.
This was my fear, too. And kinuko assured me that such parent-references should
be caught by URL parsing. Even so, I have a CL in flight to double-check that
references to the parent directory not work inside the Filesystem API
implementation.

But those two things are less important; first I want to understand why |fsid|
is necessary, and is it expected to perform any security function (such as
isolating two extensions' filesystems from each other).

[kinuko, 2013/04/26 05:58:56]

It's for reusing an existing architecture for CRX directory access and |fsid|
serves as a necessary indirection layer for content/webkit layer FileAPI code
to access the extension specific CRX directory (about which FileAPI layer has
no knowledge about that, because extension or extension ID is chrome-specific
concept).

Instead we can implement a special FS module for CRX directory access in the
browser
process and plug it in webkit layer, but that needs a lot more code and we'll
need yet another security review for the new code, and I didn't like to add new
code
just for this purpose (with an assumption that adding more code usually weakens
the security too).
What's more, just guessing existing |fsid| doesn't work, the renderer
needs to be granted permission for the |fsid| prior. (That means
there's no point to brute-force fsids)
It's GUID-like 128-bit random string, but we're not using GUID.
We can easily change the code to use GenerateGUID() and IsValidGUID() though.

The code generating a new fsid is here, fyi:
https://code.google.com/p/chromium/codesearch#chromium/src/webkit/fileapi/iso...

+IPC_MESSAGE_CONTROL2(PpapiRendererHostMsg_Ext_CrxFileSystem_Open,
+                     std::string /* extension_id */,
+                     std::string /* fsid */)
+IPC_MESSAGE_CONTROL0(PpapiPluginMsg_Ext_CrxFileSystem_RendererOpenReply)
+
 // File chooser.
 IPC_MESSAGE_CONTROL0(PpapiHostMsg_FileChooser_Create)
 IPC_MESSAGE_CONTROL4(PpapiHostMsg_FileChooser_Show,