Closed
Bug 1186158
Opened 9 years ago
Closed 9 years ago
Receive notifications of sandbox violations in the browser on OS X
Categories
(Core :: Security: Process Sandboxing, defect)
Tracking
()
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: smichaud, Assigned: smichaud)
References
Details
Attachments
(1 file, 1 obsolete file)
9.48 KB,
patch
|
spohl
:
review+
|
Details | Diff | Splinter Review |
Recently I discovered that this is possible in principle: sandboxd calls notify_post("com.apple.sandbox.violation.*") every time it sends a sandbox violation report to syslogd. The notification subsystem to which this call belongs is documented in /usr/include/notify.h. I'm going to try to write a patch here that does this. If we had this kind of information, we could (for example) crash the content process or warn the user on sandbox violations.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → smichaud
Assignee | ||
Comment 1•9 years ago
|
||
This patch is pretty minimal -- it just logs sandbox violations to stdout and the system console. But it shows that we *can* log them, and the limitations of trying to do so. Just to make sure people don't see these logs accidentally, I've preffed off my patch. I've also made sure it only "works" on OS X 10.9 and up, and then only in non-release builds. I was afraid ASL queries wouldn't be very performant, but I was pleasantly surprised -- in my tests they take at most a second or two. A more serious problem is that the notifications themselves are sometimes sent (by sandboxd) several 10s of seconds after a sandbox violation. But when this happens, the notification that sandboxd sends to syslogd is also very late. The bottom line, as best I can tell, is that we'll never receive these notifications synchronously with the actual sandbox violations.
Attachment #8638220 -
Flags: review?(spohl.mozilla.bugs)
Comment 2•9 years ago
|
||
Comment on attachment 8638220 [details] [diff] [review] Proof of concept patch Review of attachment 8638220 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/app/profile/firefox.js @@ +1259,5 @@ > > +#if defined(XP_MACOSX) && defined(MOZ_SANDBOX) > +// If this pref is on, log sandbox violations to stdout and the system console. > +// This is currently only supported in non-release builds. See bug 1186158. > +pref("security.sandbox.mac.track.violations", false); Can we simply omit this from this file? Having this here will show this pref in about:config, but I don't believe this is user-facing enough to warrant having it there. What do you think? ::: widget/cocoa/nsSandboxViolationSink.mm @@ +47,5 @@ > + > +// ViolationHandler() is always called on its own secondary thread. This > +// makes it unlikely it will interfere with other browser activity. > + > +#define ONLY_LOG_OURS 1 Would it make sense to make this a pref in about:config as well? This would require a restart of Firefox as it stands, but would be easier than a recompile. If we keep this, I'd prefer a name like "ONLY_LOG_OUR_VIOLATIONS". @@ +70,5 @@ > + asl_set_query(query, ASL_KEY_REF_PID, query_pid, ASL_QUERY_OP_EQUAL); > +#endif > + aslresponse response = asl_search(nullptr, query); > + > + // Each time ViolationHandler() is called we grab as many of "our" messages Should this be "[...] as many messages as are available."? "our messages" only applies if we're filtering for our own violations, right? @@ +73,5 @@ > + > + // Each time ViolationHandler() is called we grab as many of "our" messages > + // as are available. Otherwise we might not get them all. > + if (response) { > + for (;;) { nit: |while (true)| @@ +75,5 @@ > + // as are available. Otherwise we might not get them all. > + if (response) { > + for (;;) { > + aslmsg found; > + const char *id_str; nit: s/const char *id_str/const char* id_str/ @@ +77,5 @@ > + for (;;) { > + aslmsg found; > + const char *id_str; > + > + for (found = nullptr, id_str = nullptr; ;) { Let's change this to: aslmsg hit = nullptr; aslmsg found = nullptr; const char* id_str = nullptr; while (hit = aslresponse_next(response)) { ... } and let's remove the |if (!hit)| check at the beginning of the loop. @@ +97,5 @@ > + if (!found) { > + break; > + } > + > + const char *pid_str = asl_get(found, ASL_KEY_REF_PID); nit: s/const char *pid_str/const char* pid_str/ @@ +98,5 @@ > + break; > + } > + > + const char *pid_str = asl_get(found, ASL_KEY_REF_PID); > + const char *message_str = asl_get(found, ASL_KEY_MSG); nit: s/const char *message_str/const char* message_str/
Attachment #8638220 -
Flags: review?(spohl.mozilla.bugs)
Assignee | ||
Comment 3•9 years ago
|
||
OK, then, how's this? :-) I basically agree with all your suggestions.
Attachment #8638220 -
Attachment is obsolete: true
Attachment #8638774 -
Flags: review?(spohl.mozilla.bugs)
Updated•9 years ago
|
Attachment #8638774 -
Attachment is patch: true
Comment 4•9 years ago
|
||
Comment on attachment 8638774 [details] [diff] [review] Proof of concept patch, rev1 Review of attachment 8638774 [details] [diff] [review]: ----------------------------------------------------------------- Looks great, thanks! :-)
Attachment #8638774 -
Flags: review?(spohl.mozilla.bugs) → review+
Comment 6•9 years ago
|
||
Backed out for OSX Werror bustage. https://treeherder.mozilla.org/logviewer.html#?job_id=12158424&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/8ed1cf289546
Assignee | ||
Comment 7•9 years ago
|
||
Sigh. I'll fix it.
Assignee | ||
Comment 8•9 years ago
|
||
This is all it took: -+ while (hit = aslresponse_next(response)) { ++ while ((hit = aslresponse_next(response))) { Stephen has already given me an r+ for this change on IRC.
Assignee | ||
Comment 9•9 years ago
|
||
But now mozilla-inbound is closed. I'll wait until it reopens.
Comment 11•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/63c709f71ce3
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in
before you can comment on or make changes to this bug.
Description
•