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

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Bob Owen (:bobowen)]

Plugin processes should not need Administrator rights.

Comment 1

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5abbc1431ef8

Comment 2

[Bob Owen (:bobowen)]

Attachment: Part 1: Enable an open sandbox on Windows NPAPI processes.

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.

Comment 3

[Bob Owen (:bobowen)]

Attachment: Part 2: Use the USER_NON_ADMIN access token level for Windows NPAPI processes.

Kept the setting of the USER_NON_ADMIN token level separate for bisection purposes.

Comment 4

[Bob Owen (:bobowen)]

Beta try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b82e8d5143b

Comment 5

[Chris Peterson [:cpeterson]]

[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.

Comment 6

[Bob Owen (:bobowen)]

Try push with default and optional per plugin prefs:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c53dd3a18ba

Comment 7

[Bob Owen (:bobowen)]

Attachment: Part 3: Add prefs for the Windows NPAPI process sandbox.

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.

Comment 8

[Benjamin Smedberg]

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.

Comment 9

[Bob Owen (:bobowen)]

Attachment: Part 3: Add prefs for the Windows NPAPI process sandbox. v2

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

Comment 10

[Bob Owen (:bobowen)]

Attachment: Part 3: Add prefs for the Windows NPAPI process sandbox. v3

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

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Tracking. Expected for beta 4.

Comment 12

[Bob Owen (:bobowen)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3fba7dde5a94
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/03ba45deb84f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c3bb3f04aa2f

Comment 13

[Bob Owen (:bobowen)]

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

Comment 14

[Bob Owen (:bobowen)]

Attachment: Beta Part 3: Add prefs for the Windows NPAPI process sandbox.

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

Comment 15

[Benjamin Smedberg]

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.

Comment 16

[Bob Owen (:bobowen)]

(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.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3fba7dde5a94
https://hg.mozilla.org/mozilla-central/rev/03ba45deb84f
https://hg.mozilla.org/mozilla-central/rev/c3bb3f04aa2f

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Bob, we would like this patch in beta 4. Could you fill the uplift requests to make sure it lands before Monday afternoon. Thanks

Comment 19

[Bob Owen (:bobowen)]

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.

Comment 20

[Bob Owen (:bobowen)]

Comment on attachment 8551334 [details] [diff] [review]
Part 2: Use the USER_NON_ADMIN access token level for Windows NPAPI processes.

See comment 19.

Comment 21

[Bob Owen (:bobowen)]

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.

Comment 22

[Lawrence Mandel [:lmandel] (use needinfo)]

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)]:

Comment 23

[Bob Owen (:bobowen)]

(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)]:

Comment 24

[Bob Owen (:bobowen)]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/20dbbb1fd98a
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/f79836f9957e
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/171f05f3d01c

Comment 25

[Bob Owen (:bobowen)]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/2ab5add95717
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/f7b5148c84a1
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/9bfc57be3f2c

Comment 26

[Benjamin Smedberg]

This doesn't need to be relnoted unless/until we turn it on (bug 1125891 tracks).