Bug 110014 - [GTK][WK2] Add seccomp filters support
Summary: [GTK][WK2] Add seccomp filters support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Gtk (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on: 89875 140064 140065 140066 140067 140068 140069 140070 141206
Blocks: 89874 140071 140072
  Show dependency treegraph
 
Reported: 2013-02-16 07:01 PST by Thiago Marcos P. Santos
Modified: 2015-07-19 09:34 PDT (History)
14 users (show)

See Also:


Attachments
wip (16.70 KB, patch)
2013-02-20 08:12 PST, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff
Patch (12.90 KB, patch)
2015-01-04 20:33 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
[GTK] Implement seccomp filter support (12.82 KB, patch)
2015-03-23 15:53 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
patch (12.93 KB, patch)
2015-04-03 17:31 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
[GTK] Implement seccomp filter support (12.58 KB, patch)
2015-07-13 07:47 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (12.37 KB, patch)
2015-07-15 18:19 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (12.19 KB, patch)
2015-07-19 07:59 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thiago Marcos P. Santos 2013-02-16 07:01:52 PST
TSIA.
Comment 1 Thiago Marcos P. Santos 2013-02-20 08:12:31 PST
Created attachment 189321 [details]
wip
Comment 2 Martin Robinson 2013-02-20 08:38:12 PST
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 3 Thiago Marcos P. Santos 2013-02-20 09:00:41 PST
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.
Comment 4 Martin Robinson 2013-02-20 10:29:10 PST
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.
Comment 5 Zan Dobersek 2013-03-07 06:36:13 PST
Let's get this done.
Comment 6 Zan Dobersek 2013-03-15 03:54:54 PDT
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.
Comment 7 Michael Catanzaro 2014-12-27 20:28:40 PST
(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
Comment 8 Michael Catanzaro 2015-01-04 20:33:49 PST
Created attachment 243959 [details]
Patch
Comment 9 Michael Catanzaro 2015-01-04 22:37:51 PST
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.
Comment 10 Michael Catanzaro 2015-03-23 15:53:16 PDT
Created attachment 249293 [details]
[GTK] Implement seccomp filter support
Comment 11 Michael Catanzaro 2015-04-03 17:31:41 PDT
Created attachment 250110 [details]
patch
Comment 12 Michael Catanzaro 2015-07-13 07:47:50 PDT
Created attachment 256695 [details]
[GTK] Implement seccomp filter support
Comment 13 Michael Catanzaro 2015-07-13 08:17:23 PDT
Comment on attachment 256695 [details]
[GTK] Implement seccomp filter support

(Accidentally marked for review before the dependent patches were ready.)
Comment 14 Michael Catanzaro 2015-07-15 18:19:23 PDT
Created attachment 256883 [details]
Patch
Comment 15 Michael Catanzaro 2015-07-15 18:21:46 PDT
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 16 Zan Dobersek 2015-07-19 00:42:50 PDT
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
Comment 17 Michael Catanzaro 2015-07-19 07:54:54 PDT
(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
Comment 18 Michael Catanzaro 2015-07-19 07:59:52 PDT
Created attachment 257057 [details]
Patch
Comment 19 WebKit Commit Bot 2015-07-19 09:34:15 PDT
Comment on attachment 257057 [details]
Patch

Clearing flags on attachment: 257057

Committed r187011: <http://trac.webkit.org/changeset/187011>
Comment 20 WebKit Commit Bot 2015-07-19 09:34:22 PDT
All reviewed patches have been landed.  Closing bug.