[Webkit-unassigned] [Bug 188568] [GTK][WPE] Implement subprocess sandboxing

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Aug 14 13:44:20 PDT 2018


Michael Catanzaro <mcatanzaro at igalia.com> changed:

           What    |Removed                     |Added
                 CC|                            |mcatanzaro at igalia.com
 Attachment #347098|1                           |0
        is obsolete|                            |

--- Comment #4 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 347098
  --> https://bugs.webkit.org/attachment.cgi?id=347098

View in context: https://bugs.webkit.org/attachment.cgi?id=347098&action=review

Unsetting r? for now, since this won't be ready for review until we have branched 2.20, installed bwrap git master on the EWS, and deleted our jhbuild scripts.

Please add a very simple test in Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitSettings.cpp, at the bottom of testWebKitSettings(), to verify that the setting getter/setters work.

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

Remove this line.

> Source/WebKit/ChangeLog:17
> +        Currently the sandbox is fairly open as to not introduce many
> +        regressions in functionality with the hope that this can be tightened
> +        up over time as WebKit improves and as external projects like
> +        DBus, Pipewire, etc become more sandboxed and available.

As we discussed, you'll need to implement D-Bus filtering in a follow-up patch for this to be plausibly secure.

> Source/WebKit/Shared/SandboxExtension.h:145

I don't like that we're building without ENABLE(SANDBOX_EXTENSIONS) but nevertheless implementing SandboxExtensionGLib. Please look into this a bit more. You might want to turn on ENABLE(SANDBOX_EXTENSIONS) and just make most of the functionality dependent on PLATFORM(COCOA). Or you might want to just not use SandboxExtension.

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1174
> + * Returns: %TRUE If sandboxing is enabled, or %FALSE otherwise.

lowercase "if"

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:62
> +// From flatpak-run.c (LGPLv2.1+)

I would try harder to indicate how much code is copied from flatpak-run.c by adding an ending comment as well:

// Code adapted from flatpak-run.c (LGPLv2.1+)

// ...
// ...
// ...

// End code from flatpak-run.c

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:65
> +    const char* path, *path_end;

We usually only declare one variable per line, to avoid this problem of having the  * nest against the variable name rather than the type.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:102
> +    if (bindFlags == BindFlags::Device)
> +        type = "--dev-bind-try";
> +    else if (bindFlags == BindFlags::ReadOnly)
> +        type = "--ro-bind-try";
> +    else
> +        type = "--bind-try";

Nice that you got these new args accepted into bubblewrap!

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:157
> +        const char* configDir = g_get_user_config_dir();
> +        GUniquePtr<char> dconfConfigDir(g_build_filename(configDir, "dconf", nullptr));
> +        bindIfExists(args, dconfConfigDir.get(), BindFlags::ReadWrite);

Hmmm, so the process needs direct write access to dconf's gvdbs? Seems like that *really* limits the benefit from any D-Bus-level filtering we might implement for dconf, right? A compromised web process would not need to use the D-Bus API at all, it could just write directly to the gvdb.

Fortunately, we control dconf, so we can make changes as needed....

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:174
> +    // TODO: The server can be defined in config files we'd have to parse.

We have a weird rule that you can't write "TODO" in comments, only "FIXME." Never understood why. Just roll with it.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:175
> +    //       They can also be set as X11 props, but that is getting a bit ridiculous.

No leading space here.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:180
> +        // else it uses tcp


> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:224
> +static void bindGtkData(Vector<CString>& args)

Guard this one with #if PLATFORM(GTK) so that it doesn't get used for WPE.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:253
> +static void bindGstreamerData(Vector<CString>& args)

bindGStreamerData, capital S

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:269
> +    bindIfExists(args, "/usr/libexec/gst-install-plugins-helper");
> +    bindIfExists(args, "/usr/local/libexec/gst-install-plugins-helper");

This only works on Fedora... is it really like this in flatpak? :( Check where it is installed on Debian, it will be under /usr/lib/ somewhere and likely all other distros will have it there. (Fortunately, it won't be multiarch, since it's an executable, so that helps.)

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:272
> +static void bindOpenGL(Vector<CString>& args)

We're making an unwarranted assumption here that the graphics driver is secure, but that's all we can do for now, so this is fine. In the future, we might have a separate GPU process to tighten things down better.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:345

Please replace this comment with one of your own, written for WebKit. Use // comments.

Once this lands, let's remember to update the comment in flatpak too, to point to us, so hopefully people will notify WebKit if changing the filters in flatpak.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:380
> +        /* Block dmesg */

I'm surprised the style checker doesn't complain about this anymore, but use // comments everywhere.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:411
> +        /* Don't allow faking input to the controlling tty (CVE-2017-5226) */
> +        // FIXME: { SCMP_SYS(ioctl), &SCMP_A1(SCMP_CMP_EQ, (int)TIOCSTI) },

Does WebKit really need this? Why can't it be blocked?

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:464
> +    if (processType == ProcessLauncher::ProcessType::Plugin64
> +        || processType == ProcessLauncher::ProcessType::Plugin32)

Hm, I'm surprised style checker didn't complain about this... the || goes at the end of the previous line in WebKit, not the beginning of the new line.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:519
> +        // NOTE: DConf usage is probably entirely libsoups fault
> +        // This requirement should be improved in the future

Well glib-networking, not libsoup. You can't assume any particular proxy implementation, either. E.g. when running in GNOME, glib-networking reads proxy settings out of GSettings. But outside GNOME, it uses libproxy instead, which does all sorts of fun things like executing JavaScript in either  mozjs or using WebKit itself depending on which libproxy plugin is in  use. Good news: there are plans afoot to add a D-Bus boundary inside libproxy to isolate plugins from the calling process, which should make this even more fun to sandbox.

Anyway, don't worry about it for now, just /s/libsoup/glib-networking

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:566
> +        // TODO: We leak this fd?

Um, well yes, we need to figure this out, even leaking just a single fd per process launch can be a really big deal. I guess the fd needs to stay open until the exec to be inherited by the child, but it really needs to be closed in the parent, right? This is important to get right.

Good that you added a warning comment; I would have missed it otherwise.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:582
> +#endif
> +#if OS(LINUX)

There should be a blank line between #endif and #if here.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:599
> +        g_warning("Not using Flatpak sandbox because in a build instance");

Well that's not worth warning about, because it's totally expected... it'll even be the default developer workflow soon! I think you can just return, and have the comment to explain the behavior.

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:614
> +static Vector<CString> getFlatpakArgs(ProcessLauncher::ProcessType processType)
> +{
> +    Vector<CString> sandboxArgs = {
> +        "/usr/bin/flatpak-spawn",
> +    };
> +
> +    if (processType == ProcessLauncher::ProcessType::Storage)
> +        sandboxArgs.append("--no-network");
> +
> +    return sandboxArgs;
> +}

This is pretty underwhelming, so we need a comment here on strategy for future improvement.

Also: a higher-level comment on the difference between the bwrap sandbox and the flatpak-spawn sandbox, to explain the purpose of each one.

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:113
> +    API::WebsiteDataStore* store1 = processPool().websiteDataStore();
> +    WebsiteDataStore& store = store1->websiteDataStore();

Code style:

WebsiteDataStore& store = processPool().websiteDataStore->websiteDataStore();

Much more readable. We don't need as many local variables in C++ as we do in C, thanks to the member function syntax.

> Source/WebKit/UIProcess/Storage/StorageProcessProxy.cpp:95
> +    API::WebsiteDataStore* store1 = m_processPool.websiteDataStore();
> +    WebsiteDataStore& store = store1->websiteDataStore();


> Source/cmake/WebKitFeatures.cmake:94
> +    WEBKIT_OPTION_DEFINE(ENABLE_BUBBLEWRAP_SANDBOX "Toggle bubblewrap sandboxing support" PRIVATE OFF)
>      WEBKIT_OPTION_DEFINE(ENABLE_CHANNEL_MESSAGING "Toggle MessageChannel and MessagePort support" PRIVATE ON)

It should be alphabetized (move it one line higher)

You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180814/dfa289c5/attachment-0001.html>

More information about the webkit-unassigned mailing list