1403707 - Increase content sandbox job level to JOB_LOCKDOWN

Status: RESOLVED FIXED

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

Description

[David Parks [:handyman]]

We are currently at content sandbox level 4, which has job level JOB_RESTRICTED.  JOB_LOCKDOWN is JOB_RESTRICTED + JOB_OBJECT_LIMIT_DIE_ON_UNHANDLED_EXCEPTION [1].  This bug is just to strengthen the sandbox.

The limit's docs are:
"If an exception occurs and the system calls the UnhandledExceptionFilter function, the debugger will be given a chance to act. If there is no debugger, the functions returns EXCEPTION_EXECUTE_HANDLER. Normally, this will cause termination of the process with the exception code as the exit status."
So this is about disabling the window that asks about Windows error reporting.  I don't have an STR for this -- I don't know that I've seen the content process crash in a way that exposed the dialog.

We use the flag that underlies this flag (SEM_NOGPFAULTERRORBOX) in SetErrorMode calls in two places -- both are predicated on XRE_NO_WINDOWS_CRASH_DIALOG being defined.  I'm leaving the flag for (1) the other processes and (2) it also blocks the "find file..." dialog that comes up when the system looks for a file but can't find it (SEM_NOOPENFILEERRORBOX).

GetErrorMode in content (and parent) is mostly just SEM_FAILCRITICALERRORS (although Win32 functions frequently, briefly, change it to finish their operations).  This job flag doesn't seem to change that so I'm skeptical that it is working but it may just be implemented separately.

[1] http://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/security/sandbox/chromium/sandbox/win/src/job.cc#39

Comment 1

[David Parks [:handyman]]

Attachment: Increase content sandbox job level to JOB_LOCKDOWN

Tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d3ba16d364178046c053f5d0211687963a7a5e6

Comment 2

[Bob Owen (:bobowen)]

Comment on attachment 8912892 [details] [diff] [review]
Increase content sandbox job level to JOB_LOCKDOWN

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

Thanks.

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp
@@ +362,5 @@
>      jobLevel = sandbox::JOB_LOCKDOWN;
>      accessTokenLevel = sandbox::USER_LOCKDOWN;
>      initialIntegrityLevel = sandbox::INTEGRITY_LEVEL_LOW;
>      delayedIntegrityLevel = sandbox::INTEGRITY_LEVEL_UNTRUSTED;
> +  } else if (aSandboxLevel >= 5) {

Given that this hasn't shipped yet I think we should just add this to level 4.

Comment 3

[David Parks [:handyman]]

Attachment: Change content sandbox job level to JOB_LOCKDOWN (r=bobowen)

This one just changes level 4 to JOB_LOCKDOWN.

Tests:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=b85fd5a8a4dd909f175b864c421eccf85ea4f307

Comment 4

[Bob Owen (:bobowen)]

Comment on attachment 8913318 [details] [diff] [review]
Change content sandbox job level to JOB_LOCKDOWN (r=bobowen)

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

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp
@@ +367,5 @@
>      jobLevel = sandbox::JOB_RESTRICTED;
>      accessTokenLevel = sandbox::USER_LIMITED;
>      initialIntegrityLevel = sandbox::INTEGRITY_LEVEL_LOW;
>      delayedIntegrityLevel = sandbox::INTEGRITY_LEVEL_LOW;
> +  } else if (aSandboxLevel >= 4) {

You still need to get rid of the >= 10 above or that would weaken the sandbox.

Comment 5

[David Parks [:handyman]]

Attachment: Change content sandbox job level to JOB_LOCKDOWN (r=bobowen)

Boy, that was dumb.  Here we go.

Tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0390e01e071a792e38e3cdb6f5abfedb299a57a0

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d018ea90b0c2
Change content sandbox job level to JOB_LOCKDOWN. r=bobowen

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 8

[Sylvestre Ledru [:Sylvestre]]

AFAIK, We are no longer at l4 on 57, so, updating the flags accordingly.