TSIA.
Created attachment 189321 [details] wip
Comment on attachment 189321 [details] wip View in context: https://bugs.webkit.org/attachment.cgi?id=189321&action=review > Source/WebKit2/WebProcess/gtk/SeccompFiltersWebProcessGtk.cpp:8 > +/* > + * Copyright (C) 2013 Intel Corporation. All rights reserved. > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions > + * are met: > + * 1. Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. Is there anything GTK+ specific about this file or could it be shared with EFL?
Comment on attachment 189321 [details] wip View in context: https://bugs.webkit.org/attachment.cgi?id=189321&action=review >> Source/WebKit2/WebProcess/gtk/SeccompFiltersWebProcessGtk.cpp:8 >> + * notice, this list of conditions and the following disclaimer. > > Is there anything GTK+ specific about this file or could it be shared with EFL? The policies shared by EFL, GTK and Qt are being added by addDefaultWebProcessPolicy (see bug 89875). Because of the device APIs we have on EFL by default (battery, network status, vibration, etc), I had to add some extra policies specific to EFL. Qt has a few unique policies for fonts and non-default Qt libraries path. The trend is to fine tune policies according to features each port support and enable over time. This patch is just an stub to show the amount of code need by GTK for enabling this sandbox. If there is interest from the WebKit GTK project on this sandbox, the next step would be try to run the layout tests and see what directories and files are missing from the policies.
It seems that once this is working, we should really activate it by default on Linux -- no configuration option. Other operating systems will just have it disabled.
Let's get this done.
Working on this, there was this one rt_sigprocmask syscall that wasn't handled by the seccomp rule: rt_sigprocmask(SIG_BLOCK, ~[RTMIN RT_1], [], 8) I've avoided this by adjusting the addition of the rt_sigprocmask rule so it traps any SIG_BLOCK call: > filters->addRule("rt_sigprocmask", SeccompFilters::Trap, > 0, SeccompFilters::Equal, SIG_BLOCK, >- 1, SeccompFilters::NotEqual, 0); >+ 0, SeccompFilters::NotSet, 0); As for the additional policy permissions that must be granted via the ADD_POLICY macro, I find that the list must be both quite extensive and specific. For instance, there are multiple cache directories that must be specified, like fontconfig cache and even the ~/.nv directory that's accessed when using the proprietary nVidia drivers. I personally find this a bit impractical and hope that there's a more elegant solution to this. I'll reiterate the patch a bit and upload the first in-progress version.
(In reply to comment #6) > Working on this, there was this one rt_sigprocmask syscall that wasn't > handled by the seccomp rule: > rt_sigprocmask(SIG_BLOCK, ~[RTMIN RT_1], [], 8) > > I've avoided this by adjusting the addition of the rt_sigprocmask rule so it > traps any SIG_BLOCK call: > > filters->addRule("rt_sigprocmask", SeccompFilters::Trap, > > 0, SeccompFilters::Equal, SIG_BLOCK, > >- 1, SeccompFilters::NotEqual, 0); > >+ 0, SeccompFilters::NotSet, 0); I'm confused -- rt_sigprocmask(SIG_BLOCK, ~[RTMIN RT_1], [], 8) should already be blocked because ~[RTMIN RT_1] is not 0. Doesn't your modified rule only additionally block calls where the second parameter is 0 (useless calls)? > the ~/.nv directory that's > accessed when using the proprietary nVidia drivers. Does it need both read and write access to ~/.nv, or just read? > I personally find this a > bit impractical and hope that there's a more elegant solution to this. By nature, it requires knowledge of the changing implementation details of all our dependencies. Even if we trap just open, openat, and creat (which is insufficient), it will break all the time. A couple examlpes: since this sandbox was originally written, glibc has started reading /proc/sys/vm/overcommit_memory, which now affects the operation of malloc(). But it's not on the whitelist, because it wasn't needed two years ago. And the breakage will be distro-dependent. E.g. when DRI3 is in use (as in the latest Ubuntu and Fedora), mesa creates files for shared memory in a distro-dependent location: on Debian/Ubuntu they go in /dev/shm, which is already in our whitelist, but on Fedora/openSUSE/Arch they go into /var/tmp, which is not in our whitelist and broke accelerated compositing. My guess is that the fastest-moving distros (Arch, Fedora) will hit a couple such problems every couple of years. They'll be easy to fix once diagnosed. FWIW, Chromium's sandbox (which is much more complicated than ours, and more secure) has the same problem [1]. [1] https://chromium.googlesource.com/chromium/src/+/4e3c98a7788ba99f5dd72c87c9ba84e69fbd1bbf%5E%21
Created attachment 243959 [details] Patch
FWIW, layout tests require ~3.5% longer to complete on my machine with seccomp filters enabled, brokering only open, openat, and creat. Performance will get worse when we start brokering more system calls (which is high priority for me) but I also think it's a very good trade-off.
Created attachment 249293 [details] [GTK] Implement seccomp filter support
Created attachment 250110 [details] patch
Created attachment 256695 [details] [GTK] Implement seccomp filter support
Comment on attachment 256695 [details] [GTK] Implement seccomp filter support (Accidentally marked for review before the dependent patches were ready.)
Created attachment 256883 [details] Patch
Note: the GStreamer policies in SeccompFiltersWebProcessGtk get moved to cross-platform code in a later patch; for this patch it's easiest to keep them there.
Comment on attachment 256883 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256883&action=review The EWS isn't processing the patch, otherwise looks good. > Source/WebKit2/WebProcess/gtk/SeccompFiltersWebProcessGtk.h:43 > + virtual void platformInitialize(); override
(In reply to comment #16) > Comment on attachment 256883 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=256883&action=review > > The EWS isn't processing the patch, otherwise looks good. Ah, I broke the jhbuild module when copying it from EFL; they use tarballs for GitHub (my preference) but we use git clones (whatever). Thanks for catching it; I might not have noticed. > > Source/WebKit2/WebProcess/gtk/SeccompFiltersWebProcessGtk.h:43 > > + virtual void platformInitialize(); > > override Good catch
Created attachment 257057 [details] Patch
Comment on attachment 257057 [details] Patch Clearing flags on attachment: 257057 Committed r187011: <http://trac.webkit.org/changeset/187011>
All reviewed patches have been landed. Closing bug.