1246505 - Let the Windows low integrity content sandbox ride the trains.

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, enhancement)

Description

[Bob Owen (:bobowen)]

Just need to configure to build the content sandbox in all trees and set the level pref.

Comment 1

[Bob Owen (:bobowen)]

Attachment: Let the Windows low integrity content process sandbox ride the trains

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 8716978 [details] [diff] [review]
Let the Windows low integrity content process sandbox ride the trains

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

On the build system side, this is fine, but it's not my call to say whether this should be riding the trains.

Comment 3

[Bob Owen (:bobowen)]

Comment on attachment 8716978 [details] [diff] [review]
Let the Windows low integrity content process sandbox ride the trains

Thanks glandium.

Brad - I only know of bug 1189846 now that low integrity causes.

I have a plan to fix that and hope to get it into 47, but may not.
I very much doubt I'll land it in time for uplift to 46.

Although, I'm wondering whether it's getting too late to uplift the sandboxing anyway.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Jeff, can you comment about whether you want to uplift this to turn the sandbox on for Windows?

Comment 5

[Bob Owen (:bobowen)]

Extra context that I mentioned to Brad on IRC over the potential uplift to Aurora for this.

I'm also a bit nervous about requesting the uplift for turning on printing via the parent, just before my PTO.
This is the last patch on bug 1156742, which just flips the pref, but it's quite a big change to the way printing then works.

We'll need that and bug 1173371, before we could uplift this.

I've requested bug 1173371, but it hasn't been approved yet, although maybe it could come after, I doubt it would affect many people, particularly on Aurora.

Comment 6

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Bob Owen (:bobowen) from comment #5)
...
> I'm also a bit nervous about requesting the uplift for turning on printing
> via the parent, just before my PTO.
> This is the last patch on bug 1156742, which just flips the pref, but it's
> quite a big change to the way printing then works.

Thansk for the extra content. If you're not going to be around to fix followup issues, my preference is definitely to wait, for sandboxing   *and* printing.

> We'll need that and bug 1173371, before we could uplift this.
> 
> I've requested bug 1173371, but it hasn't been approved yet, although maybe
> it could come after, I doubt it would affect many people, particularly on
> Aurora.

Doesn't matter - if you uplift it to Aurora we have to deal with issues that arise later in Beta, and we need to make sure we track that.

Is it possible to run an experiment in beta 47 with a control group that doesn't have sandboxing enabled? If we do this early in Beta 47 we would have additional data that would inform a go/no-go.

Comment 7

[Bob Owen (:bobowen)]

(In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #6)
> (In reply to Bob Owen (:bobowen) from comment #5)

> If you're not going to be around to fix
> followup issues, my preference is definitely to wait, for sandboxing   *and*
> printing.

Thanks Jeff, we've stopped the uplift of bug 1173371 as well.
No reason to add any extra risk, as it's not needed unless we're at low integrity.

> Is it possible to run an experiment in beta 47 with a control group that
> doesn't have sandboxing enabled? If we do this early in Beta 47 we would
> have additional data that would inform a go/no-go.

Not sure how the experiments work, but if they can run from prefs, then there is a pref to set the "sandbox level".

They don't have any intrinsic meaning. They're just being incremented as we tighten the sandboxing rules, so that people can easily test potential regressions with a weaker sandbox.

Low integrity is set for level 1 and above.
Level 0 doesn't completely turn off the sandbox, but I could easily make it so that < 0 did.

Once we decide to ship we'd want to hard code the minimum level, as we don't want changes to prefs to be allowed to weaken the sandbox.
Then the only way of allowing a lower level or turning off altogether would be via an environment variable.

Comment 8

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Bob Owen (:bobowen) (less responsive 11th-18th) from comment #7)
> (In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #6)
> > (In reply to Bob Owen (:bobowen) from comment #5)
> 
> > If you're not going to be around to fix
> > followup issues, my preference is definitely to wait, for sandboxing   *and*
> > printing.
> 
> Thanks Jeff, we've stopped the uplift of bug 1173371 as well.
> No reason to add any extra risk, as it's not needed unless we're at low
> integrity.
> 
> > Is it possible to run an experiment in beta 47 with a control group that
> > doesn't have sandboxing enabled? If we do this early in Beta 47 we would
> > have additional data that would inform a go/no-go.
> 
> Not sure how the experiments work, but if they can run from prefs, then
> there is a pref to set the "sandbox level".

Experiments are actually add-ons so they're fairly flexible. We're running an e10s experiment starting the 12th of February that enables e10s for users that have zero add-ons, do not have accessibility features enabled and are not from a specific set of right-to-left locales (for example).

> They don't have any intrinsic meaning. They're just being incremented as we
> tighten the sandboxing rules, so that people can easily test potential
> regressions with a weaker sandbox.
> 
> Low integrity is set for level 1 and above.
> Level 0 doesn't completely turn off the sandbox, but I could easily make it
> so that < 0 did.
> 
> Once we decide to ship we'd want to hard code the minimum level, as we don't
> want changes to prefs to be allowed to weaken the sandbox.
> Then the only way of allowing a lower level or turning off altogether would
> be via an environment variable.

This is potentially problematic for a beta experiment - what we would need to do is land the code in beta that allows a negative value to disable sandboxing, *then* once the experiment is finished after say 2 weeks we would land an additional patch to remove this possibility.

I should add - we should only do an experiment if we have a hypothesis to test. For example, if we're concerned about the stability of sandboxing we would gather telemetry crash rates and crash signatures from crashstats and evaluate if stability was reduced in the sandboxed population vs the regular population, and use this to make a go/no-go decision about delaying sandboxing. If you don't think there is a stability (or performance) issue, then there's no need to experiment.

What do you think the risks are for performance and stability with sandboxing enabled? What data are you using to support your assertion?

Comment 9

[Bob Owen (:bobowen)]

(In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #8)
> (In reply to Bob Owen (:bobowen) (less responsive 11th-18th) from comment #7)
 
> > Low integrity is set for level 1 and above.
> > Level 0 doesn't completely turn off the sandbox, but I could easily make it
> > so that < 0 did.
> > 
> > Once we decide to ship we'd want to hard code the minimum level, as we don't
> > want changes to prefs to be allowed to weaken the sandbox.
> > Then the only way of allowing a lower level or turning off altogether would
> > be via an environment variable.
> 
> This is potentially problematic for a beta experiment - what we would need
> to do is land the code in beta that allows a negative value to disable
> sandboxing, *then* once the experiment is finished after say 2 weeks we
> would land an additional patch to remove this possibility.

Yeah, that's what I intended. :-)

I've filed bug 1250125 for turning off the sandbox with a negative pref.
 
> I should add - we should only do an experiment if we have a hypothesis to
> test. For example, if we're concerned about the stability of sandboxing we
> would gather telemetry crash rates and crash signatures from crashstats and
> evaluate if stability was reduced in the sandboxed population vs the regular
> population, and use this to make a go/no-go decision about delaying
> sandboxing. If you don't think there is a stability (or performance) issue,
> then there's no need to experiment.
> 
> What do you think the risks are for performance and stability with
> sandboxing enabled? What data are you using to support your assertion?

There is certainly a possibility that there could be stability or performance issues that we haven't found in Nightly.
There is no specific data gathered for sandboxing as it would more be its affect on other things.
Low integrity can affect various system calls.

At least if we get the affect of the pref changed now, we can decide later on the experiments.

Comment 10

[Bob Owen (:bobowen)]

This will also need to turn on printing via the parent.

Comment 11

[Bob Owen (:bobowen)]

Attachment: Let the Windows low integrity content process sandbox ride the trains

Only change here is to also enable printing via the parent, which I changed to Nightly only after the last patch was uploaded.

MozReview-Commit-ID: Ei4BWhLcYpA

Comment 12

[Bob Owen (:bobowen)]

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

Comment 13

[Jim Mathies [:jimm]]

Comment on attachment 8772480 [details] [diff] [review]
Let the Windows low integrity content process sandbox ride the trains

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

woot!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c40bfce8d35b
Let the Windows low integrity content process sandbox ride the trains. r=jimm

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c40bfce8d35b

Comment 16

[[:philipp]]

is this a change that we should document in the release notes for firefox 50?

Comment 17

[Bob Owen (:bobowen)]

(In reply to [:philipp] from comment #16)
> is this a change that we should document in the release notes for firefox 50?

As this is just a first step in content process sandboxing, we're deliberately not making big announcements about it.
However, I imagine it should probably go in the release notes.

Here is some text we gave to PR in case people noticed this and came asking for more information:

"Firefox on Windows is now running its content processes at low integrity as an initial sandboxing step. Windows integrity level checks limit the access that lower integrity processes have to higher integrity level resources.

What is the Windows Integrity mechanism?
https://msdn.microsoft.com/en-us/library/bb625957.aspx"


We might not want those last two lines and it'll possibly need rewording a bit, but hopefully that helps.
The idea was to keep it brief and factual, without making any wild security claims.

Comment 18

[Julien Cristau [:jcristau]]

Looks like this wasn't added to the 50 release notes, and it's a bit late now as we're about to ship 51.