Bug 89875 – [WK2] Drop WebProcess capabilities on Linux using seccomp filters

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

[Thiago Marcos P. Santos]

This feature needs deep investigation, but looks like we can achieve similar design as the Mac sandbox using seccomp with filters on Linux. We might be able to block the WebProcess from opening files (delegating the task to the UIProcess and perform some policy check) and/or constrain the file access to certain files/folders.

[Guillaume Emont]

An alternative that might require less work and could be a first step to have some sandboxing would be to use setuid-sandbox[1], which is the standalone version of the sandbox used in Chrome on Linux (though they are working on using seccomp too AFAIK).

[1] http://code.google.com/p/setuid-sandbox/

[Thiago Marcos P. Santos]

(In reply to comment #1)
> An alternative that might require less work and could be a first step to have some sandboxing would be to use setuid-sandbox[1], which is the standalone version of the sandbox used in Chrome on Linux (though they are working on using seccomp too AFAIK).
> 
> [1] http://code.google.com/p/setuid-sandbox/

Renata is working on this track at bug 90005.

[Thiago Marcos P. Santos]

Created attachment 156368 [details]
Work in progress of SeccompFilters v1

[Thiago Marcos P. Santos]

Created attachment 156370 [details]
Work in progress of SeccompFilters v2

Fixed a nit.

[WebKit Review Bot]

Attachment 156370 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/PlatformEfl.cmake', u'Sourc..." exit_code: 1
Source/WebKit2/Shared/seccomp/SeccompFilters.cpp:37:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebKit2/Shared/seccomp/SeccompFilters.cpp:170:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

Comment on attachment 156370 [details]
Work in progress of SeccompFilters v2

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

Looks pretty ok for a start

> Source/WebKit2/Shared/seccomp/SeccompFilters.cpp:50
> +    // No being able to initialize seccomp is considered a security
> +    // hole and the application should not go beyond this point. In fact,

security breach?

> Source/WebKit2/Shared/seccomp/SeccompFilters.cpp:63
> +    seccomp_release(m_seccompContext);

why not just m_context

> Source/WebKit2/Shared/seccomp/SeccompFilters.cpp:66
> +void SeccompFilters::setupFilters()

initializeFilters? or just initialize()

> Source/WebKit2/Shared/seccomp/SeccompFilters.cpp:68
> +    // Prioritize the are the most frequent calls.

I didn't get that English

> Source/WebKit2/Shared/seccomp/SeccompFilters.cpp:75
> +    // in the way it is implemented now. It is only blocking syscalls we are not

right now

> Source/WebKit2/Shared/seccomp/SeccompFilters.cpp:78
> +    // The list bellow summarizes the syscall whitelist needed by a EFL

below

why EFL here, in a non EFL file?

> Source/WebKit2/Shared/seccomp/SeccompFilters.cpp:114
> +    // Used by g_system_thread_set_name().
> +    seccomp_rule_add_or_crash(m_seccompContext, SCMP_ACT_ALLOW, SCMP_SYS(prctl), 0);
> +

if USE(GLIB) ?

> Source/WebKit2/Shared/seccomp/SeccompFilters.h:38
> +class SeccompFilters {
> +public:
> +    SeccompFilters();
> +    virtual ~SeccompFilters();

who will inherit from this?

[Thiago Marcos P. Santos]

Comment on attachment 156370 [details]
Work in progress of SeccompFilters v2

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

Thanks for having a look.

>> Source/WebKit2/Shared/seccomp/SeccompFilters.cpp:66
>> +void SeccompFilters::setupFilters()
> 
> initializeFilters? or just initialize()

+1 for inititalize and platformInitialize()

>> Source/WebKit2/Shared/seccomp/SeccompFilters.cpp:68
>> +    // Prioritize the are the most frequent calls.
> 
> I didn't get that English

"Prioritize the most frequent calls."

>> Source/WebKit2/Shared/seccomp/SeccompFilters.cpp:78
>> +    // The list bellow summarizes the syscall whitelist needed by a EFL
> 
> below
> 
> why EFL here, in a non EFL file?

This WARNING section is a temporary documentation of the work in progress. I'm just describing the test environment in case someone wants to give it a try. Probably if you try the same rule set on Qt (and I'm working on testing this right now), it will crash.

>> Source/WebKit2/Shared/seccomp/SeccompFilters.cpp:114
>> +
> 
> if USE(GLIB) ?

It should go to platformSetupFilters() after the triage.

>> Source/WebKit2/Shared/seccomp/SeccompFilters.h:38
>> +    virtual ~SeccompFilters();
> 
> who will inherit from this?

No one. I first wrote this as an interface and later changed to the platformInitialize() style.

[Thiago Marcos P. Santos]

Created attachment 156397 [details]
Work in progress of SeccompFilters v3

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 156397 [details]
Work in progress of SeccompFilters v3

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

Reviewing without looking at the seccomp-specific bits yet. I wonder if the EFL-independent part shouldn't be sent in a separate patch, together with build-webkit changes.

> Source/WebKit2/PlatformEfl.cmake:168
> +    LIST (APPEND WebKit2_LIBRARIES
> +        ${LIBSECCOMP_LIBRARIES}
> +    )

You also need to include its include path to the list of included directories.

> Source/WebKit2/Shared/seccomp/SeccompFilters.cpp:36
> +#define seccomp_rule_add_or_crash(context, action, syscall, count, ...) \

Isn't it better to uppercase this macro name to make it clear it is a macro?

> Source/cmake/FindLibSeccomp.cmake:31
> +INCLUDE(FindPkgConfig)
> +
> +PKG_CHECK_MODULES(LIBSECCOMP REQUIRED libseccomp>=1.0.0)

Please see https://bugs.webkit.org/show_bug.cgi?id=91864#c18 for my comments on why this should be expanded into proper CMake FIND_LIBRARY/FIND_PATH calls.

[Gyuyoung Kim]

Comment on attachment 156397 [details]
Work in progress of SeccompFilters v3

Attachment 156397 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13425732

[Thiago Marcos P. Santos]

Comment on attachment 156397 [details]
Work in progress of SeccompFilters v3

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

>> Source/WebKit2/Shared/seccomp/SeccompFilters.cpp:36
>> +#define seccomp_rule_add_or_crash(context, action, syscall, count, ...) \
> 
> Isn't it better to uppercase this macro name to make it clear it is a macro?

This macro is temporary. I'm trying to land a patch on seccomp that would allow me to wrap this in a method.

>> Source/cmake/FindLibSeccomp.cmake:31
>> +PKG_CHECK_MODULES(LIBSECCOMP REQUIRED libseccomp>=1.0.0)
> 
> Please see https://bugs.webkit.org/show_bug.cgi?id=91864#c18 for my comments on why this should be expanded into proper CMake FIND_LIBRARY/FIND_PATH calls.

Let's discuss this one when the patch is final. I have a feeling that this one is going to be long... :)

[Thiago Marcos P. Santos]

Created attachment 156604 [details]
Work in progress of SeccompFilters v4

Added to Qt buildsystem. It could load pages just fine.

[Thiago Marcos P. Santos]

Created attachment 156605 [details]
Work in progress of SeccompFilters v4 - correct

[Thiago Marcos P. Santos]

Created attachment 161845 [details]
Work in progress of SeccompFilters v4 - rebased

[Gyuyoung Kim]

Comment on attachment 161845 [details]
Work in progress of SeccompFilters v4 - rebased

Attachment 161845 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13717845

[Nandor Huszka]

Created attachment 168170 [details]
SeccompFilters on Qt (x86/64)

It works with these modifications on Qt on x86/64.

[Nandor Huszka]

I've measured the pageload time with Methanol using the syscall set that is enough to load pages correctly on Qt on an x86/64 machine. I ran the tests on sites that can be found on alexa top50. The goal of the measurement was that whether applying seccomp filters causes relevant slow-down on real world examples. I've observed ~1% slow-down using these sites.
This table contains the times with and without seccomp filters. Each page was loaded 30 times, the maximal variance was 3%.

test site      w/ libseccomp	w/o libseccomp   difference

blogger.com    31,3             31               0,9%
facebook.com   53,3             52,9             0,8%
google.com     16,2             16               1,3%
live.com       18,2             17,9             1,7%
microsoft.com  120,9            119,8            0,9%
qq.com         144,2            141,7            1,8%
taobao.com     768,2            767,8            0,0%
vkontakte.ru   28,3             27,8             1,7%
wordpress.com  112,2            111,6            0,5%
yandex.ru      129,8            129,2            0,4%
conduit.com    136,1            135,6            0,3%
ebay.com       84,4             83,3             1,4%
fc2.com        145,3            142,9            1,7%
linkedin.com   80,7             80,2             0,6%
mail.ru        105,5            104,3            1,2%
msn.com        225,5            224,9            0,3%
wikipedia.org  53,6             53,6             0,0%
yahoo.com      90,2             89,4             0,9%
youtube.com    151,4            150,5            0,6%

summary        2495,3           2480,5           0,6%

[Thiago Marcos P. Santos]

(In reply to comment #17)
> I've measured the pageload time with Methanol using the syscall set that is enough to load pages correctly on Qt on an x86/64 machine. I ran the tests on sites that can be found on alexa top50. The goal of the measurement was that whether applying seccomp filters causes relevant slow-down on real world examples. I've observed ~1% slow-down using these sites.
> This table contains the times with and without seccomp filters. Each page was loaded 30 times, the maximal variance was 3%.
> 
> test site      w/ libseccomp    w/o libseccomp   difference
> 
> blogger.com    31,3             31               0,9%
> facebook.com   53,3             52,9             0,8%
> google.com     16,2             16               1,3%
> live.com       18,2             17,9             1,7%
> microsoft.com  120,9            119,8            0,9%
> qq.com         144,2            141,7            1,8%
> taobao.com     768,2            767,8            0,0%
> vkontakte.ru   28,3             27,8             1,7%
> wordpress.com  112,2            111,6            0,5%
> yandex.ru      129,8            129,2            0,4%
> conduit.com    136,1            135,6            0,3%
> ebay.com       84,4             83,3             1,4%
> fc2.com        145,3            142,9            1,7%
> linkedin.com   80,7             80,2             0,6%
> mail.ru        105,5            104,3            1,2%
> msn.com        225,5            224,9            0,3%
> wikipedia.org  53,6             53,6             0,0%
> yahoo.com      90,2             89,4             0,9%
> youtube.com    151,4            150,5            0,6%
> 
> summary        2495,3           2480,5           0,6%

Thanks for doing this! It is really good to have these measurements.

[Nandor Huszka]

Created attachment 178024 [details]
Work in progress of SeccompFilters v5

I have updated the patch, now we need WebKit at hash 4a0b22f1bf4c3ea7553ab4655c4d16d0312cc683 and Qt5 r40 to build WK with it.
The system calls that are also needed for a non-minimal Qt build to run Methanol tests properly  can be found in SeccompFiltersQt.cpp.

After some attempts to limit open's parameter I see why we should do syscall hijacking in run-time and why we can't use libseccomp for it. One of the causes is threading. I will attach a file that contains the open calls by WebProcess after loading the rules. When I took limitation for the first parameter this way

  // addRule(SCMP_ACT_ALLOW, SCMP_SYS(open));
  const char* filePath = "/proc/self/maps"; // it is the first file that should be opened after loading the filters
  uint64t convertedFilePath = (uint64_t) filePath; // because SCMP_CMP should be used with argument type scmp_datum_t (uint64_t)
  seccomp_rule_add(m_context, SCMP_SYS_ALLOW, SCMP_SYS(open), 1, SCMP_CMP(0, SCMP_CMP_EQ, convertedFilePath, convertedFilePath)));

the WebProcess crashes when it tries to reach that file despite of it is on the whitelist. I experimented also with file descriptor argument, the same error, "Bad system call". The debugging's result is that WebProcess crashes in Source/WTF/wtf/StackBounds.cpp in method WTF::StackBounds::initialize at line

  pthread_getattr_np(thread, &sattr);

If I interpreted it correctly, the problem is an other thread doesn't see the rule we made in the main thread. Do you have any idea for it?

The other problem: we will only know the needed caches' paths after seccomp filters are activated, but we should take limitations for them statically, so we cannot add them to the whitelist simply. What do you think, will libseccomp support some kind of wildcard/regexp in respect of parameters? So what would be the right track: waiting for libseccomp's solution, or use an own for it and make a hybrid one by using libseccomp and raw kernel headers too?

(I successfully gained the limited open's parameter in run-time with the help of http://lxr.free-electrons.com/source/samples/seccomp/bpf-direct.c but it is strongly platform-dependent because of the registers. In any case with this we can examine the file's path in run-time and make some more rule.)

[Nandor Huszka]

Created attachment 178025 [details]
Opened files by QtWebProcess

It were made with strace, shows the open calls after loading seccomp filters. These files are needed to display Google.

[Nandor Huszka]

I have almost forgotten, I used ifdefs to allow syscalls for 32 and 64 bit systems, e.g.

#ifdef __x86_64__
    addRule(SCMP_ACT_ALLOW, SCMP_SYS(lseek));
#else
#ifdef __i386__
    addRule(SCMP_ACT_ALLOW, SCMP_SYS(_llseek));
#endif
#endif

What do you think about it?

[Thiago Marcos P. Santos]

(In reply to comment #21)
> I have almost forgotten, I used ifdefs to allow syscalls for 32 and 64 bit systems, e.g.
> 
> #ifdef __x86_64__
>     addRule(SCMP_ACT_ALLOW, SCMP_SYS(lseek));
> #else
> #ifdef __i386__
>     addRule(SCMP_ACT_ALLOW, SCMP_SYS(_llseek));
> #endif
> #endif
> 
> What do you think about it?

Hi Nandor,

I'm glad to tell you that I'm back on working on this issue. I'm right now implementing the syscall broker and some improvements to libseccomp itself. I'm redesigning the Seccomp class on WK to make it more flexible and with less ifdef's (if any). I'll try to uploading ASAP as soon as I have something working.

Cheers,

[Thiago Marcos P. Santos]

Created attachment 181544 [details]
Patch

[WebKit Review Bot]

Attachment 181544 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/As..." exit_code: 1
Source/WebKit2/Shared/linux/OpenSyscall.cpp:42:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Thiago Marcos P. Santos]

Created attachment 181545 [details]
Patch

[Thiago Marcos P. Santos]

Comment on attachment 181545 [details]
Patch

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

> Source/cmake/OptionsEfl.cmake:82
> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_SECCOMP_FILTERS ON)

In case we land this patch, this will be set to OFF. It is ON here to make it sure that the patch builds correctly on the bots.

[WebKit Review Bot]

Attachment 181545 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WTF/ChangeLog', u'Sou..." exit_code: 1
Source/WebKit2/Shared/linux/OpenSyscall.cpp:42:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Thiago Marcos P. Santos]

Comment on attachment 181545 [details]
Patch

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

>> Source/WebKit2/Shared/linux/OpenSyscall.cpp:42
>> +COMPILE_ASSERT(O_RDONLY == 0, O_RDONLY);
> 
> Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]

COMPILE_ASSERT(O_RDONLY + 1 == 1, O_RDONLY); passes, but IMO is ugly.

I need these asserts to make sure the value of these flags matches my assumptions. Looks like the spec doesn't mandate a value to this constants, different libc's might give my different values (although I think this is very unlikely).

[Chris Dumez]

Comment on attachment 181545 [details]
Patch

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

>>> Source/WebKit2/Shared/linux/OpenSyscall.cpp:42
>>> +COMPILE_ASSERT(O_RDONLY == 0, O_RDONLY);
>> 
>> Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
> 
> COMPILE_ASSERT(O_RDONLY + 1 == 1, O_RDONLY); passes, but IMO is ugly.
> 
> I need these asserts to make sure the value of these flags matches my assumptions. Looks like the spec doesn't mandate a value to this constants, different libc's might give my different values (although I think this is very unlikely).

I think the style script expects: COMPILE_ASSERT(!O_RDONLY, O_RDONLY);

But really, I don't know if it is better in this context.

[Anders Carlsson]

How does this relate to bug 90005? Until there is consensus on the proper way to move forward with sandboxing, I don't think this should be landed.

[Thiago Marcos P. Santos]

(In reply to comment #30)
> How does this relate to bug 90005? Until there is consensus on the proper way to move forward with sandboxing, I don't think this should be landed.

This is a different approach for sandboxing the render/plugin/network process on Linux but not an competitor to the setuid sandbox. Chromium is moving towards Seccomp Filters on Linux, but not exclusively [1]. It is actually stacking seccomp-bpf (an approach similar to this patch) on top of the setuid sandbox (something like bug 90005) [2].

I would still appreciate comments to this patch from you and other reviewers even though you marked it as r-.

A good start would be having a look at the unit tests [3]. The SetUp() method is a nice example of how a port can set some filesystem policies.

[1] http://blog.cr0.org/2012/09/introducing-chromes-next-generation.html
[2] http://www.insanitybit.com/2012/08/14/chrome-seccomp-bpf-sandbox/
[3] Tools/TestWebKitAPI/Tests/WebKit2/SeccompFilters.cpp

[Thiago Marcos P. Santos]

Created attachment 181667 [details]
Patch

Call for comments, not be landed or officially reviewed.

[EFL EWS Bot]

Comment on attachment 181667 [details]
Patch

Attachment 181667 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15755131

[EFL EWS Bot]

Comment on attachment 181667 [details]
Patch

Attachment 181667 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15741999

[Thiago Marcos P. Santos]

Created attachment 182790 [details]
Patch

Updated patch: add more syscalls and tests.

[Thiago Marcos P. Santos]

Created attachment 184566 [details]
Patch

[Thiago Marcos P. Santos]

Comment on attachment 184566 [details]
Patch

This patch is now ready for review. The main change from the latest version is the addition of file policies and support for Qt.

Whitepaper with a high level overview of this implementation:
https://docs.google.com/document/d/1Fe2ZSEazdqzxBWHDjGF8WuYwsI6-C95Ljmn-QiMyl94/edit

Incremental patch set on my development branch:
http://www.tmpsantos.com.br/cgi-bin/gitweb.cgi?p=webkit.git;a=shortlog;h=refs/heads/seccomp_filters

[Early Warning System Bot]

Comment on attachment 184566 [details]
Patch

Attachment 184566 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16110264

[Early Warning System Bot]

Comment on attachment 184566 [details]
Patch

Attachment 184566 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16072970

[Early Warning System Bot]

Comment on attachment 184566 [details]
Patch

Attachment 184566 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16112158

[WebKit Review Bot]

Comment on attachment 184566 [details]
Patch

Attachment 184566 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16119210

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

[Early Warning System Bot]

Comment on attachment 184566 [details]
Patch

Attachment 184566 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16112413

[Simon Hausmann]

Comment on attachment 184566 [details]
Patch

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

In general IMHO seccomp filters are the way to go. I have a few smaller comments below.

> Source/WTF/wtf/Assertions.cpp:352
> +#if !ENABLE(SECCOMP_FILTERS)

Then ChangeLog explains the existance of this #ifdef, but I think it would be also very useful to have as a comment in the code here.

> Source/WebKit2/WebProcess/efl/SeccompFiltersEfl.cpp:52
> +void SeccompFiltersEfl::platformInitialize()

A lot of the things in SeccompFiltersEfl is duplicated in SeccompFiltersQt. Why not share more settings?

It might also be worth documenting where certain policies originate from, i.e. /run/shm (it is /dev/shm on my machine?) from shared memory for example.

Alternatively would it make sense if the policies didn't come from .cpp files but from external files that are perhaps parsed at build time, to allow for easier sharing of settings between ports _and_ allow for better granularity with regards to the enabling/disabling of features, such as GStreamer?

> Source/WebKit2/WebProcess/efl/SeccompFiltersEfl.cpp:71
> +    ADD_POLICY(directoryPolicy, String::fromUTF8(efreet_data_home_get()) + "/WebKitEfl", ReadAndWrite);
> +    ADD_POLICY(directoryPolicy, String::fromUTF8(efreet_cache_home_get()) + "/WebKitEfl", ReadAndWrite);

Is this something that should come from the application layer perhaps? A setting on the API level?

> Source/WebKit2/WebProcess/efl/SeccompFiltersEfl.cpp:106
> +        // is not right. We need to check with these directories on gstreamer
> +        // can be configured.

Should this be #ifdef'ed via USE(GSTREAMER) then?

[Thiago Marcos P. Santos]

Created attachment 186868 [details]
Patch

[Thiago Marcos P. Santos]

Comment on attachment 186868 [details]
Patch

Major changes from the previous patch:

- Addressed issues pointed by Simon (thanks for reviewing).
- Directories paths for databases, cache, storage, etc are now coming from the UIProcess.
- Most of the directories and policies are now shared between EFL and Qt.
- Added more tests: threaded stress test that executes 15k syscalls (hooked into policies) spread in 5 threads.
- Ready to be enabled on the EFL and Qt bots if necessary. No apparent regressions on my tests.

[Kenneth Rohde Christiansen]

Comment on attachment 186868 [details]
Patch

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

> Source/WTF/ChangeLog:8
> +        Do no setup a handler for SIGSYS when Seccomp Filters are enabled.

do no?

> Source/WebCore/ChangeLog:6
> +        Make the DATA_DIR global since it is now need on WebCore and WebKit2.

needed* on -> for

> Source/WebKit2/ChangeLog:25
> +        The broker process should be initialized as early as possible on the
> +        sandboxed process main, because it only does a fork(), which is cheap

main process ?

> Source/WebKit2/ChangeLog:31
> +        Currently the broker supports only directory policies, but file policies
> +        could be implemented in the future to make narrower the filesystem
> +        exposed to the WebProcess.

only supports*

> Source/WebKit2/Shared/linux/SeccompFilters/OpenSyscall.cpp:157
> +    // things like the entry does not exist.

like if the ...

> Source/WebKit2/Shared/linux/SeccompFilters/OpenSyscall.cpp:180
> +bool OpenSyscall::decode(CoreIPC::ArgumentDecoder* decoder)
> +{
> +    if (!decoder->decode(m_path))
> +        return false;
> +    if (!decoder->decode(m_flags))
> +        return false;
> +
> +    return decoder->decode(m_mode);
> +}

you dont need to decode type()?

> Source/WebKit2/Shared/linux/SeccompFilters/OpenSyscall.h:53
> +    // Syscall implementation.
> +    virtual void setResult(const SyscallResult*);

Any reason to not call it SystemCall ?

> Source/WebKit2/Shared/linux/SeccompFilters/SeccompBroker.cpp:67
> +    int m_ncpus;

actual cpus or cpu threads?

> Source/WebKit2/Shared/linux/SeccompFilters/SeccompBroker.cpp:72
> +static ssize_t sendMessage(int socket, void* data, size_t size, int fd=-1)

space around =

> Source/WebKit2/Shared/linux/SeccompFilters/SeccompBroker.cpp:155
> +static void registerSysSignalHandler()

I would call it SystemSignalHandler...

> Source/WebKit2/Shared/linux/SeccompFilters/SeccompBroker.cpp:238
> +    // Malloc will sometimes check the number of CPUs present on the system

will sometime*

> Source/WebKit2/Shared/linux/SeccompFilters/SeccompBroker.cpp:247
> +bool SeccompBrokerClient::handleIfTryingToBlockSysSignal(ucontext_t* ucontext) const

handleAttempt...?

> Source/WebKit2/Shared/linux/SeccompFilters/SyscallPolicy.cpp:205
> +        // Mime type resolution.

MIME

[Simon Hausmann]

Comment on attachment 186868 [details]
Patch

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

> Source/WebKit2/WebProcess/qt/SeccompFiltersWebProcessQt.cpp:56
> +    const char* path = getenv("QTDIR");
> +    if (path) {
> +        QFileInfo qtDir(QString::fromUtf8(path));
> +        ADD_POLICY(&m_policy, Directory, qtDir.canonicalFilePath(), ReadAndWrite);
> +    }

QTDIR is dead :)

You could use QLibraryInfo::location(QLibraryInfo::PrefixPath) instead, but for distros that's going to be /usr, which sort of defeats the purpose :)

So I think parts of the paths of QLibraryInfo would make more sense to add, i.e. ArchData, LibrariesPath, LibrariesExecutablesPath, etc.

[Simon Hausmann]

Comment on attachment 186868 [details]
Patch

From a high-level perspective this looks good to me. I like how there's very little duplication between Efl and Qt. I think the default paths that remain can be fine-tuned in the future.

[Thiago Marcos P. Santos]

Created attachment 189320 [details]
Patch

[Thiago Marcos P. Santos]

(In reply to comment #49)
> Created an attachment (id=189320) [details]
> Patch

Andersca, Kling: The patch is ready for review. It is fully functional for EFL and Qt, has unit tests for all the features and documentation [1].

The implementation for GTK is ongoing on bug 110014 and being coordinated with the GTK developers. I will propose enabling it by default on the EFL bots on bug 110365 as soon as it lands.

Thanks a lot Kenneth and Simon for pre-reviewing.

[1] https://docs.google.com/document/d/1Fe2ZSEazdqzxBWHDjGF8WuYwsI6-C95Ljmn-QiMyl94/edit?usp=sharing

[Thiago Marcos P. Santos]

(In reply to comment #50)
> (In reply to comment #49)
> > Created an attachment (id=189320) [details] [details]
> > Patch
> 
> Andersca, Kling: The patch is ready for review. It is fully functional for EFL and Qt, has unit tests for all the features and documentation [1].
> 
> The implementation for GTK is ongoing on bug 110014 and being coordinated with the GTK developers. I will propose enabling it by default on the EFL bots on bug 110365 as soon as it lands.
> 
> Thanks a lot Kenneth and Simon for pre-reviewing.
> 
> [1] https://docs.google.com/document/d/1Fe2ZSEazdqzxBWHDjGF8WuYwsI6-C95Ljmn-QiMyl94/edit?usp=sharing

Any chances of getting this patch reviewed this week?

[Kenneth Rohde Christiansen]

andersca, could you have a look at this one?

It has backing from EFL, Qt and GTK+.

[Thiago Marcos P. Santos]

Created attachment 194031 [details]
Patch

Refactoring. Made it easier to read by removing things like macros.

[Thiago Marcos P. Santos]

Some WK2 owner could please review this patch?

[Thiago Marcos P. Santos]

May a WebKit2 Owner please review this patch?

[Maciej Stachowiak]

(In reply to comment #55)
> May a WebKit2 Owner please review this patch?

Thiago - I haven't followed this, is there a pre-review from someone who understands the Linux details? I'm not an expert on the low-level Linux APIs being used here.

[Thiago Marcos P. Santos]

(In reply to comment #56)
> (In reply to comment #55)
> > May a WebKit2 Owner please review this patch?
> 
> Thiago - I haven't followed this, is there a pre-review from someone who understands the Linux details? I'm not an expert on the low-level Linux APIs being used here.

Qt is represented here by Simon, which did a pre-review:
https://bugs.webkit.org/show_bug.cgi?id=89875#c48

EFL by Kenneth that also pre-reviewed:
https://bugs.webkit.org/show_bug.cgi?id=89875#c52

And GTK folks are following it in a different bug:
https://bugs.webkit.org/show_bug.cgi?id=110014#c5

[Carlos Garcia Campos]

(In reply to comment #57)
> (In reply to comment #56)
> > (In reply to comment #55)
> > > May a WebKit2 Owner please review this patch?
> > 
> > Thiago - I haven't followed this, is there a pre-review from someone who understands the Linux details? I'm not an expert on the low-level Linux APIs being used here.

I thought the idea of the owners thing was not that the owner reviewing the patch understands everything, but just checks the patch looks sane in general, relying on port reviewers for the real review. Otherwise it's impossible to land patches like this one. On the other hand I understand it's not easy for owners to know if the patch has been approved by a port reviewers, since we can't set r+ flag. Maybe we can try to find ways to make this easier. For example, allow port reviewers to set r+, but settings also cq-, then the owner can set cq+, or remove the cq flag or simply comment whether he approves the patch or not. We can also add a new bugzilla keyword, that is only added to bugs that has already been reviewed but are just waiting for an owner to approve it to make it easier to find patches blocked by webkit2 owners. 
 
> Qt is represented here by Simon, which did a pre-review:
> https://bugs.webkit.org/show_bug.cgi?id=89875#c48
> 
> EFL by Kenneth that also pre-reviewed:
> https://bugs.webkit.org/show_bug.cgi?id=89875#c52
> 
> And GTK folks are following it in a different bug:
> https://bugs.webkit.org/show_bug.cgi?id=110014#c5

[Maciej Stachowiak]

(In reply to comment #58)
> (In reply to comment #57)
> > (In reply to comment #56)
> > > (In reply to comment #55)
> > > > May a WebKit2 Owner please review this patch?
> > > 
> > > Thiago - I haven't followed this, is there a pre-review from someone who understands the Linux details? I'm not an expert on the low-level Linux APIs being used here.
> 
> I thought the idea of the owners thing was not that the owner reviewing the patch understands everything, but just checks the patch looks sane in general, relying on port reviewers for the real review. 

Yes, that's why I asked for pointers to the pre-reviews, which were graciously provided. I'll do my best to do a quick sanity check today. I'm actually really excited to see WebProcess sandboxing come to Linux.

> Otherwise it's impossible to land patches like this one. On the other hand I understand it's not easy for owners to know if the patch has been approved by a port reviewers, since we can't set r+ flag. Maybe we can try to find ways to make this easier. For example, allow port reviewers to set r+, but settings also cq-, then the owner can set cq+, or remove the cq flag or simply comment whether he approves the patch or not. We can also add a new bugzilla keyword, that is only added to bugs that has already been reviewed but are just waiting for an owner to approve it to make it easier to find patches blocked by webkit2 owners. 

 I think something like that (or a new bugzilla flag) for module owner sign-off makes a lot of sense.

[Maciej Stachowiak]

Comment on attachment 194031 [details]
Patch

This looks fine to me, given the pre-reviews from various ports and the lack of effect on cross-platform code.

[WebKit Commit Bot]

Comment on attachment 194031 [details]
Patch

Rejecting attachment 194031 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=webkit-cq-02', 'validate-changelog', '--non-interactive', 194031, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-commit-queue.appspot.com/results/17599093

[Thiago Marcos P. Santos]

Committed r147998: <http://trac.webkit.org/changeset/147998>