Bug 110014 – [GTK][WK2] Add seccomp filters support

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Thiago Marcos P. Santos]

TSIA.

[Thiago Marcos P. Santos]

Created attachment 189321 [details]
wip

[Martin Robinson]

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?

[Thiago Marcos P. Santos]

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.

[Martin Robinson]

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.

[Zan Dobersek]

Let's get this done.

[Zan Dobersek]

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.

[Michael Catanzaro]

(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

[Michael Catanzaro]

Created attachment 243959 [details]
Patch

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 249293 [details]
[GTK] Implement seccomp filter support

[Michael Catanzaro]

Created attachment 250110 [details]
patch

[Michael Catanzaro]

Created attachment 256695 [details]
[GTK] Implement seccomp filter support

[Michael Catanzaro]

Comment on attachment 256695 [details]
[GTK] Implement seccomp filter support

(Accidentally marked for review before the dependent patches were ready.)

[Michael Catanzaro]

Created attachment 256883 [details]
Patch

[Michael Catanzaro]

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.

[Zan Dobersek]

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

[Michael Catanzaro]

(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

[Michael Catanzaro]

Created attachment 257057 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 257057 [details]
Patch

Clearing flags on attachment: 257057

Committed r187011: <http://trac.webkit.org/changeset/187011>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.