942696 - Remove access() from seccomp-bpf whitelist for Linux/Desktop

Status: RESOLVED DUPLICATE of bug 936274

(Core :: Security: Process Sandboxing, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

Attachment: remove_access_from_whitelist_stacktrace.txt

Removing the access() syscall from the whitelist reveals that we have to remote functions in nsLocalFileUnix [1].
Probably not only ::Exists, but also ::IsWriteAble, ::IsReadAble, etc.

[1] http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsLocalFileUnix.cpp#1382

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_942696_remove_access_from_whitelist.patch

Since |access| is used explicitly in nsLocalFile::Exists [1], I thought the easiest solution might be to just remote the call to access for nsLocalFile. I modifed PNecko.ipdl to realize that, which works in nsLocalFile::Exists. Unfortunately |access| is further used in [2], which seems to be not so easy to remote.

Kang, would you suggest a completely different approach to remote |access|? Any other ideas/guidance?

[1] http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsLocalFileUnix.cpp#1382
[2] http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/linking/prlink.c#803

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: access_stacktrace2.txt

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #1)
> Unfortunately |access| is further used in [2], which seems to be not so easy
> to remote.

Adding stacktrace for the next use of |access|.

Comment 3

[Guillaume Destuynder [:kang] [this account is no longer active]]

generally using access is a good idea before accessing the file for proper error handling in particular.
i believe that in general we should remote the resource accesses:

- The parent process makes decisions on resource accesses AND enforces them (in an ideal world, it'd do just that)
- Mainly, the parent process enforces instead of the OS because of platform compatibility issues, eg, SELinux/similar doesn't exist on Windows)

That means remoting all fs functions (resource access decision) and passing a fd (resource access enforcement) is the way to go, IMO - but that also means having a policy handled in the parent process, with something that associates subjects (tabs) to objects (resources, here, fd's/path), so it's a big change.

The alternative, which is what has been done so far, is to make it so that whatever calls nsLocalFileUnix functions is entirely remoted, instead of the calls themselves. While it's not as nice: easier and no policy file needed.

Comment 4

[Guillaume Destuynder [:kang] [this account is no longer active]]

There's more I wanted to add, but I was on a flight and connection was pretty spotty.. here goes:

Another issue is that when remoting a whole API then the code runs in the parent, which is trusted - it may be used to compromise the parent if there's bugs in the code.

Thus, while we've generally be moving stuff to the parent for simplicity - sometimes the decision is tricky and depends on the API. There are other APIs which we'd actually trying to move back to the child due to this.

Maybe we should really start thinking about the policy handling in the parent.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8339498 [details] [diff] [review]
bug_942696_remove_access_from_whitelist.patch

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

I do not understand the goal here.

My first thought is that we should create a remoted file:// protocol handler that streams the file contents over IPC. (AFAICT, there is no reason that file:// has to be optimized like app:// has to; if something is fast enough for http:// then it is fast enough for file://). Similarly, it seems like the DOM File API and <input type=file> need to be remoted, if they aren't already. But, it seems like higher-level APIs for each of those things would be better than defining a low-level remoting of nsLocalFile.

Also, since access() is more-or-less benign and easy, I suggest we figure out what to do about the other syscalls that are less benign first, and then fix access() once we have a plan for them. That means, for example, if we're going to get rid of open() we should have a comprehensive list of everywhere we call open() in the child process and a plan for eliminating all of those uses. Otherwise, we might end up doing a lot of work to implement 90% of an approach that falls apart at the very end.

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

BTW, calling access() before open() also seems like a waste to me. 99.9% of the time, open() is going to succeed and the results from access() won't matter, right? Usually it would be better to call open() optimistically and then maybe call access() in the error handling logic when open() fails, if the error code from open() doesn't do a good enough job of explaining the issue.

Comment 7

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

Move process sandboxing bugs to their new, separate component.

(Sorry for the bugspam; filter on 3c21328c-8cfb-4819-9d88-f6e965067350.)

Comment 8

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

This is going to wind up using the broker from bug 930258 to take care of {open, access, stat, lstat} at the same time, so we won't need separate bugs.