Closed Bug 1123245 Opened 5 years ago Closed 5 years ago

On Windows, enable a sandbox using the USER_NON_ADMIN access token level for NPAPI processes.

Categories

(Core :: Security: Process Sandboxing, defect)

All
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox35 --- wontfix
firefox36 + fixed
firefox37 + fixed
firefox38 --- fixed
relnote-firefox --- -

People

(Reporter: bobowen, Assigned: bobowen)

References

Details

Attachments

(4 files, 2 obsolete files)

Plugin processes should not need Administrator rights.
This patch enables an open sandbox on Windows for the NPAPI processes.

Similar to the other processes, I've added an env var MOZ_DISABLE_NPAPI_SANDBOX to disable the sandbox.

I've also added Multi-process name to aid with log reading.

Tim - for the sandbox parts.
Josh - for the NPAPI parts.
Attachment #8551333 - Flags: review?(tabraldes)
Attachment #8551333 - Flags: review?(joshmoz)
Kept the setting of the USER_NON_ADMIN token level separate for bisection purposes.
Attachment #8551334 - Flags: review?(tabraldes)
Blocks: 1123759
Attachment #8551333 - Flags: review?(tabraldes) → review+
Attachment #8551334 - Flags: review?(tabraldes) → review+
Attachment #8551333 - Flags: review?(joshmoz) → review+
[Tracking Requested - why for this release]:

Bug 1119941 disabled Flash's Protected Mode for testing on Nightly 38 and Aurora 37 (and possibly Beta 36 soon). This NPAPI plugin process sandbox is a partial mitigation and we should probably test it in all pre-release channels.
Try push with default and optional per plugin prefs:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c53dd3a18ba
This adds a default pref for whether the Windows NPAPI sandbox is enabled:
dom.ipc.plugins.sandbox.default

and allows you to set for each plugin if you want:
dom.ipc.plugins.sandbox.<plugin-nice-filename>

I've added the one for flash, just so it is easy to flip:
dom.ipc.plugins.sandbox.flash


I've tried the patches on Beta and they need a bit of fixing up, but it's not too bad.
I've not tried Aurora, but I think it should be fairly painless.
Attachment #8553198 - Flags: review?(benjamin)
Comment on attachment 8553198 [details] [diff] [review]
Part 3: Add prefs for the Windows NPAPI process sandbox.

I wonder if it wouldn't be cleaner to avoid per-platform differences in the signature of PluginProcessParent::Launch and just use ifdefs in the implementation. That's my slight preference but not super strong.

r=me in either case.
Attachment #8553198 - Flags: review?(benjamin) → review+
r=bsmedberg - from comment 8

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #8)
> Comment on attachment 8553198 [details] [diff] [review]
> Part 3: Add prefs for the Windows NPAPI process sandbox.
> 
> I wonder if it wouldn't be cleaner to avoid per-platform differences in the
> signature of PluginProcessParent::Launch and just use ifdefs in the
> implementation. That's my slight preference but not super strong.
> 
> r=me in either case.

After quick IRC chat, changed to make the PluginProcessParent::Launch signature changes for all platforms/builds - for more readable code.

Noticed that I hadn't updated the documentation comment above PluginProcessParent::Launch.

I also meant to mention that I have manually tested with Flash and Silverlight, with all combinations of default and flash prefs, to make sure the processes are sandboxed correctly in each case.
Did this for e10s and non-e10s.

Try push to make sure I've not screwed up non-Windows builds:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb87f83e371c
Attachment #8553198 - Attachment is obsolete: true
Attachment #8553243 - Flags: review+
r=bsmedberg - from comment 8

(In reply to Bob Owen (:bobowen) from comment #9)

> Try push to make sure I've not screwed up non-Windows builds:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb87f83e371c

Which I had:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd9a8672f57c
Attachment #8553243 - Attachment is obsolete: true
Attachment #8553256 - Flags: review+
The patches apply cleanly to Aurora.
Try push with the default and flash Windows NPAPI sandbox prefs set to true:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ae98c6696ed
Part 1 doesn't quite apply cleanly to Beta, but only because of a small context difference.
Part 2 is fine.
Part 3 was a bit more involved, so here's a separate patch for that part for Beta.

Try push with the default and flash Windows NPAPI sandbox prefs set to true:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c09ed02133dd
At this point for Aurora/beta we only want to change the Flash setting, not for all plugins.

I'd be willing to experiment with doing this for all plugins on nightly and see how that ride the trains.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #15)
> At this point for Aurora/beta we only want to change the Flash setting, not
> for all plugins.
> 
> I'd be willing to experiment with doing this for all plugins on nightly and
> see how that ride the trains.

They were just flipped in the try push patch, to make the try runs more meaningful.
In both Part 3 patches they are set to false at the moment.

I suppose it would make sense to set the flash one to true when we disable protected mode.
Bob, we would like this patch in beta 4. Could you fill the uplift requests to make sure it lands before Monday afternoon. Thanks
Flags: needinfo?(bobowen.code)
Comment on attachment 8551333 [details] [diff] [review]
Part 1: Enable an open sandbox on Windows NPAPI processes.

(In reply to Sylvestre Ledru [:sylvestre] from comment #18)
> Bob, we would like this patch in beta 4. Could you fill the uplift requests
> to make sure it lands before Monday afternoon. Thanks

Sure, no problem.

Approval Request Comment
[Feature/regressing bug #]: This patch introduces an NPAPI process sandbox on Windows that means that the process isn't started with Administrator rights.
The feature is controlled by a default pref, which covers all plugins.
This can be overridden by per plugin prefs.
The sandbox is currently preffed off for all plugin types.

[User impact if declined]: We will not be able to turn on our own sandbox for plugin processes.

[Describe test coverage new/current, TreeHerder]: The sandbox is not tested directly, but there are plugin tests, which helps to ensure that the sandbox doesn't cause a regression. While the prefs are off at the moment, try runs have been done for Aurora and Beta with the sandbox enabled.

[Risks and why]: Low, the sandbox is preffed off at the moment.
This probably rises to medium when the sandbox is turned on.
While we don't believe that any plugins should require the rights that the sandbox removes, it is possible they do or the sandbox interferes with them in some other way.

[String/UUID change made/needed]: None.
Flags: needinfo?(bobowen.code)
Attachment #8551333 - Flags: approval-mozilla-beta?
Attachment #8551333 - Flags: approval-mozilla-aurora?
Comment on attachment 8551334 [details] [diff] [review]
Part 2: Use the USER_NON_ADMIN access token level for Windows NPAPI processes.

See comment 19.
Attachment #8551334 - Flags: approval-mozilla-beta?
Attachment #8551334 - Flags: approval-mozilla-aurora?
Comment on attachment 8553256 [details] [diff] [review]
Part 3: Add prefs for the Windows NPAPI process sandbox. v3

See comment 19.

This patch doesn't apply cleanly to Beta, so I have created a separate patch that can be used:
Beta Part 3: Add prefs for the Windows NPAPI process sandbox.
Attachment #8553256 - Flags: approval-mozilla-beta?
Attachment #8553256 - Flags: approval-mozilla-aurora?
Attachment #8551333 - Flags: approval-mozilla-beta?
Attachment #8551333 - Flags: approval-mozilla-beta+
Attachment #8551333 - Flags: approval-mozilla-aurora?
Attachment #8551333 - Flags: approval-mozilla-aurora+
Attachment #8551334 - Flags: approval-mozilla-beta?
Attachment #8551334 - Flags: approval-mozilla-beta+
Attachment #8551334 - Flags: approval-mozilla-aurora?
Attachment #8551334 - Flags: approval-mozilla-aurora+
Attachment #8553256 - Flags: approval-mozilla-beta?
Attachment #8553256 - Flags: approval-mozilla-beta+
Attachment #8553256 - Flags: approval-mozilla-aurora?
Attachment #8553256 - Flags: approval-mozilla-aurora+
Release Note Request (optional, but appreciated)
[Why is this notable]: New security feature.
[Suggested wording]: NPAPI plug-ins now sandboxed by default
[Links (documentation, blog post, etc)]:
relnote-firefox: --- → ?
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #22)
> Release Note Request (optional, but appreciated)
> [Why is this notable]: New security feature.
> [Suggested wording]: NPAPI plug-ins now sandboxed by default

The sandbox is preffed off by default, so I'm not sure whether we want a release note for this.

> [Links (documentation, blog post, etc)]:
Flags: needinfo?(benjamin)
Blocks: 1125891
This doesn't need to be relnoted unless/until we turn it on (bug 1125891 tracks).
Flags: needinfo?(benjamin)
Depends on: 1126570
You need to log in before you can comment on or make changes to this bug.