Windows Low integrity sandbox causes windowed plugins to be position at 0,0.

RESOLVED FIXED in Firefox 41

Status

()

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

People

(Reporter: bobowen, Assigned: bobowen)

Tracking

unspecified
mozilla41
All
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

3 years ago
Windows Low integrity sandbox causes windowed plugins to be position at 0,0.

This is being caused by a failure on this call to SetParent:
https://hg.mozilla.org/mozilla-central/file/f578b845c4b8/dom/plugins/ipc/PluginInstanceChild.cpp#l1408

This is because |hWndParent| has been created by the broker process and so is at a higher integrity level.

To fix this I think we will need to make another IPC call to the broker to do the re-parenting.

Unfortunately, we can't just return the window as part of this call because in e10s mode this call comes from the content process.
Assuming that it is running at low integrity it also couldn't do the re-parenting.
(Assignee)

Comment 1

3 years ago
Try push with a PoC fix just for non-e10s:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfe04ea6a42e

This now works for windowed plugins on non-e10s and proves that a fix will work.
(Assignee)

Comment 3

3 years ago
Created attachment 8611246 [details] [diff] [review]
For Windows NPAPI do window re-parenting in the chrome process to allow for sandboxing.

This feels a little bit of a hack in places, but I found it quite difficult to work out how to route things back to the chrome process.
Mainly because the PluginInstanceParent SetWindow code is called through the NPAPI interface.

However, I thought I'd put something together that at least seems to work and tries to work within the existing interfaces, with just a couple of IPC changes.
Assignee: nobody → bobowen.code
Status: NEW → ASSIGNED
Attachment #8611246 - Flags: feedback?
(Assignee)

Updated

3 years ago
Attachment #8611246 - Flags: feedback? → feedback?(jmathies)

Comment 4

3 years ago
Generally looks ok to me. What are the security implications here? When we 'approve' the low integrity child window by setting it in chrome, do we open up an attack vector where the low integrity process can access medium integrity window information? Or does Windows continue to enforce some restrictions here?

Updated

3 years ago
Attachment #8611246 - Flags: feedback?(jmathies) → feedback+
(Assignee)

Comment 5

3 years ago
(In reply to Jim Mathies [:jimm] from comment #4)
> Generally looks ok to me. What are the security implications here? When we
> 'approve' the low integrity child window by setting it in chrome, do we open
> up an attack vector where the low integrity process can access medium
> integrity window information? Or does Windows continue to enforce some
> restrictions here?

Thanks.
Hmm, I'm not sure, but I don't think we're giving anymore to the child process or window than it already has, just setting it's parent.

I'll do some experiments to see if the child process can now access things on the parent window that it couldn't before.

Comment 6

3 years ago
(In reply to Bob Owen (:bobowen) from comment #5)
> (In reply to Jim Mathies [:jimm] from comment #4)
> > Generally looks ok to me. What are the security implications here? When we
> > 'approve' the low integrity child window by setting it in chrome, do we open
> > up an attack vector where the low integrity process can access medium
> > integrity window information? Or does Windows continue to enforce some
> > restrictions here?
> 
> Thanks.
> Hmm, I'm not sure, but I don't think we're giving anymore to the child
> process or window than it already has, just setting it's parent.
> 
> I'll do some experiments to see if the child process can now access things
> on the parent window that it couldn't before.

I'm more interested in tracking down the msdn docs that explain Windows reaction to something like this, if they exist. I'll do some digging.
(Assignee)

Comment 7

3 years ago
(In reply to Bob Owen (:bobowen) from comment #2)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d14f55d7b5c

M(3) failures in this try run are because a SetFocus isn't happening with the new setup.
The plugin tests pass, but tests later in the chunk that rely on focus fail.

Still investigating what triggers the missing focus in the existing code.
(Assignee)

Comment 9

3 years ago
Created attachment 8614740 [details] [diff] [review]
For Windows NPAPI do window re-parenting in the chrome process to allow for sandboxing.

As I explained on IRC, the loss of focus that was causing the later M(3) failures was due to SetWindowPos still being in the plugin process.

This was being done when the window still didn't have the correct parent and caused focus to be lost.

I've moved this into PluginInstanceParent just after we have asked the Chrome process to re-parent.
I did this because I didn't seem to have the correct width and height in nsWindow and it is probably better to keep the SetNativeData part simple and do the extra bits outside anyway.

So, I also moved the ShowWindow into PluginInstanceParent, after the position has been set.

Try push with these latest patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc9dcb268dfe
Attachment #8611246 - Attachment is obsolete: true
Attachment #8614740 - Flags: review?(jmathies)
(Assignee)

Comment 10

3 years ago
Comment on attachment 8614740 [details] [diff] [review]
For Windows NPAPI do window re-parenting in the chrome process to allow for sandboxing.

Bad news ... this patch is breaking Silverlight even with the sandbox turned off.
Attachment #8614740 - Flags: review?(jmathies)
(Assignee)

Comment 11

3 years ago
The more promising news is that, without this patch and adding very permissive rules, Silverlight seems to work, albeit with this 0,0 problem.

So, once I fix the problem with this patch there is a chance that a sensible set of rules can make Silverlight work inside the sandbox.

I tested it playing on netflix and amazon.
(Assignee)

Comment 12

3 years ago
Created attachment 8622448 [details] [diff] [review]
For Windows NPAPI do window re-parenting in the chrome process to allow for sandboxing.

Finally worked out why Silverlight on Amazon was broken.

After ruling out loads of things around the system calls failing for some reason, due to timing.

It turned out that it was because I'd moved the SetWindowPos before we set the height and width member variables for PluginInstanceChild.
These get used in our own processing of the Windows message that comes off the back of SetWindowPos.
During the initialisation of Silverlight on Amazon a 1,1 window gets sent down and this was getting used for the final call to SetWindowPos before the video starts.

Try push with this change and the sandbox turned on:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3807ed607d33

Try push with e10s enabled as well:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e113c27c9356

I've manually tested a few flash video and game sites with and without e10s.

I've also tested Amazon and Netflix with Silverlight.
Silverlight is only working with a temporary really permissive set of rules, so I need to lock those down to the essentials, but I'll do that in another bug.

Tried one Unity game and the plugin works with the low integrity sandbox, but the plugin updater that it kicks off if you don't have the right version doesn't.
Attachment #8614740 - Attachment is obsolete: true
Attachment #8622448 - Flags: review?(jmathies)

Comment 13

3 years ago
Comment on attachment 8622448 [details] [diff] [review]
For Windows NPAPI do window re-parenting in the chrome process to allow for sandboxing.

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

::: dom/plugins/ipc/PluginInstanceChild.cpp
@@ +1230,5 @@
> +                  reinterpret_cast<uint64_t>(mPluginWindowHWND);
> +          } else {
> +              // Now we know that the window has the correct parent we can show
> +              // it. The actual visibility is controlled by its parent. This
> +              // works, because SetWindow is always called at least twice.

Seems a bit risky to rely on this as I doubt it's defined behavior, but if it reliably happens and we need it ok.

::: widget/windows/nsWindow.h
@@ +132,5 @@
>                                       bool aUpdateNCArea = false,
>                                       bool aIncludeChildren = false);
>    NS_IMETHOD              Invalidate(const nsIntRect & aRect);
>    virtual void*           GetNativeData(uint32_t aDataType);
> +  void SetNativeData(uint32_t aDataType, uintptr_t aVal) override;

nit spacing
Attachment #8622448 - Flags: review?(jmathies) → review+
(Assignee)

Comment 14

3 years ago
Created attachment 8622988 [details] [diff] [review]
For Windows NPAPI do window re-parenting in the chrome process to allow for sandboxing. v3

Thanks for the review Jim.

(In reply to Jim Mathies [:jimm] from comment #13)

> ::: dom/plugins/ipc/PluginInstanceChild.cpp
> @@ +1230,5 @@
> > +                  reinterpret_cast<uint64_t>(mPluginWindowHWND);
> > +          } else {
> > +              // Now we know that the window has the correct parent we can show
> > +              // it. The actual visibility is controlled by its parent. This
> > +              // works, because SetWindow is always called at least twice.
> 
> Seems a bit risky to rely on this as I doubt it's defined behavior, but if
> it reliably happens and we need it ok.

Yes, I was a bit concerned about this ... and thinking about it I don't know why I removed the calls in the Parent.
This patch puts them back and they are only called when a window is passed back, i.e. when they are not called in the child.
I've changed the comments to explain this.

> ::: widget/windows/nsWindow.h
> >    virtual void*           GetNativeData(uint32_t aDataType);
> > +  void SetNativeData(uint32_t aDataType, uintptr_t aVal) override;
> 
> nit spacing

Fixed, thanks.
Attachment #8622448 - Attachment is obsolete: true
Attachment #8622988 - Flags: review?(jmathies)
(Assignee)

Updated

3 years ago
Attachment #8622988 - Attachment description: For Windows NPAPI do window re-parenting in the chrome process to allow for sandboxing. → For Windows NPAPI do window re-parenting in the chrome process to allow for sandboxing. v3

Updated

3 years ago
Attachment #8622988 - Flags: review?(jmathies) → review+
(Assignee)

Comment 16

3 years ago
For the record, before landing I added in a flash specific pref for the sandbox level.
This is the same value as the default, but will make it easier to change for testing.

https://hg.mozilla.org/integration/mozilla-inbound/diff/6a5137a37ec5/browser/app/profile/firefox.js
https://hg.mozilla.org/mozilla-central/rev/6a5137a37ec5
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
(Assignee)

Updated

3 years ago
Blocks: 1185532

Updated

3 years ago
Depends on: 1185529
(Assignee)

Updated

3 years ago
Depends on: 1236911
You need to log in before you can comment on or make changes to this bug.