Bug 140068 – [Linux] SeccompBrokerClient should cache arbitrary file descriptors

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Michael Catanzaro]

If malloc() attempts to open /proc/sys/vm/overcommit_memory in a SIGSYS signal handler, the SeccompBroker will attempt to recursively broker the open() syscall. Generalize the existing code that already handles the similar case where malloc() opens /sys/devices/system/cpu/online to handle this situation as well.

[Michael Catanzaro]

Created attachment 243956 [details]
Patch

[Michael Catanzaro]

So this works well enough for just open. But for the past few weeks I've been using a whitelist of calls to not trap, rather than a blacklist. This is risky because if any syscall gets trapped inside a call to malloc or free, we are doomed because they're not reentrant and we use them all over the place inside the SIGSYS handler.

So, a few things to try:

* A custom memory allocator, to parcel out preallocated memory, which we would have to use everywhere inside the signal handler implementation. I think this won't work because we use WebKit IPC, and would have to ensure that no memory is ever allocated by the IPC code, or at least goes through the custom allocator. Tricky since the allocator would need to be written without using any dynamic memory.
* LD_PRELOAD on malloc and free. Set up a new SIGSYS handler for just these two functions, to kill the process if SIGSYS arrives inside the function. Then if we could determine whether we're currently running in our main SIGSYS handler, we could allocate from a preallocated pool instead of getting more memory from the OS. That would slow down the entire web process and would be a bit tricky since now memory could have been allocated by either our custom allocator (which also can't use any dynamic memory!) or normal malloc. (Actually I'm not even sure if we can call normal malloc if we use LD_PRELOAD to replace it.)
* Completely give up on SIGSYS and switch to using ptrace instead.
* Or simply hope that I've whitelisted all syscalls that can be used inside malloc. (brk is the obvious one here.) This is actually not as bad an option as it seems, since it only fails if malloc uses syscall we don't expect, which is the failure case for the rest of the sandbox anyway.

[Michael Catanzaro]

Created attachment 249294 [details]
[Linux] SeccompBrokerClient should cache arbitrary file descriptors

If malloc() attempts to open /proc/sys/vm/overcommit_memory in a SIGSYS
signal handler, the SeccompBroker will attempt to recursively broker the
open() syscall. Generalize the existing code that already handles the
similar case where malloc() opens /sys/devices/system/cpu/online to
handle this situation as well.

[Michael Catanzaro]

(In reply to comment #2)
> * Or simply hope that I've whitelisted all syscalls that can be used inside
> malloc. (brk is the obvious one here.) This is actually not as bad an option
> as it seems, since it only fails if malloc uses syscall we don't expect,
> which is the failure case for the rest of the sandbox anyway.

This is the approach I decided to take. It's not robust, but nothing about this sandbox is, and it's working well in practice. Any of the other options would be quite complex to implement.

[Zan Dobersek]

Comment on attachment 249294 [details]
[Linux] SeccompBrokerClient should cache arbitrary file descriptors

View in context: https://bugs.webkit.org/attachment.cgi?id=249294&action=review

> Source/WebKit2/Shared/linux/SeccompFilters/SeccompBroker.cpp:64
> +    void cacheFile(const char* pathname);

Nit: I'd use 'path' or 'pathName'.

> Source/WebKit2/Shared/linux/SeccompFilters/SeccompBroker.cpp:199
> +    for (auto pair : m_fileDescriptorCache)
> +        close(pair.value);

Just iterate over m_fileDescriptorCache.values():

    for (int fd : m_fileDescriptorCache.values())
        close(fd);

> Source/WebKit2/Shared/linux/SeccompFilters/SeccompBroker.cpp:243
> +    auto iter = m_fileDescriptorCache.find(String(path));

Nit: the String ctor is not required, but I'd prefer this to be aligned in whichever way with the HashMap<>::set() call in ::cacheFile().

> Source/WebKit2/Shared/linux/SeccompFilters/SeccompBroker.cpp:258
> +void SeccompBrokerClient::cacheFile(const char* pathname)

Nit: same as above, I'd prefer 'path' or 'pathName', but it's not that critical.

[Michael Catanzaro]

Created attachment 256839 [details]
Patch

[Michael Catanzaro]

Created attachment 256840 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 256840 [details]
Patch

Clearing flags on attachment: 256840

Committed r186839: <http://trac.webkit.org/changeset/186839>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.