Seccomp sandbox violation: sys_unlink called in content process of Firefox desktop

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: tedd, Assigned: tedd)

Tracking

unspecified
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: sblc1)

Attachments

(3 attachments)

Posted file Seccomp error log
When enabling content sandboxing on Linux desktop (using ac_add_options --enable-content-sandbox), the content process crashes early on due to a seccomp violation when calling sys_unlink.

Attached is the error log, I will attach another file which shows a better backtrace using debug symbols.

The code in question seems to be related to shared memory [1].

It seems unlink was removed from the whitelist roughly one year ago (Bug 906996), but I think it was b2g related.

The straight forward way would be to add unlink back to the whitelist, but maybe someone knows the code better and there is a way to avoid this.

[1] https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/ipc/chromium/src/base/shared_memory_posix.cc#162
(Assignee)

Comment 2

3 years ago
In the first milestone for desktop Linux content sandboxing, we want to enable seccomp on nightly. This is currently listed as a blocker.

I think the way to go here would be extending the file broker to handle unlinks, which I guess will be used to fix Bug 936274. 

Extending the file broker for unlinks would require some additional work, because right now the broker operates on existing paths (as far as I know) and the file created/unlinked here is a temporary file created with mkstemp() [1] which includes 6 random characters.

This requires the broker to be able to allow read/write access based on directory permissions. But this should require too much work, but needs to be done with care to prevent directory traversals etc.

As far as this bug is concerned, I would say that we add unlink() to the seccomp whitelist, and remove it along with open() as part of the second milestone along with Bug Bug 936274.

What is your opinion on this :jld?

[1] https://dxr.mozilla.org/mozilla-central/rev/6202ade0e6d688ffb67932398e56cfc6fa04ceb3/ipc/chromium/src/base/shared_memory_posix.cc#157
Flags: needinfo?(jld)
(Assignee)

Comment 3

3 years ago
Sorry I meant to say "shouldn't require". For adding the unlink() capability we can file a separate bug and make it part of sblc2.
(In reply to Julian Hector [:tedd] [:jhector] from comment #2)
> As far as this bug is concerned, I would say that we add unlink() to the
> seccomp whitelist, and remove it along with open() as part of the second
> milestone along with Bug Bug 936274.

That sounds fine.  I didn't intend to remove unlink() for desktop, but I wasn't really thinking about desktop at all when I made that change.

Longer-term, since we control the IPC shared memory code, we could replace it to a call directly to the sandbox broker.  Or we could make it try O_TMPFILE first, since that way it's just one system call to intercept with seccomp-bpf instead of open+unlink (and the broker can use open+unlink to simulate it if necessary, but that would be transparent to the broker client).
Flags: needinfo?(jld)
(Assignee)

Comment 5

3 years ago
Ok thanks. Then I will get a patch ready to add unlink() back to the whitelist.
Assignee: nobody → julian.r.hector
Attachment #8738706 - Flags: review?(jld) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Comment 9

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/43a6c26b28ae
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.