Closed
Bug 1445167
Opened 6 years ago
Closed 6 years ago
Make chromium sandbox CHECK and LOG_FATAL messages crash the process.
Categories
(Core :: Security: Process Sandboxing, defect, P1)
Core
Security: Process Sandboxing
Tracking
()
RESOLVED
FIXED
mozilla61
People
(Reporter: bobowen, Assigned: bobowen)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
13.93 KB,
patch
|
handyman
:
review+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
1.29 KB,
patch
|
jld
:
review+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•6 years ago
|
||
I realised my initial OFFICIAL_BUILD patch was windows only: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7005c2b09e61d77a6d058e6a54e93bc44efc894
Assignee | ||
Comment 2•6 years ago
|
||
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)
Assignee | ||
Comment 3•6 years ago
|
||
Attachment #8958405 -
Flags: review?(jld)
Assignee | ||
Comment 4•6 years ago
|
||
This potentially optimizes CHECK code.
Attachment #8958406 -
Flags: review?(mh+mozilla)
Comment 5•6 years ago
|
||
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+
Updated•6 years ago
|
Attachment #8958404 -
Flags: review?(davidp99) → review+
Comment 6•6 years ago
|
||
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)
Assignee | ||
Comment 7•6 years ago
|
||
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
Assignee | ||
Comment 8•6 years ago
|
||
(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 10•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e4c3c038006b https://hg.mozilla.org/mozilla-central/rev/78dc6b0c4798
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee | ||
Comment 11•6 years ago
|
||
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?
Assignee | ||
Comment 12•6 years ago
|
||
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?
Updated•6 years ago
|
status-firefox60:
--- → affected
Comment 13•6 years ago
|
||
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+
Updated•6 years ago
|
Attachment #8958405 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 14•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/0c055f790f2e https://hg.mozilla.org/releases/mozilla-beta/rev/0bea88fd85ba
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•