Bug 140131 – [GStreamer] Seccomp Filters: Avoid gstreamer registry update in web process

• Component: WebCore Misc.
• Status: RESOLVED WONTFIX

[Michael Catanzaro]

We currently run plugin scanning in-process if seccomp filters are enabled, rather than use gst-plugin-scanner, so that it doesn't receive SIGSYS and dump core. See bug #140069. But this has disadvantages:

"Note that putting the plugin scanning in-process has two huge disadvantages though. You will dlopen() all (changed) plugins, which in turn loads all dependent libraries... and they will never be unloaded again for this process. And if any plugin crashes during initialization, it will just take your application process with it."

I see three options:

* Live with it. Presumably, plugins won't be changed very often, and only a modified plugin should be dlopened() by the plugin scanner (right?).
* Try to run the plugin scanner before initializing seccomp filters. We'd have to initialize gstreamer in WebKit2 instead of WebCore, even in web processes that never use gstreamer. Yuck.
* Rewrite the sandbox so that it's based on ptrace() rather than SIGSYS. (I haven't researched ptrace() fully. I think it would suffice, but I'm not sure what the disadvantages would be.) This would require rewriting the sandbox from scratch. It's probably not worth it for just gst-plugin-scanner, but perhaps another dependency will try to run a subprocess in the future.

I intend to at least explore option #3.

[Sebastian Dröge (slomo)]

What exactly is the reason for the plugin scanner failing with the current sandbox approach? Does it disallow forking processes in general, or is it something the plugin scanner is doing that breaks?

[Michael Catanzaro]

(In reply to comment #1)
> What exactly is the reason for the plugin scanner failing with the current
> sandbox approach? Does it disallow forking processes in general

Not in general, but the subprocess needs to play ball with our seccomp broker process, so arbitrary subprocesses are a no-go. The point of this is that a compromised web process should not be able to escape the sandbox by using fork() and exec().

> or is it something the plugin scanner is doing that breaks?

If the subprocess tries to use a trapped system call, e.g. open(), the kernel will send it SIGSYS. The subprocess is then responsible for handling SIGSYS (rather than dumping core) and communicating its request for a system call via some form of IPC to an unconfined process (our seccomp broker) to be audited and executed remotely. So we cannot spawn arbitrary subprocesses, only subprocesses that have been modified to chat with our sandbox broker process (which currently isn't set up to handle subprocesses anyway).

Another trick we might try would be to create a shared library that reimplements the glibc wrappers for the system calls I want to trap and set LD_PRELOAD before spawning the subprocess. I didn't think of that earlier, but this seems like a use case for LD_PRELOAD if there ever was one. The reimplemented functions would not use the system calls at all, they would serialize the arguments and send them to the seccomp broker process. (Now, setting up the IPC for that in the subprocess might be *interesting.*) If updated in tandem with the list of trapped calls, the subprocess would not even need to install a SIGSYS handler. I would need to update the seccomp broker to broker calls for arbitrary processes (rather than using a private socket to an individual web process), but I don't see any security reason to disallow that.

[Sebastian Dröge (slomo)]

LD_PRELOAD seems like a good idea, but more generically why is the IPC needed at all? Shouldn't this all be implemented at the kernel level, controlled via some kind of rule set instead of talking to some magic process?

[Michael Catanzaro]

(In reply to comment #3)
> LD_PRELOAD seems like a good idea, but more generically why is the IPC
> needed at all? Shouldn't this all be implemented at the kernel level,
> controlled via some kind of rule set

Good question. Actually it is implemented at the kernel level, but we just use it to say "send SIGSYS when you see open()" rather than "block open unless xyz." I think we can make the kernel allow access to particular paths and ditch the magic process, but the magic process canocalizes file paths, which I'm not sure we can do otherwise.

[Michael Catanzaro]

Another benefit of the broker: if the open() is impermissible, the broker will just return -1 and EACCES, the web process's signal handler will put EACCES into errno and -1 into the proper register, and then the signal handler will complete and the web process will resume from the syscall as if it was a normal permission denied error. If we have the kernel do the check, it would instead kill the web process. (I think that would be acceptable, though.)

[Michael Catanzaro]

(In reply to comment #4)
> the magic process canocalizes

I mean "canonicalizes." To resolve symlinks.

[Gwang Yoon Hwang]

In my opinion, we should not allow WebProcess to spawn any subprocesses.
These spawning request should be delegated to the UIProcess.

We should use UIProcess as a broker and process launcher.
Like Web Processes and Plugin / Network Processes, UIProcess should know every subprocesses and relationships between them. It will be much easier to implement broker login in UI Process.

And UIProcess cannot be sandboxed so this process should be unconfined already.
If we restrict the UIProcess, client program (like web browser) cannot even open a file itself.

Also, I think it is not acceptable just kill the web process when it has a problem.
Users cannot distinguish between crash from attack or crash from bug.

[Sebastian Dröge (slomo)]

In that case maybe the best is to run the plugin scanner from the UI process, and in the web process just disable the plugin scanner completely? GST_REGISTRY_UPDATE=no in the environment would do that.

[Michael Catanzaro]

(In reply to comment #8)
> In that case maybe the best is to run the plugin scanner from the UI
> process, and in the web process just disable the plugin scanner completely?
> GST_REGISTRY_UPDATE=no in the environment would do that.

Well that's clearly the best option in this case, so let's do that. I guess I failed to mention that the UI process is unconfined. :)

If another dependency tries to spawn a subprocess in the future, which hopefully will not happen, then we can handle it with the LD_PRELOAD method.

[Michael Catanzaro]

We decided it would be best to run the plugin scanner from a child of the UI process that will just call gst_init() and then immediately exit. However, we need to make sure missing plugin installation still works; see https://bugzilla.gnome.org/show_bug.cgi?id=742617

[Michael Catanzaro]

I spent a considerable amount of time trying to get the LD_PRELOAD trick to work once I realized I could use the GCC constructor extension to run arbitrary code in the subprocess after exec but before main. That would have allowed subprocesses other than gst-plugin-scanner to be used. Alas, it was not to be (the subprocess dies trying to call open before it gets to exec), so I'll try the approach in comment #10 next; since gst-plugin-scanner is the only subprocess we ever create, that will suffice.

[Michael Catanzaro]

Oops, I forgot to post my patch for this :)

[Michael Catanzaro]

Created attachment 256696 [details]
[Seccomp] Avoid gstreamer registry update in web process

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:62:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:65:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebKit2/UIProcess/Launcher/gtk/ProcessLauncherGtk.cpp:79:  Semicolon defining empty statement for this loop. Use { } instead.  [whitespace/semicolon] [5]
Total errors found: 3 in 3 files


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

[Michael Catanzaro]

Comment on attachment 256696 [details]
[Seccomp] Avoid gstreamer registry update in web process

(Didn't mean to request review: this is currently the end of a long series of patches.)

[Michael Catanzaro]

Created attachment 256702 [details]
[Seccomp] Avoid gstreamer registry update in web process

[Michael Catanzaro]

Hm, this no longer works; I'll need to figure out what went wrong....