Closed Bug 1165903 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Security: Process Sandboxing, defect)

All
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: bobowen, Assigned: bobowen)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
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.
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?
Attachment #8611246 - Flags: feedback? → feedback?(jmathies)
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?
Attachment #8611246 - Flags: feedback?(jmathies) → feedback+
(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.
(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.
(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.
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)
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)
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.
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 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+
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)
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
Attachment #8622988 - Flags: review?(jmathies) → review+
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
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Blocks: 1185532
Depends on: 1185529
Depends on: 1236911
You need to log in before you can comment on or make changes to this bug.