Bug 89875

Summary: [WK2] Drop WebProcess capabilities on Linux using seccomp filters
Product: WebKit Reporter: Thiago Marcos P. Santos <tmpsantos>
Component: WebKit2Assignee: Thiago Marcos P. Santos <tmpsantos>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, abecsi, aestes, andersca, ap, bdakin, beidson, benjamin, cgarcia, cmarcelo, commit-queue, darin, dbates, dglazkov, d-r, enrica, felipe, guijemont, gustavo, gyuyoung.kim, hausmann, hnandor, hyatt, jarkko.j.sakkinen, joone, kenneth, kling, laszlo.gombos, levin+threading, menard, mitz, mjs, mrowe, ojan.autocc, rakuco, rhodovan.u-szeged, sam, simon.fraser, thorton, tmpsantos, vestbo, webkit-ews, webkit.review.bot, yael, zoltan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Linux   
URL: http://www.tmpsantos.com.br/cgi-bin/gitweb.cgi?p=webkit.git;a=shortlog;h=refs/heads/seccomp_filters
Bug Depends on: 91836    
Bug Blocks: 89874, 110014, 110365, 111543    
Attachments:
Description Flags
Work in progress of SeccompFilters v1
none
Work in progress of SeccompFilters v2
none
Work in progress of SeccompFilters v3
gyuyoung.kim: commit-queue-
Work in progress of SeccompFilters v4
none
Work in progress of SeccompFilters v4 - correct
none
Work in progress of SeccompFilters v4 - rebased
none
SeccompFilters on Qt (x86/64)
none
Work in progress of SeccompFilters v5
none
Opened files by QtWebProcess
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch mjs: review+, commit-queue: commit-queue-

Description Thiago Marcos P. Santos 2012-06-25 06:31:40 PDT
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.
Comment 1 Guillaume Emont 2012-07-20 07:34:51 PDT
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/
Comment 2 Thiago Marcos P. Santos 2012-07-20 08:06:40 PDT
(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.
Comment 3 Thiago Marcos P. Santos 2012-08-03 07:14:49 PDT
Created attachment 156368 [details]
Work in progress of SeccompFilters v1
Comment 4 Thiago Marcos P. Santos 2012-08-03 07:20:50 PDT
Created attachment 156370 [details]
Work in progress of SeccompFilters v2

Fixed a nit.
Comment 5 WebKit Review Bot 2012-08-03 07:24:18 PDT
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.
Comment 6 Kenneth Rohde Christiansen 2012-08-03 08:06:07 PDT
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?
Comment 7 Thiago Marcos P. Santos 2012-08-03 08:45:47 PDT
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.
Comment 8 Thiago Marcos P. Santos 2012-08-03 09:27:39 PDT
Created attachment 156397 [details]
Work in progress of SeccompFilters v3
Comment 9 Raphael Kubo da Costa (:rakuco) 2012-08-03 10:45:08 PDT
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.
Comment 10 Gyuyoung Kim 2012-08-03 11:42:23 PDT
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
Comment 11 Thiago Marcos P. Santos 2012-08-03 14:17:40 PDT
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... :)
Comment 12 Thiago Marcos P. Santos 2012-08-05 23:41:16 PDT
Created attachment 156604 [details]
Work in progress of SeccompFilters v4

Added to Qt buildsystem. It could load pages just fine.
Comment 13 Thiago Marcos P. Santos 2012-08-05 23:43:52 PDT
Created attachment 156605 [details]
Work in progress of SeccompFilters v4 - correct
Comment 14 Thiago Marcos P. Santos 2012-09-02 11:22:34 PDT
Created attachment 161845 [details]
Work in progress of SeccompFilters v4 - rebased
Comment 15 Gyuyoung Kim 2012-09-02 11:50:13 PDT
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
Comment 16 Nandor Huszka 2012-10-11 01:48:52 PDT
Created attachment 168170 [details]
SeccompFilters on Qt (x86/64)

It works with these modifications on Qt on x86/64.
Comment 17 Nandor Huszka 2012-11-07 09:36:57 PST
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%
Comment 18 Thiago Marcos P. Santos 2012-11-07 09:58:22 PST
(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.
Comment 19 Nandor Huszka 2012-12-06 10:14:06 PST
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.)
Comment 20 Nandor Huszka 2012-12-06 10:18:26 PST
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.
Comment 21 Nandor Huszka 2012-12-06 10:30:41 PST
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?
Comment 22 Thiago Marcos P. Santos 2012-12-10 13:44:38 PST
(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,
Comment 23 Thiago Marcos P. Santos 2013-01-07 12:30:21 PST
Created attachment 181544 [details]
Patch
Comment 24 WebKit Review Bot 2013-01-07 12:36:30 PST
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.
Comment 25 Thiago Marcos P. Santos 2013-01-07 12:38:06 PST
Created attachment 181545 [details]
Patch
Comment 26 Thiago Marcos P. Santos 2013-01-07 12:39:27 PST
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.
Comment 27 WebKit Review Bot 2013-01-07 12:41:46 PST
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.
Comment 28 Thiago Marcos P. Santos 2013-01-07 12:44:44 PST
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).
Comment 29 Chris Dumez 2013-01-07 12:50:37 PST
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.
Comment 30 Anders Carlsson 2013-01-07 12:55:17 PST
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.
Comment 31 Thiago Marcos P. Santos 2013-01-07 23:36:13 PST
(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
Comment 32 Thiago Marcos P. Santos 2013-01-08 00:53:58 PST
Created attachment 181667 [details]
Patch

Call for comments, not be landed or officially reviewed.
Comment 33 EFL EWS Bot 2013-01-08 03:17:22 PST
Comment on attachment 181667 [details]
Patch

Attachment 181667 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15755131
Comment 34 EFL EWS Bot 2013-01-08 03:55:46 PST
Comment on attachment 181667 [details]
Patch

Attachment 181667 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15741999
Comment 35 Thiago Marcos P. Santos 2013-01-15 09:29:21 PST
Created attachment 182790 [details]
Patch

Updated patch: add more syscalls and tests.
Comment 36 Thiago Marcos P. Santos 2013-01-24 13:24:38 PST
Created attachment 184566 [details]
Patch
Comment 37 Thiago Marcos P. Santos 2013-01-24 13:32:41 PST
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
Comment 38 Early Warning System Bot 2013-01-24 14:31:00 PST
Comment on attachment 184566 [details]
Patch

Attachment 184566 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16110264
Comment 39 Early Warning System Bot 2013-01-24 15:35:42 PST
Comment on attachment 184566 [details]
Patch

Attachment 184566 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16072970
Comment 40 Early Warning System Bot 2013-01-24 16:37:49 PST
Comment on attachment 184566 [details]
Patch

Attachment 184566 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16112158
Comment 41 WebKit Review Bot 2013-01-25 01:45:53 PST
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
Comment 42 Early Warning System Bot 2013-01-25 02:42:26 PST
Comment on attachment 184566 [details]
Patch

Attachment 184566 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16112413
Comment 43 Simon Hausmann 2013-01-25 04:54:25 PST
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?
Comment 44 Thiago Marcos P. Santos 2013-02-06 09:09:44 PST
Created attachment 186868 [details]
Patch
Comment 45 Thiago Marcos P. Santos 2013-02-07 07:17:12 PST
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.
Comment 46 Kenneth Rohde Christiansen 2013-02-13 02:51:30 PST
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
Comment 47 Simon Hausmann 2013-02-14 04:44:14 PST
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.
Comment 48 Simon Hausmann 2013-02-14 04:46:03 PST
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.
Comment 49 Thiago Marcos P. Santos 2013-02-20 08:06:02 PST
Created attachment 189320 [details]
Patch
Comment 50 Thiago Marcos P. Santos 2013-02-20 12:46:48 PST
(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
Comment 51 Thiago Marcos P. Santos 2013-03-04 07:47:22 PST
(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?
Comment 52 Kenneth Rohde Christiansen 2013-03-06 11:30:45 PST
andersca, could you have a look at this one?

It has backing from EFL, Qt and GTK+.
Comment 53 Thiago Marcos P. Santos 2013-03-20 06:07:08 PDT
Created attachment 194031 [details]
Patch

Refactoring. Made it easier to read by removing things like macros.
Comment 54 Thiago Marcos P. Santos 2013-03-28 01:34:28 PDT
Some WK2 owner could please review this patch?
Comment 55 Thiago Marcos P. Santos 2013-04-04 05:30:21 PDT
May a WebKit2 Owner please review this patch?
Comment 56 Maciej Stachowiak 2013-04-04 23:33:52 PDT
(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.
Comment 57 Thiago Marcos P. Santos 2013-04-05 01:06:37 PDT
(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
Comment 58 Carlos Garcia Campos 2013-04-05 08:54:13 PDT
(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
Comment 59 Maciej Stachowiak 2013-04-05 11:52:41 PDT
(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.
Comment 60 Maciej Stachowiak 2013-04-08 15:48:05 PDT
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.
Comment 61 WebKit Commit Bot 2013-04-09 01:47:18 PDT
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
Comment 62 Thiago Marcos P. Santos 2013-04-09 02:06:16 PDT
Committed r147998: <http://trac.webkit.org/changeset/147998>