Make chromium sandbox CHECK and LOG_FATAL messages crash the process.

RESOLVED FIXED in Firefox 60

Status

()

defect
P1
normal
RESOLVED FIXED
Last year
3 months ago

People

(Reporter: bobowen, Assigned: bobowen)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Trunk
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed, firefox61 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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
I realised my initial OFFICIAL_BUILD patch was windows only:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7005c2b09e61d77a6d058e6a54e93bc44efc894
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.
Attachment #8958404 - Flags: review?(davidp99)
This potentially optimizes CHECK code.
Attachment #8958406 - Flags: review?(mh+mozilla)
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”?
Attachment #8958405 - Flags: review?(jld) → review+
Attachment #8958404 - Flags: review?(davidp99) → review+
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?)
Attachment #8958406 - Flags: review?(mh+mozilla)
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.
Attachment #8958406 - Attachment is obsolete: true
(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.
Pushed by bobowencode@gmail.com:
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 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
Attachment #8958404 - Flags: approval-mozilla-beta?
Comment on attachment 8958405 [details] [diff] [review]
Part 2: Make LOG_FATAL messages in chromium sandbox code crash

See comment 11.
Attachment #8958405 - Flags: approval-mozilla-beta?
Comment on attachment 8958404 [details] [diff] [review]
Part 1: Revert change to make USER_NON_ADMIN a blacklist

sandbox hardening, beta60+
Attachment #8958404 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8958405 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1449480
No longer blocks: 1466097
Depends on: 1466097
Depends on: 1555076
No longer depends on: 1555076
You need to log in before you can comment on or make changes to this bug.