1185472 - Only allow NPAPI HWNDs to be adopted by an HWND in the chrome process.

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Bob Owen (:bobowen)]

In order to get low integrity sandboxing to work for NPAPI, we've had to allow for HWNDs to be passed from the NPAPI process to be adopted in the chrome process.

For e10s those HWNDs pass through the content process and so IPC messages have been created that pass the HWNDs up to the chrome process for adoption.

We should probably add checks in the chrome process to make sure that these HWNDs actually come from an NPAPI process.

Comment 1

[Jim Mathies [:jimm]]

Maybe we could use the bridge to hand them directly to the browser process.

Comment 2

[David Parks [:handyman]]

Attachment: Guarantee NS_NATIVE_CHILD_OF_SHAREABLE_WINDOW messages came from a plugin process before acting on them

Pretty sure this is right -- but let me know if there is another message I'm supposed to include in this bug.

Summary: We are sending NS_NATIVE_CHILD_OF_SHAREABLE_WINDOW from the plugin process (P) to the content proc (C), which then sends the message to the main proc (M).  So P->C->M.  We need a mechanism to guarantee that M only processes messages that came from P (assuming malicious C).  So I send a message P->M that I use for cross-validation in M.

I considered jimm's brainstorm about skipping C but C has code for identifying the correct widget that we cannot do otherwise, so I went with cross-checking:
https://dxr.mozilla.org/mozilla-central/rev/05328d3102efd4d5fc0696489734d7771d24459f/dom/plugins/base/nsPluginInstanceOwner.cpp#774

I believe the current code has an IPDL protocol issue -- note that both the P->C and C->M messages are async.  If P or C then issue commands to the window that we operate on (this includes commands that happen in M as the result of P->M IPDL messages), those commands may happen out of sequence.  In the existing patch, I've made P->C and C->M messages sync to avoid this but I made the new message (P->M) async.  This message will also need to be sync if P is capable of executing commands on the window without IPDL.  I left it async because I _think_ the only instruction P can issue are IPDL messages actually run on M.  So IPDL sequencing will handle order.  But I might not be right about that...

Comment 3

[Bob Owen (:bobowen)]

Comment on attachment 8815054 [details] [diff] [review]
Guarantee NS_NATIVE_CHILD_OF_SHAREABLE_WINDOW messages came from a plugin process before acting on them

Review of attachment 8815054 [details] [diff] [review]:
-----------------------------------------------------------------

I'm sorry to keep doing this to you, but if we go with this solution, it'll have to be jimm or another NPAPI peer who reviews this. Particularly with the async/sync changes as I know the plugin stuff is really prone to hangs.

However, how about this as a simpler solution.

* Add static unordered_set<base::ProcessId> member to PluginProcessParent, (probably have to make it a pointer and create on first use/delete when empty to prevent static initializers).
* In PluginProcessParent::OnChannelConnected add pid into set, (creating if needed).
* In PluginProcessParent::~PluginProcessParent remove from set, (deleting if empty). Probably need to store the pid in a member variable as I'm not sure we can guarantee mChildProcessHandle is still populated at this point.
* Add a new static method PluginProcessParent::IsPluginProcessId(base::ProcessId aPid), which returns true if it is in the set.
* In nsWindow::SetNativeData use GetWindowThreadProcessId to get the pid for the HWND and call the static method to check it.

This also covers PluginWidgetParent::RecvSetNativeChildWindow as well, although that goes away once we stop windowed plugins.
Probably still best to get jimm to review as a double check on this solution.

Also, if you still think those calls need to be sync maybe that should be a different bug.

Comment 4

[David Parks [:handyman]]

Attachment: Verify nsWindow::SetNativeData child windows originated from plugin processes

Bob's suggestion of GetWindowThreadProcessId greatly simplifies this patch.

The breakdown in comment 3 describes the patch behavior in detail.  I've just confirmed this with breakpoints in the debugger -- pages with Flash plugins always seem to send a NS_NATIVE_CHILD_OF_SHAREABLE_WINDOW message when launched (for example, http://www.tinywebgallery.com/en/tfu/web_demo2.php ).  

I still have some concerns about the async PBrowser::SetNativeChildOfShareableWindow and PPluginInstance::SetNetscapeWindowAsParent messages causing future window events to happen before the window parentage has actually changed... but they are no longer part of the patch for this bug.

Comment 5

[Jim Mathies [:jimm]]

Comment on attachment 8818654 [details] [diff] [review]
Verify nsWindow::SetNativeData child windows originated from plugin processes

Review of attachment 8818654 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/ipc/PluginProcessParent.cpp
@@ +275,5 @@
>      return mProcessState == PROCESS_CONNECTED;
>  }
>  
> +bool
> +PluginProcessParent::IsPluginProcessId(base::ProcessId procId) {

what process are we expecting this to be called in? Chrome? We should add process asserts here to be sure only the right callers can access this.

::: widget/windows/nsWindow.cpp
@@ +3372,5 @@
> +      {
> +        HWND childHwnd = reinterpret_cast<HWND>(aVal);
> +        DWORD childProc = 0;
> +        GetWindowThreadProcessId(childHwnd, &childProc);
> +        if (!PluginProcessParent::IsPluginProcessId(static_cast<base::ProcessId>(childProc))) {

From widget/nsWindow code so I assume a chrome process query?

Comment 6

[Jim Mathies [:jimm]]

Comment on attachment 8818654 [details] [diff] [review]
Verify nsWindow::SetNativeData child windows originated from plugin processes

Review of attachment 8818654 [details] [diff] [review]:
-----------------------------------------------------------------

clearing until a new rev gets posted.

Comment 7

[David Parks [:handyman]]

Attachment: Verify nsWindow::SetNativeData child windows originated from plugin processes

Added ASSERT to PluginProcessParent method (indeed, all of this stuff happens in the chrome process).

Comment 8

[David Parks [:handyman]]

Tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5c9eab718d405796d1f63cec0822874a002f665&selectedJob=32986773

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/49f1c7ec4eca
Only allow NPAPI HWNDs to be adopted by an HWND in the chrome process r=jmathies

Comment 10

[Carsten Book [:Tomcat]]

backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=40875070&repo=mozilla-inbound

Comment 11

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c80e362e2bf
Backed out changeset 49f1c7ec4eca for bustage

Comment 12

[David Parks [:handyman]]

Attachment: Verify nsWindow::SetNativeData child windows originated from plugin processes (r+ed by jimm)

*sigh*.  Fixed #ifdefs for other platforms.

A more thorough suite of tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b51d966fd118ec7ea62ccd0956e1a9202dcefd08&selectedJob=33331781

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/34a6ce3bfa09
Only allow NPAPI HWNDs to be adopted by an HWND in the chrome process. r=jimm

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/34a6ce3bfa09