WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
140068
[Linux] SeccompBrokerClient should cache arbitrary file descriptors
https://bugs.webkit.org/show_bug.cgi?id=140068
Summary
[Linux] SeccompBrokerClient should cache arbitrary file descriptors
Michael Catanzaro
Reported
2015-01-04 19:38:39 PST
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.
Attachments
Patch
(5.73 KB, patch)
2015-01-04 19:54 PST
,
Michael Catanzaro
no flags
Details
Formatted Diff
Diff
[Linux] SeccompBrokerClient should cache arbitrary file descriptors
(5.77 KB, patch)
2015-03-23 15:55 PDT
,
Michael Catanzaro
no flags
Details
Formatted Diff
Diff
Patch
(5.05 KB, patch)
2015-07-15 07:59 PDT
,
Michael Catanzaro
no flags
Details
Formatted Diff
Diff
Patch
(5.05 KB, patch)
2015-07-15 08:05 PDT
,
Michael Catanzaro
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Michael Catanzaro
Comment 1
2015-01-04 19:54:23 PST
Created
attachment 243956
[details]
Patch
Michael Catanzaro
Comment 2
2015-03-13 08:48:15 PDT
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
Comment 3
2015-03-23 15:55:23 PDT
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
Comment 4
2015-07-14 11:04:01 PDT
(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 5
2015-07-15 03:13:56 PDT
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
Comment 6
2015-07-15 07:59:11 PDT
Created
attachment 256839
[details]
Patch
Michael Catanzaro
Comment 7
2015-07-15 08:05:47 PDT
Created
attachment 256840
[details]
Patch
WebKit Commit Bot
Comment 8
2015-07-15 09:02:45 PDT
Comment on
attachment 256840
[details]
Patch Clearing flags on attachment: 256840 Committed
r186839
: <
http://trac.webkit.org/changeset/186839
>
WebKit Commit Bot
Comment 9
2015-07-15 09:02:56 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug