Closed Bug 1386558 Opened 2 years ago Closed 2 years ago

Sandboxing level 2 is no longer working correctly

Categories

(Core :: Security: Process Sandboxing, defect, P1, major)

All
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: gcp, Assigned: gcp)

References

Details

(Whiteboard: sb+)

Attachments

(1 file)

A number of reports where sandboxing level 3 broke things said they needed to go back to level 1, not 2, to get things working again.

Manual verification shows that this is true: setting sandboxing to level 2 has no effect. That's likely because the preference is checked before actually being available.
Whiteboard: sb?
Comment on attachment 8892817 [details]
Bug 1386558 - Check sandboxing level 2 after permissions are available.

https://reviewboard.mozilla.org/r/163804/#review169486

::: security/sandbox/linux/broker/SandboxBroker.cpp:217
(Diff revision 1)
>  
>    // Add a path permission on the dir itself so it can
>    // be opened. We're guaranteed to have a trailing / now,
>    // so just cut that.
>    path.Truncate(path.Length() - 1);
> +  if (!path.IsEmpty()) {

I'm feeling slightly embarrassed that I didn't notice this edge case.

::: security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp:227
(Diff revision 1)
>    MOZ_ASSERT(mCommonContentPolicy);
>    UniquePtr<SandboxBroker::Policy>
>      policy(new SandboxBroker::Policy(*mCommonContentPolicy));
>  
> +  // No read blocking at level 2 and below
> +  if (GetEffectiveContentSandboxLevel() <= 2) {

Could this get a comment explaining why it's down here in the per-process part?  The startup ordering problem is… subtle.
Attachment #8892817 - Flags: review?(jld) → review+
Comment on attachment 8892817 [details]
Bug 1386558 - Check sandboxing level 2 after permissions are available.

Requesting re-review because there was at least one other gotcha here. (Exiting without processing the write-path-whitelist first)
Attachment #8892817 - Flags: review+ → review?(jld)
Assignee: nobody → gpascutto
Blocks: 1308400
Priority: -- → P1
Whiteboard: sb? → sb+
Comment on attachment 8892817 [details]
Bug 1386558 - Check sandboxing level 2 after permissions are available.

https://reviewboard.mozilla.org/r/163804/#review170656

I'm just going to assume without checking the history that the early return cutting off the write whitelist was something I missed on an earlier review, so, sorry about that.  This looks good.
Attachment #8892817 - Flags: review?(jld) → review+
Pushed by gpascutto@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0dd9cbe575fe
Check sandboxing level 2 after permissions are available. r=jld
https://hg.mozilla.org/mozilla-central/rev/0dd9cbe575fe
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment on attachment 8892817 [details]
Bug 1386558 - Check sandboxing level 2 after permissions are available.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1308400
[User impact if declined]: No way to fall back sandboxing to level 2, must fall back to level 1. We want to disable bug 1308400 on beta (level 3 -> level 2) but due to this bug that is broken.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Landed yesterday
[Needs manual test from QE? If yes, steps to reproduce]: Yes, would be nice. See Bug 1385712, Bug 1385329 for user visible fallout. Setting about:config security.sandbox.content.level = 2 should fix those bugs (affecting old-style add-ons). It will require = 1 without this patch. Note that they are fixed by bug 1385891 on Nightly, but we don't want to uplift that.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Changes the order a preference is read.
[String changes made/needed]: NA
Attachment #8892817 - Flags: approval-mozilla-beta?
Comment on attachment 8892817 [details]
Bug 1386558 - Check sandboxing level 2 after permissions are available.

Sounds useful to keep particular extensions working in 56. Let's uplift for beta 2.
Attachment #8892817 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.