1202910 - Content sandboxing issues with NPAPI NPN_PostURL related calls

Status: RESOLVED WORKSFORME

(Core Graveyard :: Plug-ins, defect, P3)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Sandboxing of content processes has some issues caused by the content process's role in hosting NPAPI plug-ins.  These might want to be broken out into separate bugs:

1. As noted in bug 1190032, we currently see the content process creating and using a temporary directory to hold a cache file when hosting an NPAPI plug-in.  (Also, the workaround in that bug looks like it's not quite enough, because the directory is CreateUnique()d, so its name might not be exactly the argument.)

2. NPAPI allows the plugin to give the host the path to a temporary file to get POST data from (see https://developer.mozilla.org/en-US/Add-ons/Plugins/Reference/NPN_PostURLNotify).  That message goes to the content process, which in a sandboxed world can't be trusted to relay it accurately to the parent.  If actual use of this feature is limited enough, we might be able to get away with restricting what files are allowed.

Comment 1

[Jim Mathies [:jimm]]

(In reply to Jed Davis [:jld] [⏰MDT; UTC-6] from comment #0)
> 1. As noted in bug 1190032, we currently see the content process creating
> and using a temporary directory to hold a cache file when hosting an NPAPI
> plug-in.  (Also, the workaround in that bug looks like it's not quite
> enough, because the directory is CreateUnique()d, so its name might not be
> exactly the argument.)

Plugin temp is covered by other bugs.

> 2. NPAPI allows the plugin to give the host the path to a temporary file to
> get POST data from (see
> https://developer.mozilla.org/en-US/Add-ons/Plugins/Reference/
> NPN_PostURLNotify).  That message goes to the content process, which in a
> sandboxed world can't be trusted to relay it accurately to the parent.  If
> actual use of this feature is limited enough, we might be able to get away
> with restricting what files are allowed.

Leaving this bug open and dropping into our first file access restriction milestones. We need to dig into this and figure out what we want to do here. The temp files should be located in the new content process safe temp directory. The trust worthiness of the data though is open to question. 

I'm not sure there's anything we can do here. The chrome side receiver of the NPN_PostURLNotify should be hardened to handle 'questionable' data. Beyond this though I'm not sure what else we need to do.

Comment 2

[Stephen A Pohl [:spohl]]

After reading up on NPN_PostURLNotify[1] and NPN_PostURL[2] I believe that there could be two attack vectors:

1. A plugin sets the |buf| param to a local file instead of a temporary file. This could lead to local files being sent to a remote server.
2. If our implementation of NPN_PostURLNotify and/or NPN_PostURL is inadequate, specially crafted data passed via the |buf| param could possibly break out of the sandbox.

Regarding 1: I haven't verified this yet, but I'm assuming that there is already some form of enforcement to only send temporary files rather than arbitrary local files. We should ensure that this is restricted to sandboxed temp files.

Regarding 2: Without having read the implementation of either NPN_PostURLNotify or NPN_PostURL yet (and just starting to wrap my head around sandboxing), it is hard to say if this is a legitimate attack vector.

Jed, does this sum up your point 2 from comment 0? Or were you referring to a different issue all together?


[1] https://developer.mozilla.org/en-US/Add-ons/Plugins/Reference/NPN_PostURLNotify
[2] https://developer.mozilla.org/en-US/Add-ons/Plugins/Reference/NPN_PostURL

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Stephen A Pohl [:spohl] from comment #2)
> 1. A plugin sets the |buf| param to a local file instead of a temporary
> file. This could lead to local files being sent to a remote server.

The failure mode I had in mind was more like: nothing is actually compromised, but the plugin wants to read a file from a sensitive location where we don't want to give the content process read access.  Alternately: the content process is compromised 

> Regarding 1: I haven't verified this yet, but I'm assuming that there is
> already some form of enforcement to only send temporary files rather than
> arbitrary local files. We should ensure that this is restricted to sandboxed
> temp files.

But what does “temporary file” mean?  Does NPAPI have a way for the plugin host to communicate a temporary directory to be used for things like this, or will the plugin just kind of guess and maybe use something difficult?

> 2. If our implementation of NPN_PostURLNotify and/or NPN_PostURL is
> inadequate, specially crafted data passed via the |buf| param could possibly
> break out of the sandbox.

This is a general failure mode for IPDL interfaces, but it's not really an issue here — as I understand it, in general an NPAPI plugin will be *less* sandboxed than a content process, so a compromised NPAPI plugin wouldn't gain anything by trying to exploit a content process embedding it.

…which, now that I think about it, might be one approach: make the plugin process open the file and then send it to the content process (e.g., as a serialized input stream rather than the current (nsCString, bool) pair).  But that would have to be tunneled through the old plugin code (see PluginInstanceParent::AnswerNPN_PostURL and NPNetscapeFuncs::posturl) somehow.


But there are also ongoing plans to dispose of NPAPI — it might not be completely gone by the time we need to deal with this, but if the only remaining plugin is Flash then this might be easier to deal with.  (Do we know anything about how/if Flash uses NPN_PostURL* with file=true?)

Comment 4

[Jim Mathies [:jimm]]

APIs:

NPN_PostURL(NPP instance, const char *url, const char *target, uint32 len, const char *buf, NPBool file)
  
  url - post url target. in code we3 often  refer to this as a relative url.
  target - window name (which loads result in the window and can include the frame the plugin is in, triggering an unload)
  buffer, len - data to send or a path to a local file
  isfile - bool, indicates if the caller is passing a file path or raw data in buffer

NPN_PostURL/NPN_PostURLNotify - writes data from a file or buffer to the URL and either displays the server response in the target window or delivers it to the plug-in.

NPN_GetURL/NPN_GetURLNotify - reads data from the URL and either displays it in the target window or delivers it to the plug-in.

Reply:

void NPP_URLNotify(NPP instance, const char* url, NPReason reason, void* notifyData);

Local file example:

char* myData = "file:///c\/myDirectory/myFileName";
uint32 myLength = strlen(myData) + 1;
err = NPN_PostURL(instance, "ftp://[e-mail]/pub/", "response", myLength, myData, TRUE);


Where these calls come from in the plugin instances:
http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/dom/plugins/ipc/PluginModuleChild.cpp#1209

Main chrome side handler:
http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/dom/plugins/base/nsNPAPIPlugin.cpp#392

Handler depending on type of request:

nsPluginHost::PostURL:
http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/dom/plugins/base/nsPluginHost.cpp#513
nsPluginHost::GetURL:
http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/dom/plugins/base/nsPluginHost.cpp#467

Here, for cases where isfile is true we call:

rv = CreateTempFileToPost(postData, getter_AddRefs(file));

http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/dom/plugins/base/nsPluginHost.cpp#539
http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/dom/plugins/base/nsPluginHost.cpp#3732

which calls GetPluginTempDir:
http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/dom/plugins/base/nsPluginHost.cpp#3764

and then parses the file data and transfers it over to a temp file:
http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/dom/plugins/base/nsPluginHost.cpp#3793

The parsing bit here is a little concerning. Also, we do not restrict access to files on the system currently except through existing sandbox rules.

Comment 5

[Jim Mathies [:jimm]]

I don't think we need to worry about these apis right now. Sandboxing will take care of file access restrictions. Temp file access is already handled. The file buffer processing in Chrome needs to be investigated for hardening. 

ni to Paul, so this is on his teams radar for fuzzing/hardening investigation.

Comment 6

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #5)
> I don't think we need to worry about these apis right now. Sandboxing will
> take care of file access restrictions. Temp file access is already handled.

I just realized the file reads happen in the chrome process, the plugin process sends a path.

So it looks like we may want to superseded on the chrome side before access these files.

Comment 7

[Benjamin Smedberg]

NPAPI post with file is gone, so this is no longer an issue.