Closed Bug 1123759 Opened 5 years ago Closed 5 years ago

Set low integrity on NPAPI processes for Windows sandboxing policy

Categories

(Core :: Security: Process Sandboxing, defect)

All
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox39 --- unaffected
firefox40 --- wontfix
firefox41 --- fixed

People

(Reporter: bobowen, Assigned: bobowen)

References

Details

Attachments

(1 file)

If I try to set it to low integrity, it pauses a lot and eventually gives me something to click to play.
Another long pause and then it plays video only, no sound.

This is with completely open rules in place to allow access to files registry etc.

I've tried moving the start of the sandbox until the end of PluginModuleChild::RecvSetAudioSessionData, but that still doesn't work, even thought the AudioSession seems to be initializing correctly.
When I tried this Flash actually started its own sandbox again, did you see that?
I think for audio we need to allow access to "\RPC Control\AudioSrv", which is an ALPC Port. Doing that locally however didn't fix the audio problem.
(In reply to Tom Schuster [:evilpie] from comment #1)
> When I tried this Flash actually started its own sandbox again, did you see
> that?

I suspect that the low integrity setting is interfering with the method used to disable the protected mode.
I've been using the mms.cfg file directly to disable protected mode so I haven't seen this.

(In reply to Tom Schuster [:evilpie] from comment #2)
> I think for audio we need to allow access to "\RPC Control\AudioSrv", which
> is an ALPC Port. Doing that locally however didn't fix the audio problem.

Interesting, where have you seen this being blocked?
Audio is definitely one of the blockers for running at low integrity.
IAudioClient::Initialize is the most likely culprit it returns E_HANDLE during sandboxing.
(In reply to Tom Schuster [:evilpie] from comment #4)
> IAudioClient::Initialize is the most likely culprit it returns E_HANDLE
> during sandboxing.

Yeah, we have the same problem with the content process sandbox.
Unfortunately, it's not something that we can allow via the policy rules.

Thanks for confirming this.
Is audio blocked in both INTEGRITY_LEVEL_LOW and INTEGRITY_LEVEL_MEDIUM_LOW?
Here's a try push, as part of investigations as to how strong a sandbox we can enable for a 64-bit NPAPI sandbox:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a22533ddf6c7

This sets the default NPAPI sandbox level to 2 for 64-bit as well as making the process low integrity.

Clearly this is breaking some crash reporting tests at the moment.
OS: Windows 7 → Windows
Hardware: x86_64 → All
Depends on: 1165895
Depends on: 1165903
Assignee: nobody → bobowen.code
This adds low integrity to NPAPI sandbox level 2 (or greater).

Brian - I've re-jigged the sandbox policy setup function to be more like the content process one because I think it's generally easier to follow.
I've moved some of the things that I think can be reasonably set on all policies out of the conditional parts, again to make it a bit clearer.

Ben - As protected mode doesn't work at all with low integrity, I've changed it so that it gets turned off, in case people try to use level 2 for 32-bit.
It seems to fail because understandably the flash broker process can't run at low integrity and it gets into a start-up / crash loop for the plugin and flash broker processes.
I've not put #ifs around the new member variable related bits as it would make the code pretty horrible to read.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c44db4ccf929
Attachment #8610519 - Flags: review?(netzen)
Attachment #8610519 - Flags: review?(benjamin)
Comment on attachment 8610519 [details] [diff] [review]
Set low integrity on NPAPI processes for Windows sandboxing policy level >= 2.

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

::: dom/plugins/ipc/PluginModuleParent.cpp
@@ +564,5 @@
>      }
>  #endif
>  #endif
>  
> +#if defined(XP_WIN) && defined(_X86_)

might be worth mentioning here as well that protected mode only applies to x86.

::: dom/plugins/ipc/PluginProcessParent.cpp
@@ +101,5 @@
>                            NS_LITERAL_STRING("\\Adobe\\Flash Player\\*"));
> +
> +#if defined(_X86_)
> +    // Write access to the Temp directory should only be needed for 32-bit as
> +    // it is used to turn off protceted mode.

*nit: protected
Also pls add since protected mode only applies to x86.
Attachment #8610519 - Flags: review?(netzen) → review+
Attachment #8610519 - Flags: review?(benjamin) → review+
Thanks both for the reviews.

(In reply to Brian R. Bondy [:bbondy] from comment #9)
> Comment on attachment 8610519 [details] [diff] [review]

> > +#if defined(XP_WIN) && defined(_X86_)
> 
> might be worth mentioning here as well that protected mode only applies to
> x86.

Added locally.
 
> > +#if defined(_X86_)
> > +    // Write access to the Temp directory should only be needed for 32-bit as
> > +    // it is used to turn off protceted mode.
> 
> *nit: protected
> Also pls add since protected mode only applies to x86.

Fixed locally, thanks.
Also added x86 part.
https://hg.mozilla.org/mozilla-central/rev/3998f5aab3a4
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Hi Jeromie,

I've been working on getting the Flash Player NPAPI plugin running with our own sandbox policy for 64-bit.

I wonder if you would have time to test the latest Nightly 64-bit with your internal test suite.

To turn on the sandbox for Flash you need to set the following pref and then restart Firefox:
dom.ipc.plugins.sandbox-level.flash=2

Thanks for your help,
Bob
Flags: needinfo?(jeclark)
Adobe's US offices are closed this week.  I'll poke around and see if I can get someone to pick this up on one of the global teams.  If not, I'll get one of the folks on my team to do it next week.
Flags: needinfo?(jeclark)
(In reply to Jeromie Clark from comment #14)
> Adobe's US offices are closed this week.  I'll poke around and see if I can
> get someone to pick this up on one of the global teams.  If not, I'll get
> one of the folks on my team to do it next week.

That's great!

Next week is fine.
Thanks for organising this.
Depends on: 1180645
Depends on: 1179972
Depends on: 1182411
Wasn't sure if this is on release management's radar.

[Tracking Requested - why for this release]:
As 64-bit Firefox is planned to be available for Fx40 and there in no protected mode for Flash 64-bit, we want to be able to turn on our own NPAPI sandbox.

I need to get bug 1182411 landed and uplifted to 41 and then I intend to turn the sandbox on by default for Flash 64-bit on Nightly then shortly after Aurora and eventually on Beta.
I don't think this blocks our Win64 release in 40. ni Javaun to confirm. If we don't need this in 40, let's ship it in 41. If we do need this in 40, I would like to ensure that the fix is verified before uplifting.

Tracking until I know better.
Flags: needinfo?(jmoradi)
Lawrence, correct. We can still build win64 in 40 and make available on the all systems page. https://www.mozilla.org/en-US/firefox/all/ 

Our assumption was always sandboxing in 41. As sandboxing offers more safety, we'll wait for sandboxing to arrive (and some other fixes) before promoting win64 builds more heavily or moving beyond the /all page
Flags: needinfo?(jmoradi)
Based on comment 18, I'm dropping tracking for 40 and setting status to wontfix for this release. We'll ship this fix in 41.
(In reply to Javaun Moradi [:javaun] from comment #18)
> Lawrence, correct. We can still build win64 in 40 and make available on the
> all systems page. https://www.mozilla.org/en-US/firefox/all/ 
> 
> Our assumption was always sandboxing in 41. As sandboxing offers more
> safety, we'll wait for sandboxing to arrive (and some other fixes) before
> promoting win64 builds more heavily or moving beyond the /all page

Thanks for the clarification Javaun.
I'm pretty glad actually, because I found out yesterday that one of my changes to allow for sandboxing has also broken old flash games (Action Script 2) keyboard interaction for 32-bit protected mode.
(This is on top of the bug with our own sandbox turned on).

So, I may have to back those out if I can't find a fix soon.
You need to log in before you can comment on or make changes to this bug.