NaCl: enable meta-based validation for shared libraries.

NaCl: enable meta-based validation for shared libraries.

This is the Chrome-side half of a CL to allow mmaping and skipping validation
for chrome-extension: files we have seen before and know are safe.  To do this
we need to know the path of the file on disk, but we don't entirely trust the
renderer not to tamper with it.  To work around this, a nonce is passed along
with the file handle.  This nonce can be used by the NaCl process to acquire the
file handle directly from the browser process, as well as a fresh copy of the
file handle.

This change significantly revises the OpenNaClExecutable method of the
PPB_NaCl_Private interface.  The method was added anticipation of this CL, but
the overall design shifted after the method was added.

BUG=https://code.google.com/p/chromium/issues/detail?id=224434

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

R=dmichael@chromium.org, jschuh@chromium.org, mseaborn@chromium.org

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

[Nick Bray (chromium), 2013-05-13 20:49:09]

PPAPI OWNER: dmichael@chromium.org
NaCl OWNER: mseaborn@chromium.org
IPC security OWNER: jschuh@chromium.org
FYI: sehr@chromium.org, bbudge@chromium.org

This is the Chrome-side half of a CL to allow mmaping and skipping validation
for chrome-extension: files we have seen before and know are safe.  To do this
we need to know the path of the file on disk, but we don't entirely trust the
renderer not to tamper with it.  To work around this, a nonce is passed along
with the file handle.  This nonce can be used by the NaCl process to acquire the
file handle directly from the browser process, as well as a fresh copy of the
file handle.

This is the NaCl-side CL: https://codereview.chromium.org/15039022/

This CL has been tested using a unified diff of both changes.

[dmichael (off chromium), 2013-05-15 18:37:53]

ppapi stuff looks pretty straightforward, though I don't feel qualified to
comment on the nonce approach. I'm guessing that's already been vetted in
another forum?

https://codereview.chromium.org/14750007/diff/1/ppapi/native_client/src/trust...
File ppapi/native_client/src/trusted/plugin/file_downloader.cc (right):

https://codereview.chromium.org/14750007/diff/1/ppapi/native_client/src/trust...
ppapi/native_client/src/trusted/plugin/file_downloader.cc:177: return info;
I find these error returns less obvious now. Not sure of a nicer way to do it.
You could maybe do:
return { NACL_NO_FILE_DESC, nonce };
? That has the downside that NaClFileInfo can only change by appending, though.

https://codereview.chromium.org/14750007/diff/1/ppapi/native_client/src/trust...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

https://codereview.chromium.org/14750007/diff/1/ppapi/native_client/src/trust...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:179: &error_info,
&op_complete);
I know this isn't your fault, but this seems to warrant a comment to me: to_open
is made on the heap, but has pointers to stack variables (and |info| which I
guess comes from SRPC?). It appears that to_open will be deleted in the
continuation, and that must happen in another thread before this function exits.
So I think it's all OK, but it looks fishy at first glance.

https://codereview.chromium.org/14750007/diff/1/ppapi/native_client/src/trust...
File ppapi/native_client/src/trusted/plugin/service_runtime.h (right):

https://codereview.chromium.org/14750007/diff/1/ppapi/native_client/src/trust...
ppapi/native_client/src/trusted/plugin/service_runtime.h:72:
file_info(file_info),
please use distinct names for the parameter and the member.

[bennet.yee, 2013-05-16 01:52:48]

https://codereview.chromium.org/14750007/diff/1/chrome/browser/nacl_host/nacl...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/14750007/diff/1/chrome/browser/nacl_host/nacl...
chrome/browser/nacl_host/nacl_process_host.cc:961: if
(!NaClBrowser::GetInstance()->GetFilePath(nonce, &file_path)){
this should never happen in normal operation -- and is a good signal that the
renderer and/or the NaCl module was compromised.  perhaps logging something? 
kill either the renderer or the NaCl module, or both?  justin: any advice?

[Nick Bray (chromium), 2013-05-16 16:38:17]

PTAL

https://codereview.chromium.org/14750007/diff/1/chrome/browser/nacl_host/nacl...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/14750007/diff/1/chrome/browser/nacl_host/nacl...
chrome/browser/nacl_host/nacl_process_host.cc:961: if
(!NaClBrowser::GetInstance()->GetFilePath(nonce, &file_path)){
On 2013/05/16 01:52:48, bennet.yee wrote:
> this should never happen in normal operation -- and is a good signal that the
> renderer and/or the NaCl module was compromised.  perhaps logging something? 
> kill either the renderer or the NaCl module, or both?  justin: any advice?

This could happen someone launches 10+ NaCl modules at once.  The file path
cache has a limited size, and it will evict the oldest entry.  Queries for
evicted entries will fail gracefully.  Added a comment.

https://codereview.chromium.org/14750007/diff/1/ppapi/native_client/src/trust...
File ppapi/native_client/src/trusted/plugin/file_downloader.cc (right):

https://codereview.chromium.org/14750007/diff/1/ppapi/native_client/src/trust...
ppapi/native_client/src/trusted/plugin/file_downloader.cc:177: return info;
On 2013/05/15 18:37:53, dmichael wrote:
> I find these error returns less obvious now. Not sure of a nicer way to do it.
> You could maybe do:
> return { NACL_NO_FILE_DESC, nonce };
> ? That has the downside that NaClFileInfo can only change by appending,
though.

The file info structure is defined in the NaCl repo, so I want to avoid any
assumptions about the layout.  I get what you're saying and considered it
myself, but didn't see a better way.

https://codereview.chromium.org/14750007/diff/1/ppapi/native_client/src/trust...
File ppapi/native_client/src/trusted/plugin/service_runtime.h (right):

https://codereview.chromium.org/14750007/diff/1/ppapi/native_client/src/trust...
ppapi/native_client/src/trusted/plugin/service_runtime.h:72:
file_info(file_info),
On 2013/05/15 18:37:53, dmichael wrote:
> please use distinct names for the parameter and the member.

Done.

[dmichael (off chromium), 2013-05-16 17:08:35]

https://codereview.chromium.org/14750007/diff/1/ppapi/native_client/src/trust...
File ppapi/native_client/src/trusted/plugin/file_downloader.cc (right):

https://codereview.chromium.org/14750007/diff/1/ppapi/native_client/src/trust...
ppapi/native_client/src/trusted/plugin/file_downloader.cc:177: return info;
On 2013/05/16 16:38:17, Nick Bray (chromium) wrote:
> On 2013/05/15 18:37:53, dmichael wrote:
> > I find these error returns less obvious now. Not sure of a nicer way to do
it.
> > You could maybe do:
> > return { NACL_NO_FILE_DESC, nonce };
> > ? That has the downside that NaClFileInfo can only change by appending,
> though.
> 
> The file info structure is defined in the NaCl repo, so I want to avoid any
> assumptions about the layout.  I get what you're saying and considered it
> myself, but didn't see a better way.
How about a simple little helper function in the unnamed namespace, and do:
MakeNaClFileInfo(NACL_NO_FILE_DESC, nonce);
here.
MakeNaClFileInfo would do the memset and assign those fields without making
assumptions about layout.

Or if you have any better ideas, I'm open. I would just like it to be super
obvious that these branches are error conditions somehow.

https://codereview.chromium.org/14750007/diff/1/ppapi/native_client/src/trust...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

https://codereview.chromium.org/14750007/diff/1/ppapi/native_client/src/trust...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:179: &error_info,
&op_complete);
On 2013/05/15 18:37:53, dmichael wrote:
> I know this isn't your fault, but this seems to warrant a comment to me:
to_open
> is made on the heap, but has pointers to stack variables (and |info| which I
> guess comes from SRPC?). It appears that to_open will be deleted in the
> continuation, and that must happen in another thread before this function
exits.
> So I think it's all OK, but it looks fishy at first glance.
Could you comment this a little, pretty please? The way this method works by
jumping threads and shuffling pointers around is unusual and a bit tricky. It
would be helpful to have a high-level comment about what it's doing (which will
also hopefully explain to the reader why it's okay to pass stack pointers to
this heap object).

[Nick Bray (chromium), 2013-05-16 17:44:15]

PTAL

https://codereview.chromium.org/14750007/diff/1/ppapi/native_client/src/trust...
File ppapi/native_client/src/trusted/plugin/file_downloader.cc (right):

https://codereview.chromium.org/14750007/diff/1/ppapi/native_client/src/trust...
ppapi/native_client/src/trusted/plugin/file_downloader.cc:177: return info;
On 2013/05/16 17:08:35, dmichael wrote:
> On 2013/05/16 16:38:17, Nick Bray (chromium) wrote:
> > On 2013/05/15 18:37:53, dmichael wrote:
> > > I find these error returns less obvious now. Not sure of a nicer way to do
> it.
> > > You could maybe do:
> > > return { NACL_NO_FILE_DESC, nonce };
> > > ? That has the downside that NaClFileInfo can only change by appending,
> > though.
> > 
> > The file info structure is defined in the NaCl repo, so I want to avoid any
> > assumptions about the layout.  I get what you're saying and considered it
> > myself, but didn't see a better way.
> How about a simple little helper function in the unnamed namespace, and do:
> MakeNaClFileInfo(NACL_NO_FILE_DESC, nonce);
> here.
> MakeNaClFileInfo would do the memset and assign those fields without making
> assumptions about layout.
> 
> Or if you have any better ideas, I'm open. I would just like it to be super
> obvious that these branches are error conditions somehow.

Didn't think of that one.  Good idea.  Done.

https://codereview.chromium.org/14750007/diff/1/ppapi/native_client/src/trust...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

https://codereview.chromium.org/14750007/diff/1/ppapi/native_client/src/trust...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:179: &error_info,
&op_complete);
On 2013/05/16 17:08:35, dmichael wrote:
> On 2013/05/15 18:37:53, dmichael wrote:
> > I know this isn't your fault, but this seems to warrant a comment to me:
> to_open
> > is made on the heap, but has pointers to stack variables (and |info| which I
> > guess comes from SRPC?). It appears that to_open will be deleted in the
> > continuation, and that must happen in another thread before this function
> exits.
> > So I think it's all OK, but it looks fishy at first glance.
> Could you comment this a little, pretty please? The way this method works by
> jumping threads and shuffling pointers around is unusual and a bit tricky. It
> would be helpful to have a high-level comment about what it's doing (which
will
> also hopefully explain to the reader why it's okay to pass stack pointers to
> this heap object).

Done.

[dmichael (off chromium), 2013-05-16 18:06:38]

ppapi/ lgtm

[Mark Seaborn, 2013-05-16 23:01:47]

> This is the Chrome-side half of a CL to allow mmaping and
> skipping validation for chrome-extension: files we have seen
> before and know are safe.  To do this we need to know the path
> of the file on disk, but we don't entirely trust the renderer
> not to tamper with it.  To work around this, a nonce is passed
> along with the file handle.  This nonce can be used by the NaCl
> process to acquire the file handle directly from the browser
> process, as well as a fresh copy of the file handle.

Please put the description above into the commit message.

Also, is there a canonical place that the whole nonce/token scheme can be
explained?  Maybe explain it next to PutFilePath()/GetFilePath() in
nacl_browser.h?

In particular, explain the attack scenario you're trying to prevent, which is:
 * Compromised renderer compromises a NaCl process and gets it to put bad
results into the NaCl validation cache.  This then allows a web page (maybe from
an uncompromised renderer) to run a NaCl nexe which can escape from the NaCl
sandbox.  (But isn't this scenario possible with the hash-based validation
caching anyway?)

The attack you're potentially opening up is:
 * Compromised NaCl process guesses nonce (file token) used for another NaCl
process (running under a different origin) and thereby reads data from the other
origin.

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_browser.cc (right):

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.cc:117: // allow a sandbox escape.
"NaCl inner sandbox escape"?

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.cc:122: base::PLATFORM_FILE_READ |
Maybe format as:
(FLAG |
 FLAG |
 FLAG)

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.cc:131: // to an extension directory could
allow a sandbox escape.
Same here - which sandbox?  Probably not the inner sandbox this time, but
certainly some Linux outer sandboxes.

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.cc:134: {
Put on previous line

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.cc:156: path_cache_(10),
From discussion in person:  10 is the number of requests that can be in progress
at once.  If you have 11 requests in progress simultaneously, some can fail.  So
this isn't a cache -- cache misses shouldn't affect correctness.

So you either need to fix that, or put in a big TODO and explanation here of the
meaning of 10.  And don't call it "path_cache_". :-)

Why is this limit set so low when you're only mapping IDs -> filenames?  It's
not like this is holding on to file descriptors.

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.cc:413: nonce = base::RandUint64();
I'm not sure 'nonce' is the appropriate term here, since it suggests the number
is unguessable, that this is a cryptographic protocol, etc., which isn't the
case.

Something like "file_token" instead, perhaps?

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.cc:416: if (nonce != 0) {
Has the nonce == 0 code path ever been executed? :-)

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_browser.h (right):

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.h:21: // Open an immutable executable file
that can be mmaped.
"mmapped"

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.h:22: // This function should only be
called on a tread that can perform file IO.
"thread"

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.h:84: bool GetFilePath(uint64 nonce,
base::FilePath *path);
"* " spacing

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_file_host.cc (right):

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_file_host.cc:134: void
DoOpenNaClExecutableRegister(
Not sure I understand the name, since it suggests "executable register" is a
noun phrase.  "RegisterNaClExecutableFile"?

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_file_host.cc:226: base::Bind(
Why does this need a PostTask() now?  Is the CreatePlatformFile() call being
moved to a different thread?  If so, why?

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:938: 
Remove empty line at function start

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:962: // resolving the nonce may
fail because the path cache was thrashed while the
As I said in the other comment, this is bad, so you should at least have a TODO
to fix this.

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:966: if
(!NaClBrowser::GetInstance()->GetFilePath(nonce, &file_path)){
Add space: ") {"

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:967:
NaClProcessMsg_ResolveFileNonce::WriteReplyParams(
Wrong indentation here (it's using 2+3 instead of 2+2)

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:980: FROM_HERE,
Indent the function's arguments

https://codereview.chromium.org/14750007/diff/27001/chrome/common/nacl_messag...
File chrome/common/nacl_messages.h (right):

https://codereview.chromium.org/14750007/diff/27001/chrome/common/nacl_messag...
chrome/common/nacl_messages.h:82: // Used by the NaCl process to aquire trusted
information about a file.
"acquire".

Could you be more specific about what "trusted information about a file" means? 
Also, this isn't just getting information about the file, it's getting an
FD/handle.

https://codereview.chromium.org/14750007/diff/27001/chrome/common/nacl_messag...
chrome/common/nacl_messages.h:84: uint64, /* nonce */
"file token"?  Please make it clear that this value identifies a file.  On its
own, "nonce" doesn't do that.

https://codereview.chromium.org/14750007/diff/27001/chrome/common/render_mess...
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/14750007/diff/27001/chrome/common/render_mess...
chrome/common/render_messages.h:599: uint64_t /* nonce */)
Ditto: "file token"

https://codereview.chromium.org/14750007/diff/27001/chrome/nacl/nacl_listener.cc
File chrome/nacl/nacl_listener.cc (right):

https://codereview.chromium.org/14750007/diff/27001/chrome/nacl/nacl_listener...
chrome/nacl/nacl_listener.cc:157: *path = ipc_path.AsUTF8Unsafe();
Maybe comment why this is safe here, despite the name (assuming it is actually
safe?).

https://codereview.chromium.org/14750007/diff/27001/chrome/nacl/nacl_validati...
File chrome/nacl/nacl_validation_query.cc (right):

https://codereview.chromium.org/14750007/diff/27001/chrome/nacl/nacl_validati...
chrome/nacl/nacl_validation_query.cc:135: static int ResolveFileNonce(void*
handle, uint64 nonce, int32* fd,
So you're returning absolute filenames to the NaCl process.  This means that
you'll reveal pathnames like "/home/mseaborn/..." to the NaCl process.

That isn't ideal, although env vars are currently inherited by the NaCl process
which expose similar information.  But it would be nice not to add to the set of
things that are exposed this way.

Could you pass a hash of the filename to the NaCl process instead?

https://codereview.chromium.org/14750007/diff/27001/chrome/nacl/nacl_validati...
chrome/nacl/nacl_validation_query.cc:143: *file_path = (char *)
malloc(path.length() + 1);
Use C++-style casts?

https://codereview.chromium.org/14750007/diff/27001/chrome/nacl/nacl_validati...
chrome/nacl/nacl_validation_query.cc:147: *file_path_length = (uint32)
path.length();
Ditto

https://codereview.chromium.org/14750007/diff/27001/ppapi/c/private/ppb_nacl_...
File ppapi/c/private/ppb_nacl_private.h (left):

https://codereview.chromium.org/14750007/diff/27001/ppapi/c/private/ppb_nacl_...
ppapi/c/private/ppb_nacl_private.h:63: struct PP_NaClExecutableMetadata {
Explain in commit message that this was a not-fully-baked interface that is
being removed.

https://codereview.chromium.org/14750007/diff/27001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/file_downloader.cc (right):

https://codereview.chromium.org/14750007/diff/27001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/file_downloader.cc:24: namespace {
Add empty line after this

https://codereview.chromium.org/14750007/diff/27001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/file_downloader.cc:28: struct
NaClFileInfo NoFileInfo() {
'struct NaClFileInfo' -> 'NaClFileInfo' in C++?  Same below.

https://codereview.chromium.org/14750007/diff/27001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/14750007/diff/27001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/plugin.cc:1326: memset(&info, 0,
sizeof(info));
You could instead do:
struct NaClFileInfo info = {0};
(sometimes depending on warning settings)

https://codereview.chromium.org/14750007/diff/27001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

https://codereview.chromium.org/14750007/diff/27001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:327: // TODO(ncbray):
more info for faster validation?
I'm not sure what this TODO means

[Nick Bray (chromium), 2013-05-21 20:09:05]

Moved to 128-bit nonce, PTAL.

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_browser.cc (right):

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.cc:117: // allow a sandbox escape.
On 2013/05/16 23:01:47, Mark Seaborn wrote:
> "NaCl inner sandbox escape"?

Done.

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.cc:122: base::PLATFORM_FILE_READ |
On 2013/05/16 23:01:47, Mark Seaborn wrote:
> Maybe format as:
> (FLAG |
>  FLAG |
>  FLAG)

Done.

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.cc:131: // to an extension directory could
allow a sandbox escape.
On 2013/05/16 23:01:47, Mark Seaborn wrote:
> Same here - which sandbox?  Probably not the inner sandbox this time, but
> certainly some Linux outer sandboxes.

Done.

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.cc:134: {
On 2013/05/16 23:01:47, Mark Seaborn wrote:
> Put on previous line

Done.

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.cc:156: path_cache_(10),
On 2013/05/16 23:01:47, Mark Seaborn wrote:
> From discussion in person:  10 is the number of requests that can be in
progress
> at once.  If you have 11 requests in progress simultaneously, some can fail. 
So
> this isn't a cache -- cache misses shouldn't affect correctness.
> 
> So you either need to fix that, or put in a big TODO and explanation here of
the
> meaning of 10.  And don't call it "path_cache_". :-)
> 
> Why is this limit set so low when you're only mapping IDs -> filenames?  It's
> not like this is holding on to file descriptors.

Done, modulo being able to think of a better name.

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.cc:413: nonce = base::RandUint64();
On 2013/05/16 23:01:47, Mark Seaborn wrote:
> I'm not sure 'nonce' is the appropriate term here, since it suggests the
number
> is unguessable, that this is a cryptographic protocol, etc., which isn't the
> case.
> 
> Something like "file_token" instead, perhaps?

Moving to 128 bits and Justin's request, which implies it is a nonce.

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.cc:416: if (nonce != 0) {
On 2013/05/16 23:01:47, Mark Seaborn wrote:
> Has the nonce == 0 code path ever been executed? :-)

It may be.  Some day.  We should add a prize.  On the other hand, once it's
moved to 128 bits, probably not.

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_file_host.cc (right):

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_file_host.cc:134: void
DoOpenNaClExecutableRegister(
On 2013/05/16 23:01:47, Mark Seaborn wrote:
> Not sure I understand the name, since it suggests "executable register" is a
> noun phrase.  "RegisterNaClExecutableFile"?

Done.

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_file_host.cc:226: base::Bind(
On 2013/05/16 23:01:47, Mark Seaborn wrote:
> Why does this need a PostTask() now?  Is the CreatePlatformFile() call being
> moved to a different thread?  If so, why?

Done.

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:938: 
On 2013/05/16 23:01:47, Mark Seaborn wrote:
> Remove empty line at function start

Done.

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:962: // resolving the nonce may
fail because the path cache was thrashed while the
On 2013/05/16 23:01:47, Mark Seaborn wrote:
> As I said in the other comment, this is bad, so you should at least have a
TODO
> to fix this.

The cache is big enough this shouldn't happen in practice, this behavior is not
particularly bad, and there isn't a good alternative, given architectural
constraints.  A TODO would just be clutter.

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:966: if
(!NaClBrowser::GetInstance()->GetFilePath(nonce, &file_path)){
On 2013/05/16 23:01:47, Mark Seaborn wrote:
> Add space: ") {"

Done.

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:967:
NaClProcessMsg_ResolveFileNonce::WriteReplyParams(
On 2013/05/16 23:01:47, Mark Seaborn wrote:
> Wrong indentation here (it's using 2+3 instead of 2+2)

Done.

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:980: FROM_HERE,
On 2013/05/16 23:01:47, Mark Seaborn wrote:
> Indent the function's arguments

They are.  Four space indent.  Which is ambiguous with the bracket alignment, in
this case.  If you have a better suggestion, please be explicit.

https://codereview.chromium.org/14750007/diff/27001/chrome/common/nacl_messag...
File chrome/common/nacl_messages.h (right):

https://codereview.chromium.org/14750007/diff/27001/chrome/common/nacl_messag...
chrome/common/nacl_messages.h:82: // Used by the NaCl process to aquire trusted
information about a file.
On 2013/05/16 23:01:47, Mark Seaborn wrote:
> "acquire".
> 
> Could you be more specific about what "trusted information about a file"
means? 
> Also, this isn't just getting information about the file, it's getting an
> FD/handle.

Done.

https://codereview.chromium.org/14750007/diff/27001/chrome/nacl/nacl_listener.cc
File chrome/nacl/nacl_listener.cc (right):

https://codereview.chromium.org/14750007/diff/27001/chrome/nacl/nacl_listener...
chrome/nacl/nacl_listener.cc:157: *path = ipc_path.AsUTF8Unsafe();
On 2013/05/16 23:01:47, Mark Seaborn wrote:
> Maybe comment why this is safe here, despite the name (assuming it is actually
> safe?).

Done.

https://codereview.chromium.org/14750007/diff/27001/chrome/nacl/nacl_validati...
File chrome/nacl/nacl_validation_query.cc (right):

https://codereview.chromium.org/14750007/diff/27001/chrome/nacl/nacl_validati...
chrome/nacl/nacl_validation_query.cc:135: static int ResolveFileNonce(void*
handle, uint64 nonce, int32* fd,
On 2013/05/16 23:01:47, Mark Seaborn wrote:
> So you're returning absolute filenames to the NaCl process.  This means that
> you'll reveal pathnames like "/home/mseaborn/..." to the NaCl process.
> 
> That isn't ideal, although env vars are currently inherited by the NaCl
process
> which expose similar information.  But it would be nice not to add to the set
of
> things that are exposed this way.
> 
> Could you pass a hash of the filename to the NaCl process instead?

Security team is not worried, and I have too much to deal with already.  The
interface does not preclude changing this later.

https://codereview.chromium.org/14750007/diff/27001/chrome/nacl/nacl_validati...
chrome/nacl/nacl_validation_query.cc:143: *file_path = (char *)
malloc(path.length() + 1);
On 2013/05/16 23:01:47, Mark Seaborn wrote:
> Use C++-style casts?

Done.

https://codereview.chromium.org/14750007/diff/27001/chrome/nacl/nacl_validati...
chrome/nacl/nacl_validation_query.cc:147: *file_path_length = (uint32)
path.length();
On 2013/05/16 23:01:47, Mark Seaborn wrote:
> Ditto

Done.

https://codereview.chromium.org/14750007/diff/27001/ppapi/c/private/ppb_nacl_...
File ppapi/c/private/ppb_nacl_private.h (left):

https://codereview.chromium.org/14750007/diff/27001/ppapi/c/private/ppb_nacl_...
ppapi/c/private/ppb_nacl_private.h:63: struct PP_NaClExecutableMetadata {
On 2013/05/16 23:01:47, Mark Seaborn wrote:
> Explain in commit message that this was a not-fully-baked interface that is
> being removed.

Done.

https://codereview.chromium.org/14750007/diff/27001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/file_downloader.cc (right):

https://codereview.chromium.org/14750007/diff/27001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/file_downloader.cc:24: namespace {
On 2013/05/16 23:01:47, Mark Seaborn wrote:
> Add empty line after this

Done.

https://codereview.chromium.org/14750007/diff/27001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/file_downloader.cc:28: struct
NaClFileInfo NoFileInfo() {
On 2013/05/16 23:01:47, Mark Seaborn wrote:
> 'struct NaClFileInfo' -> 'NaClFileInfo' in C++?  Same below.

Staying consistent with the C code base.

https://codereview.chromium.org/14750007/diff/27001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/14750007/diff/27001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/plugin.cc:1326: memset(&info, 0,
sizeof(info));
On 2013/05/16 23:01:47, Mark Seaborn wrote:
> You could instead do:
> struct NaClFileInfo info = {0};
> (sometimes depending on warning settings)

Rather not risk the wrath of compiler skew.

https://codereview.chromium.org/14750007/diff/27001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

https://codereview.chromium.org/14750007/diff/27001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:327: // TODO(ncbray):
more info for faster validation?
On 2013/05/16 23:01:47, Mark Seaborn wrote:
> I'm not sure what this TODO means

Done.

[jschuh, 2013-05-23 01:02:32]

https://codereview.chromium.org/14750007/diff/73001/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/14750007/diff/73001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:967: &file_path)) {
This seems like unambiguously bad occurrence. At a minimum we should have a
NOTREACHED, but maybe we should actually terminate the child process?

[Nick Bray (chromium), 2013-05-23 16:44:11]

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_browser.h (right):

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.h:21: // Open an immutable executable file
that can be mmaped.
On 2013/05/16 23:01:47, Mark Seaborn wrote:
> "mmapped"

Done.

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.h:22: // This function should only be
called on a tread that can perform file IO.
On 2013/05/16 23:01:47, Mark Seaborn wrote:
> "thread"

Done.

https://codereview.chromium.org/14750007/diff/27001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.h:84: bool GetFilePath(uint64 nonce,
base::FilePath *path);
On 2013/05/16 23:01:47, Mark Seaborn wrote:
> "* " spacing

Done.

https://codereview.chromium.org/14750007/diff/27001/chrome/common/nacl_messag...
File chrome/common/nacl_messages.h (right):

https://codereview.chromium.org/14750007/diff/27001/chrome/common/nacl_messag...
chrome/common/nacl_messages.h:84: uint64, /* nonce */
On 2013/05/16 23:01:47, Mark Seaborn wrote:
> "file token"?  Please make it clear that this value identifies a file.  On its
> own, "nonce" doesn't do that.

Done.

https://codereview.chromium.org/14750007/diff/27001/chrome/common/render_mess...
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/14750007/diff/27001/chrome/common/render_mess...
chrome/common/render_messages.h:599: uint64_t /* nonce */)
On 2013/05/16 23:01:47, Mark Seaborn wrote:
> Ditto: "file token"

Done.

https://codereview.chromium.org/14750007/diff/73001/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/14750007/diff/73001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:967: &file_path)) {
On 2013/05/23 01:02:32, Justin Schuh wrote:
> This seems like unambiguously bad occurrence. At a minimum we should have a
> NOTREACHED, but maybe we should actually terminate the child process?

See the above comment.  Bounding the cache size implies that it may be valid for
the query to fail. :/

[jschuh, 2013-05-23 16:47:03]

okay. ipc security lgtm.

[bsy_cr, 2013-05-23 17:57:24]

https://codereview.chromium.org/14750007/diff/73001/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/14750007/diff/73001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:967: &file_path)) {
On 2013/05/23 16:44:11, Nick Bray (chromium) wrote:
> On 2013/05/23 01:02:32, Justin Schuh wrote:
> > This seems like unambiguously bad occurrence. At a minimum we should have a
> > NOTREACHED, but maybe we should actually terminate the child process?
> 
> See the above comment.  Bounding the cache size implies that it may be valid
for
> the query to fail. :/

Just because it's valid doesn't mean it's likely.  We should do (upper bound)
estimates of number of tabs -- hundreds? -- that might re-open when the browser
restarts, and the percentage of them that might involve NaCl -- 5%? 10%? -- and
the number of manifest entries -- 64? 128? -- to get an estimate for when the
cache might hit size limits.  Assume worse case behavior (not serial startup),
etc.

Furthermore, since the cache doesn't actually hold file descriptors (limited
global resources) any more and just hold file tokens, that argument for keeping
the cache really small no longer apply.  The only limit is memory, albeit still
a limited resource, but much less limiting.  So, I'd like to see at least a
ballpark analysis/discussion for the proper size for the cache, its cost in
memory usage, versus the likelihood of a valid cache miss.  If the probability
of a valid cache miss is low, then this could be a useful signal.  Even if it is
not super low, a sharp spike in the cache miss rate is a signal that there might
be something "interesting" going on in the wild.

Could we have UMA or other feedback statistics for this?  At least a comment
about the considerations, please.

[Nick Bray (chromium), 2013-05-23 23:29:26]

ping

[Nick Bray (chromium), 2013-05-24 16:54:27]

https://codereview.chromium.org/14750007/diff/73001/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/14750007/diff/73001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:967: &file_path)) {
On 2013/05/23 17:57:24, bsy_cr wrote:
> On 2013/05/23 16:44:11, Nick Bray (chromium) wrote:
> > On 2013/05/23 01:02:32, Justin Schuh wrote:
> > > This seems like unambiguously bad occurrence. At a minimum we should have
a
> > > NOTREACHED, but maybe we should actually terminate the child process?
> > 
> > See the above comment.  Bounding the cache size implies that it may be valid
> for
> > the query to fail. :/
> 
> Just because it's valid doesn't mean it's likely.  We should do (upper bound)
> estimates of number of tabs -- hundreds? -- that might re-open when the
browser
> restarts, and the percentage of them that might involve NaCl -- 5%? 10%? --
and
> the number of manifest entries -- 64? 128? -- to get an estimate for when the
> cache might hit size limits.  Assume worse case behavior (not serial startup),
> etc.
> 
> Furthermore, since the cache doesn't actually hold file descriptors (limited
> global resources) any more and just hold file tokens, that argument for
keeping
> the cache really small no longer apply.  The only limit is memory, albeit
still
> a limited resource, but much less limiting.  So, I'd like to see at least a
> ballpark analysis/discussion for the proper size for the cache, its cost in
> memory usage, versus the likelihood of a valid cache miss.  If the probability
> of a valid cache miss is low, then this could be a useful signal.  Even if it
is
> not super low, a sharp spike in the cache miss rate is a signal that there
might
> be something "interesting" going on in the wild.
> 
> Could we have UMA or other feedback statistics for this?  At least a comment
> about the considerations, please.

Done.

[Mark Seaborn, 2013-05-24 20:21:57]

LGTM with the changes below.  I don't know this aspect of
ppapi/native_client/src/trusted/plugin so well, so I think you should get
someone else to sign off on that part.

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_browser.cc (right):

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.cc:42: // other hand, entries are not
always claimed, so the size of the cache will
Say "claimed (and hence removed)"?  Otherwise it's not clear what "claimed"
means.

Why aren't entries always claimed?

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.cc:137: // to an extension directory could
allow an outer sandbox escape. openat
"openat" -> "openat()" to be clearer what you're referring to?

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.cc:417: void
NaClBrowser::PutFilePath(const base::FilePath& path, uint64 *file_token_lo,
Fix "*" spacing style

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.cc:418: uint64 *file_token_hi) {
Fix indentation

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.cc:425: std::string
key(reinterpret_cast<char *>(file_token), sizeof(file_token));
Spacing style should be "char*"

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.cc:440: std::string
key(reinterpret_cast<char *>(file_token), sizeof(file_token));
Spacing style should be "char*"

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.cc:441: PathCacheType::iterator iter  =
path_cache_.Peek(key);
Fix spacing (you have 2 spaces before '=')

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_browser.h (right):

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.h:87: // trusted the handle was read only
but it was not, a mmaped file could
"an mmapped"

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.h:88: // mutated after validation,
allowing an escape of the NaCl sandbox.
"could be mutated" -- or better, "could be modified"

"escape of" -> "escape from"?

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.h:92: // Instead of allowing these
attacks, we only trust information we get
"we" -> "the NaCl process"

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.h:101: // TODO(ncbray): move the cache
onto the NaCl process host to completely
"onto the NaCl process host" -> "into NaClProcessHost"

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.h:102: // prevent one process from
guessing the token of another process.
Nit: it doesn't prevent guessing, it prevents guessing from having any
consequence.  "So that we don't rely on file tokens being unguessable"?

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:959: // Was the file registered?
Nit: add empty line after each comment paragraph for readability.

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:965: // However: each NaCl process
will consume 2-3 entries as it starts up, this
Why 2-3?  Explain more in comment, perhaps?

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:990: FROM_HERE,
Indent arguments properly.  i.e.:

// Wrong
if (some_function(
    arg1)) ...

// Right
if (some_function(
        arg1)) ...

https://codereview.chromium.org/14750007/diff/79001/chrome/common/nacl_messag...
File chrome/common/nacl_messages.h (right):

https://codereview.chromium.org/14750007/diff/79001/chrome/common/nacl_messag...
chrome/common/nacl_messages.h:82: // Used by the NaCl process to aquire trusted
information about a file directly
"acquire"

https://codereview.chromium.org/14750007/diff/79001/chrome/common/nacl_messag...
chrome/common/nacl_messages.h:85:
IPC_SYNC_MESSAGE_CONTROL2_2(NaClProcessMsg_ResolveFileToken,
"Resolve" is vague.  How about "OpenFileFromToken" or "OpenFileGivenToken"?

(I know you already used the term "Resolve" in the NaCl code, but I still think
it would be better to use "Open" here.)

https://codereview.chromium.org/14750007/diff/79001/chrome/nacl/nacl_listener.cc
File chrome/nacl/nacl_listener.cc (right):

https://codereview.chromium.org/14750007/diff/79001/chrome/nacl/nacl_listener...
chrome/nacl/nacl_listener.cc:134: virtual bool ResolveFileToken(struct
NaClFileToken *file_token,
Fix "*" spacing style

https://codereview.chromium.org/14750007/diff/79001/chrome/nacl/nacl_listener...
chrome/nacl/nacl_listener.cc:160: // It doesn't matter if the path is valid UTF8
as long as it's repeatable
Do you mean "It doesn't matter if the path is invalid UTF8"?

Not sure what you mean by repeatable here.

https://codereview.chromium.org/14750007/diff/79001/chrome/nacl/nacl_validati...
File chrome/nacl/nacl_validation_db.h (right):

https://codereview.chromium.org/14750007/diff/79001/chrome/nacl/nacl_validati...
chrome/nacl/nacl_validation_db.h:21: virtual bool ResolveFileToken(struct
NaClFileToken *file_token,
Fix "*" spacing style

https://codereview.chromium.org/14750007/diff/79001/chrome/nacl/nacl_validati...
File chrome/nacl/nacl_validation_query.cc (right):

https://codereview.chromium.org/14750007/diff/79001/chrome/nacl/nacl_validati...
chrome/nacl/nacl_validation_query.cc:34: struct NaClFileToken *file_token,
Fix "*" spacing style

https://codereview.chromium.org/14750007/diff/79001/chrome/nacl/nacl_validati...
chrome/nacl/nacl_validation_query.cc:140: uint32 *file_path_length) {
Fix "*" spacing style

https://codereview.chromium.org/14750007/diff/79001/chrome/nacl/nacl_validati...
chrome/nacl/nacl_validation_query.cc:147: *file_path = static_cast<char
*>(malloc(path.length() + 1));
Style is "char*"

https://codereview.chromium.org/14750007/diff/79001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/14750007/diff/79001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/plugin.cc:1327: std::map<nacl::string,
struct NaClFileInfo>::iterator it =
As an aside, having this URL -> FD mapping seems to be wrong because:

 * It might be leaking FDs if entries are never removed
 * It might mean that two open() calls return the same FD, with the seek
position being shared.
 * It might mean that a fresh open() call doesn't refetch the URL (which might
be wrong if the resource is changeable).

I don't know the scope of this.  Can you file an issue (with the info above),
please?

https://codereview.chromium.org/14750007/diff/79001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

https://codereview.chromium.org/14750007/diff/79001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:174: struct
NaClFileInfo *info) {
Fix "*" spacing style

https://codereview.chromium.org/14750007/diff/79001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/service_runtime.h (right):

https://codereview.chromium.org/14750007/diff/79001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/service_runtime.h:76: struct NaClFileInfo
*file_info;
Fix "*" spacing style

[Nick Bray (chromium), 2013-05-24 21:35:23]

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_browser.cc (right):

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.cc:42: // other hand, entries are not
always claimed, so the size of the cache will
On 2013/05/24 20:21:58, Mark Seaborn wrote:
> Say "claimed (and hence removed)"?  Otherwise it's not clear what "claimed"
> means.
> 
> Why aren't entries always claimed?

Done.

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.cc:137: // to an extension directory could
allow an outer sandbox escape. openat
On 2013/05/24 20:21:58, Mark Seaborn wrote:
> "openat" -> "openat()" to be clearer what you're referring to?

Done.

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.cc:417: void
NaClBrowser::PutFilePath(const base::FilePath& path, uint64 *file_token_lo,
On 2013/05/24 20:21:58, Mark Seaborn wrote:
> Fix "*" spacing style

Done.

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.cc:418: uint64 *file_token_hi) {
On 2013/05/24 20:21:58, Mark Seaborn wrote:
> Fix indentation

Done.

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.cc:425: std::string
key(reinterpret_cast<char *>(file_token), sizeof(file_token));
On 2013/05/24 20:21:58, Mark Seaborn wrote:
> Spacing style should be "char*"

Done.

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.cc:440: std::string
key(reinterpret_cast<char *>(file_token), sizeof(file_token));
On 2013/05/24 20:21:58, Mark Seaborn wrote:
> Spacing style should be "char*"

Done.

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.cc:441: PathCacheType::iterator iter  =
path_cache_.Peek(key);
On 2013/05/24 20:21:58, Mark Seaborn wrote:
> Fix spacing (you have 2 spaces before '=')

Done.

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_browser.h (right):

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.h:87: // trusted the handle was read only
but it was not, a mmaped file could
On 2013/05/24 20:21:58, Mark Seaborn wrote:
> "an mmapped"

Done.

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.h:88: // mutated after validation,
allowing an escape of the NaCl sandbox.
On 2013/05/24 20:21:58, Mark Seaborn wrote:
> "could be mutated" -- or better, "could be modified"
> 
> "escape of" -> "escape from"?

Done.

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.h:92: // Instead of allowing these
attacks, we only trust information we get
On 2013/05/24 20:21:58, Mark Seaborn wrote:
> "we" -> "the NaCl process"

Done.

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.h:101: // TODO(ncbray): move the cache
onto the NaCl process host to completely
On 2013/05/24 20:21:58, Mark Seaborn wrote:
> "onto the NaCl process host" -> "into NaClProcessHost"

Done.

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.h:102: // prevent one process from
guessing the token of another process.
On 2013/05/24 20:21:58, Mark Seaborn wrote:
> Nit: it doesn't prevent guessing, it prevents guessing from having any
> consequence.  "So that we don't rely on file tokens being unguessable"?

Done.

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:965: // However: each NaCl process
will consume 2-3 entries as it starts up, this
On 2013/05/24 20:21:58, Mark Seaborn wrote:
> Why 2-3?  Explain more in comment, perhaps?

Done.

https://codereview.chromium.org/14750007/diff/79001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:990: FROM_HERE,
On 2013/05/24 20:21:58, Mark Seaborn wrote:
> Indent arguments properly.  i.e.:
> 
> // Wrong
> if (some_function(
>     arg1)) ...
> 
> // Right
> if (some_function(
>         arg1)) ...

Done.

https://codereview.chromium.org/14750007/diff/79001/chrome/common/nacl_messag...
File chrome/common/nacl_messages.h (right):

https://codereview.chromium.org/14750007/diff/79001/chrome/common/nacl_messag...
chrome/common/nacl_messages.h:82: // Used by the NaCl process to aquire trusted
information about a file directly
On 2013/05/24 20:21:58, Mark Seaborn wrote:
> "acquire"

Done.

https://codereview.chromium.org/14750007/diff/79001/chrome/common/nacl_messag...
chrome/common/nacl_messages.h:85:
IPC_SYNC_MESSAGE_CONTROL2_2(NaClProcessMsg_ResolveFileToken,
On 2013/05/24 20:21:58, Mark Seaborn wrote:
> "Resolve" is vague.  How about "OpenFileFromToken" or "OpenFileGivenToken"?
> 
> (I know you already used the term "Resolve" in the NaCl code, but I still
think
> it would be better to use "Open" here.)

Opening is only part of the elephant.  I don't think "Open" is the right verb. 
"Given" is also an awkward way to resolve the resulting ambiguity.  Leaving as
is.

https://codereview.chromium.org/14750007/diff/79001/chrome/nacl/nacl_listener.cc
File chrome/nacl/nacl_listener.cc (right):

https://codereview.chromium.org/14750007/diff/79001/chrome/nacl/nacl_listener...
chrome/nacl/nacl_listener.cc:134: virtual bool ResolveFileToken(struct
NaClFileToken *file_token,
On 2013/05/24 20:21:58, Mark Seaborn wrote:
> Fix "*" spacing style

Done.

https://codereview.chromium.org/14750007/diff/79001/chrome/nacl/nacl_listener...
chrome/nacl/nacl_listener.cc:160: // It doesn't matter if the path is valid UTF8
as long as it's repeatable
On 2013/05/24 20:21:58, Mark Seaborn wrote:
> Do you mean "It doesn't matter if the path is invalid UTF8"?
> 
> Not sure what you mean by repeatable here.

Done.

https://codereview.chromium.org/14750007/diff/79001/chrome/nacl/nacl_validati...
File chrome/nacl/nacl_validation_db.h (right):

https://codereview.chromium.org/14750007/diff/79001/chrome/nacl/nacl_validati...
chrome/nacl/nacl_validation_db.h:21: virtual bool ResolveFileToken(struct
NaClFileToken *file_token,
On 2013/05/24 20:21:58, Mark Seaborn wrote:
> Fix "*" spacing style

Done.

https://codereview.chromium.org/14750007/diff/79001/chrome/nacl/nacl_validati...
File chrome/nacl/nacl_validation_query.cc (right):

https://codereview.chromium.org/14750007/diff/79001/chrome/nacl/nacl_validati...
chrome/nacl/nacl_validation_query.cc:34: struct NaClFileToken *file_token,
On 2013/05/24 20:21:58, Mark Seaborn wrote:
> Fix "*" spacing style

Done.

https://codereview.chromium.org/14750007/diff/79001/chrome/nacl/nacl_validati...
chrome/nacl/nacl_validation_query.cc:140: uint32 *file_path_length) {
On 2013/05/24 20:21:58, Mark Seaborn wrote:
> Fix "*" spacing style

Done.

https://codereview.chromium.org/14750007/diff/79001/chrome/nacl/nacl_validati...
chrome/nacl/nacl_validation_query.cc:147: *file_path = static_cast<char
*>(malloc(path.length() + 1));
On 2013/05/24 20:21:58, Mark Seaborn wrote:
> Style is "char*"

Done.

https://codereview.chromium.org/14750007/diff/79001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/14750007/diff/79001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/plugin.cc:1327: std::map<nacl::string,
struct NaClFileInfo>::iterator it =
On 2013/05/24 20:21:58, Mark Seaborn wrote:
> As an aside, having this URL -> FD mapping seems to be wrong because:
> 
>  * It might be leaking FDs if entries are never removed
>  * It might mean that two open() calls return the same FD, with the seek
> position being shared.
>  * It might mean that a fresh open() call doesn't refetch the URL (which might
> be wrong if the resource is changeable).
> 
> I don't know the scope of this.  Can you file an issue (with the info above),
> please?

I will certainly help you file an issue.  This change has taken way too long and
I just can't take the time to be a good citizen, myself, at this point.

https://codereview.chromium.org/14750007/diff/79001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

https://codereview.chromium.org/14750007/diff/79001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:174: struct
NaClFileInfo *info) {
On 2013/05/24 20:21:58, Mark Seaborn wrote:
> Fix "*" spacing style

Done.

https://codereview.chromium.org/14750007/diff/79001/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/service_runtime.h (right):

https://codereview.chromium.org/14750007/diff/79001/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/service_runtime.h:76: struct NaClFileInfo
*file_info;
On 2013/05/24 20:21:58, Mark Seaborn wrote:
> Fix "*" spacing style

Done.

[commit-bot: I haz the power, 2013-05-24 21:40:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ncbray@chromium.org/14750007/93001

[Mark Seaborn, 2013-05-24 21:55:41]

https://codereview.chromium.org/14750007/diff/93001/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_browser.h (right):

https://codereview.chromium.org/14750007/diff/93001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_browser.h:102: // TODO(ncbray): move the cache
onto NaClProcesHost so that we don't need to
"NaClProcessHost"

[commit-bot: I haz the power, 2013-05-24 22:41:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ncbray@chromium.org/14750007/106001

[Mark Seaborn, 2013-05-24 22:48:11]

https://chromiumcodereview.appspot.com/14750007/diff/106001/chrome/nacl/nacl_...
File chrome/nacl/nacl_validation_query.cc (right):

https://chromiumcodereview.appspot.com/14750007/diff/106001/chrome/nacl/nacl_...
chrome/nacl/nacl_validation_query.cc:10: #include
"native_client/src/include/portability.h"
Why?  You shouldn't be using native_client/src/include/portability.h in
Chromium-side code, outside of the legacy code in ppapi/native_client.

[commit-bot: I haz the power, 2013-05-24 22:52:20]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-05-24 23:38:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ncbray@chromium.org/14750007/106001

[commit-bot: I haz the power, 2013-05-25 14:10:10]

Change committed as 202278

[Nick Bray (chromium), 2013-05-29 00:19:33]

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