| Summary: | [GStreamer] Seccomp Filters: Avoid gstreamer registry update in web process | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||
| Component: | WebCore Misc. | Assignee: | Michael Catanzaro <mcatanzaro> | ||||||
| Status: | RESOLVED WONTFIX | ||||||||
| Severity: | Normal | CC: | cgarcia, commit-queue, guijemont, mcatanzaro, pnormand, slomo, tmpsantos, yoon | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | PC | ||||||||
| OS: | Linux | ||||||||
| See Also: |
https://bugzilla.gnome.org/show_bug.cgi?id=742617 https://bugs.webkit.org/show_bug.cgi?id=135972 |
||||||||
| Bug Depends on: | 142978 | ||||||||
| Bug Blocks: | 140072 | ||||||||
| Attachments: |
|
||||||||
|
Description
Michael Catanzaro
2015-01-06 10:50:24 PST
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? (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. 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? (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. 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.) (In reply to comment #4) > the magic process canocalizes I mean "canonicalizes." To resolve symlinks. 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. 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. (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. 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 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. Oops, I forgot to post my patch for this :) Created attachment 256696 [details]
[Seccomp] Avoid gstreamer registry update in web process
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.
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.)
Created attachment 256702 [details]
[Seccomp] Avoid gstreamer registry update in web process
Hm, this no longer works; I'll need to figure out what went wrong.... |