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

RESOLVED FIXED in Firefox 53

Status

()

Core
Security: Process Sandboxing
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: bobowen, Assigned: handyman)

Tracking

41 Branch
mozilla53
All
Windows
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: sbwc2)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

3 years ago
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

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

Updated

2 years ago
Whiteboard: sbwc2
(Assignee)

Comment 2

a year ago
Created attachment 8815054 [details] [diff] [review]
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...
Attachment #8815054 - Flags: review?(bobowencode)
(Assignee)

Updated

a year ago
Assignee: nobody → davidp99
(Reporter)

Comment 3

a year ago
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.
Attachment #8815054 - Flags: review?(bobowencode)
(Assignee)

Comment 4

a year ago
Created attachment 8818654 [details] [diff] [review]
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.
Attachment #8815054 - Attachment is obsolete: true
Attachment #8818654 - Flags: review?(jmathies)
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 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.
Attachment #8818654 - Flags: review?(jmathies)
(Assignee)

Comment 7

a year ago
Created attachment 8820038 [details] [diff] [review]
Verify nsWindow::SetNativeData child windows originated from plugin processes

Added ASSERT to PluginProcessParent method (indeed, all of this stuff happens in the chrome process).
Attachment #8818654 - Attachment is obsolete: true
Attachment #8820038 - Flags: review?(jmathies)

Updated

a year ago
Attachment #8820038 - Flags: review?(jmathies) → review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 9

a year ago
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=40875070&repo=mozilla-inbound
Flags: needinfo?(davidp99)

Comment 11

a year ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c80e362e2bf
Backed out changeset 49f1c7ec4eca for bustage
(Assignee)

Comment 12

a year ago
Created attachment 8821282 [details] [diff] [review]
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
Attachment #8820038 - Attachment is obsolete: true
Flags: needinfo?(davidp99)
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 13

a year ago
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed

Comment 14

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/34a6ce3bfa09
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.