1445167 - Make chromium sandbox CHECK and LOG_FATAL messages crash the process.

Status: RESOLVED FIXED

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

Description

[Bob Owen (:bobowen)]

This is blocked by the change to make USER_NON_ADMIN a blacklist in bug 1321724.
USER_NON_ADMIN is only now used by default for the file content process and in bug 1344453 we added a rule to allow read access to all files, so the change to USER_NON_ADMIN should no longer be required.

Try push with everything passing on Linux and some Windows failures due to the USER_NON_ADMIN change:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0dd5ad56e120d8417cec7883c2d237ace5138e0d

Try push with the USER_NON_ADMIN change backed out:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bea3a99bb6d43eabb6dafa576ade44dda6abcff1

Comment 1

[Bob Owen (:bobowen)]

I realised my initial OFFICIAL_BUILD patch was windows only:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7005c2b09e61d77a6d058e6a54e93bc44efc894

Comment 2

[Bob Owen (:bobowen)]

Attachment: Part 1: Revert change to make USER_NON_ADMIN a blacklist

This is only used by default in the file content process now and we also have a
FILES_ALLOW_READONLY rule for all paths anyway.

Comment 3

[Bob Owen (:bobowen)]

Attachment: Part 2: Make LOG_FATAL messages in chromium sandbox code crash

Comment 4

[Bob Owen (:bobowen)]

Attachment: Part 3: Define OFFICIAL_BUILD in chromium sandbox code when MOZILLA_OFFICIAL

This potentially optimizes CHECK code.

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8958405 [details] [diff] [review]
Part 2: Make LOG_FATAL messages in chromium sandbox code crash

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

::: security/sandbox/chromium-shim/base/logging.cpp
@@ +116,5 @@
>  }
>  
>  LogMessage::~LogMessage() {
> +  if (severity_ == LOG_FATAL) {
> +    MOZ_CRASH("Received fatal chromium sandbox message.");

Nit: this sounds like it's referring to an IPC message; maybe say something like “assertion failed” instead of “received message”?

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8958406 [details] [diff] [review]
Part 3: Define OFFICIAL_BUILD in chromium sandbox code when MOZILLA_OFFICIAL

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

MOZILLA_OFFICIAL is certainly not the right knob for this. I think it should be tied to some condition that makes some MOZ_*_ASSERT enabled (but which one? MOZ_ASSERT? MOZ_DISAGNOSTIC_ASSERT?)

Comment 7

[Bob Owen (:bobowen)]

Comment on attachment 8958406 [details] [diff] [review]
Part 3: Define OFFICIAL_BUILD in chromium sandbox code when MOZILLA_OFFICIAL

After talking with glandium, we're still not quite sure when to set OFFICIAL_BUILD, so I'm going to leave that until bug 1055227, because I want to get the actual crashing landed.

Comment 8

[Bob Owen (:bobowen)]

(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #5)
...
> > +    MOZ_CRASH("Received fatal chromium sandbox message.");
> 
> Nit: this sounds like it's referring to an IPC message; maybe say something
> like “assertion failed” instead of “received message”?

Yes, I agree, thanks.
I've changed this to "Hit fatal chromium sandbox condition.".
I've used condition instead of assertion just to avoid confusion with our assertions and static ones.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4c3c038006b
Part 1: Revert change to make USER_NON_ADMIN a blacklist. r=handyman
https://hg.mozilla.org/integration/mozilla-inbound/rev/78dc6b0c4798
Part 2: Make LOG_FATAL messages in chromium sandbox code crash. r=jld

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/e4c3c038006b
https://hg.mozilla.org/mozilla-central/rev/78dc6b0c4798

Comment 11

[Bob Owen (:bobowen)]

Comment on attachment 8958404 [details] [diff] [review]
Part 1: Revert change to make USER_NON_ADMIN a blacklist

Approval Request Comment
[Feature/Bug causing the regression]:
Windows chromium sandbox.

[User impact if declined]:
There is a chance that we might not be crashing safely, when the chromium sandbox code hits a condition that it believes should cause a crash.

[Is this code covered by automated tests?]:
The chromium sandbox code itself, is used in nearly all tests with child processes.

[Has the fix been verified in Nightly?]:
We don't have STR, this is a hygiene fix.

[Needs manual test from QE? If yes, steps to reproduce]: 
No

[List of other uplifts needed for the feature/fix]:
Other patch in this bug.

[Is the change risky?]:
Slightly

[Why is the change risky/not risky?]:
The change has a backout for some code that is now only used for the file:// page process and a very simple change to crash on a fatal log message. These are situations where we should have been crashing now but are not and we haven't seen any of these crashes in Nightly yet.

[String changes made/needed]:
None

Comment 12

[Bob Owen (:bobowen)]

Comment on attachment 8958405 [details] [diff] [review]
Part 2: Make LOG_FATAL messages in chromium sandbox code crash

See comment 11.

Comment 13

[Julien Cristau [:jcristau]]

Comment on attachment 8958404 [details] [diff] [review]
Part 1: Revert change to make USER_NON_ADMIN a blacklist

sandbox hardening, beta60+

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0c055f790f2e
https://hg.mozilla.org/releases/mozilla-beta/rev/0bea88fd85ba