Closed Bug 1403707 Opened 2 years ago Closed 2 years ago

Increase content sandbox job level to JOB_LOCKDOWN


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




Tracking Status
firefox57 --- unaffected
firefox58 --- fixed


(Reporter: handyman, Assigned: handyman)



(Whiteboard: sb+)


(1 file, 2 obsolete files)

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.

Attachment #8912892 - Flags: review?(bobowencode)
Comment on attachment 8912892 [details] [diff] [review]
Increase content sandbox job level to JOB_LOCKDOWN

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


::: 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.
Attachment #8912892 - Flags: review?(bobowencode) → review+
Whiteboard: sb+
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.
Keywords: checkin-needed
Pushed by
Change content sandbox job level to JOB_LOCKDOWN. r=bobowen
Keywords: checkin-needed
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
AFAIK, We are no longer at l4 on 57, so, updating the flags accordingly.
You need to log in before you can comment on or make changes to this bug.