|
Description
Patrick Griffis
2018-08-14 11:45:12 PDT
Created attachment 347098 [details]
Patch
Attachment 347098 [details] did not pass style-queue:
WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitWebContext.h"
ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
Total errors found: 1 in 22 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 347101 [details]
Patch
Comment on attachment 347098 [details] Patch 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 > +#if !ENABLE(SANDBOX_EXTENSIONS) && !PLATFORM(GTK) && !PLATFORM(WPE) 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 Wow.... > 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 > + /**** BEGIN NOTE ON CODE SHARING 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(); Ditto > Source/cmake/WebKitFeatures.cmake:94 > WEBKIT_OPTION_DEFINE(ENABLE_CACHE_PARTITIONING "Toggle cache partitioning support" PRIVATE OFF) > + 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) (In reply to Michael Catanzaro from comment #4) > Comment on attachment 347098 [details] > Patch > > 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. I mean, it won't be ready for committing... you can use 'webkit-patch upload --no-review' until we've reached the finish line and you think it is ready for trunk. Created attachment 347158 [details]
Patch
(In reply to Michael Catanzaro from comment #4) > 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. This API is for web context. I don't see similar tests there. > > Source/WebKit/Shared/SandboxExtension.h:145 > > +#if !ENABLE(SANDBOX_EXTENSIONS) && !PLATFORM(GTK) && !PLATFORM(WPE) > > 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. `ENABLE(SANDBOX_EXTENSIONS)` doesn't make any sense as the rest of their API is a complete mismatch to ours *except* this group of standalone functions that do exactly what we want, which is at resolve time of paths and before launching processes ensure the directories exist. It is a perfect fit for our needs. Not using it means copy pasting these calls in the same places with an added #if. > > 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.) We already grant read access to /usr/lib, so that is fine. We could do blanket read access to /usr/libexec also. > > 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. ERROR: Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:461: Boolean expressions thatspan multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] --- Everything else was resolved. Attachment 347158 [details] did not pass style-queue:
WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitWebContext.h"
ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
Total errors found: 1 in 22 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to Patrick Griffis from comment #7) > (In reply to Michael Catanzaro from comment #4) > > 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. > > This API is for web context. I don't see similar tests there. Ah yes, TestWebKitWebContext would be the right place: you can add it there instead. > `ENABLE(SANDBOX_EXTENSIONS)` doesn't make any sense as the rest of their API > is > a complete mismatch to ours *except* this group of standalone functions that > do exactly what we want, which is at resolve time of paths and before > launching > processes ensure the directories exist. It is a perfect fit for our needs. > Not using it means copy pasting these calls in the same places with an added > #if. I suggest copypasting these calls in the same places with an added #if: that would probably be more clear and hopefully less-confusing. Try it and we'll see what the result looks like. I know it would be more code, but I think it's important to avoid using SandboxExtensions here. > > > 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.) > > We already grant read access to /usr/lib, so that is fine. We could do > blanket read access to /usr/libexec also. Hm good point. This seems fine, then. I wouldn't give access to all of /usr/libexec for the same reason we don't give access to /usr/bin. > > > 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. > > > ERROR: > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:461: Boolean > expressions thatspan multiple lines should have their operators on the left > side of the line instead of the right side. [whitespace/operators] [4] Oh wow, shows what I know.... Comment on attachment 347158 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347158&action=review > Source/WebCore/ChangeLog:20 > +2018-08-15 Patrick Griffis <pgriffis@igalia.com> > + > + [GTK][WPE] Implement subprocess sandboxing > + https://bugs.webkit.org/show_bug.cgi?id=188568 > + > + Reviewed by NOBODY (OOPS!). > + > + No new tests (OOPS!). > + > + * PlatformGTK.cmake: > + > +2018-08-14 Patrick Griffis <pgriffis@igalia.com> > + > + [GTK][WPE] Implement subprocess sandboxing > + https://bugs.webkit.org/show_bug.cgi?id=188568 > + > + Reviewed by NOBODY (OOPS!). > + > + * PlatformGTK.cmake: > + Watch your ChangeLog! You've got duplicate entries in the other ChangeLog, too. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:348 > + // NOTE: This is shared code (LGPLv2.1+) I would be more clear about where you copied it from.... Comment on attachment 347158 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347158&action=review This looks pretty good, thanks! I haven't had the time to apply the patch locally and test, though. Other than the comment about the WPE API header, this looks good to me and would like to see it landed if an actual reviewer rubber-stamps the patch (I am not one yet ^_^). > Source/WebKit/UIProcess/API/gtk/WebKitWebContext.h:256 > + You forgot to also copy these two prototypes in “…/API/wpe/WebKitWebContext.h” (We have the headers duplicated for both ports; one of the reasons was to avoid the need to clutter them with “#ifdef”s inside them or having to generate code, and I think there might be some “gtk-doc” related reason as well… Anyway, the public APIs are not modified often, so it's no big deal for things to be this way :P) > Source/WebKit/UIProcess/API/gtk/docs/webkit2gtk-4.0-sections.txt:55 > +webkit_web_context_set_sandbox_enabled …but we do not yet have the corresponding “…API/wpe/docs/wpewebkit-0.1-sections.txt” because we still have pending to make possible to run gtk-doc for the WPE port so there is no need to worry about this for WPE O:-) > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:175 > +{ Some systems may not use PulseAudio at all, and in that case the GStreamer autoplugging machinery will choose ALSA (or even OSS!) if possible. I think almost nobody is using OSS on GNU/Linux anymore in 2018, so I think at least we should bind “/dev/snd” inside the sandbox. That would also cover for cases like people configuring audio capture to use some particular ALSA device instead of PulseAudio. One example of the latter that I have witnessed is having a headset for confcalls that has an USB-audio interface but Pulse would not pick it up somehow. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:327 > + "--dev-bind-try", "/dev/nvidia", "/dev/nvidia", While this list is a good initial approach that covers (all?) the Open Source drivers (which are the sane ones, and have their device nodes under “/dev/dri”), and a couple of extra cases (nice to see the entries here already for Nvidia users), there are a few many proprietary drivers which are not covered. For example we have WPE running on Adreno GPUs that make “/dev/kgsl-3d0” and “/dev/ion”. But the story does not end there: WPE can (and sometimes will) run without a windowing system, which on some platforms means there is some WPE backend implementation which may e.g. use old style framebuffer devices (“/dev/fbN”) or even some custom device nodes. I guess there's no better good way of doing this without adding entries to the whitelist later on as we keep finding more cases of “bad” drivers which have their device node paths outside of “/dev/dri” :-( Anyway, I think this can go in like this for now because with the sandbox being an opt-in it's not like we are going to break anything. I just want to raise some awareness of this issue because we may want to think if there's something we can do to improve on this in the future. (In reply to Adrian Perez from comment #11) > You forgot to also copy these two prototypes in > “…/API/wpe/WebKitWebContext.h” > > (We have the headers duplicated for both ports; one of the reasons was to > avoid > the need to clutter them with “#ifdef”s inside them or having to generate > code, > and I think there might be some “gtk-doc” related reason as well… Anyway, the > public APIs are not modified often, so it's no big deal for things to be this > way :P) OptionsWPE.cmake will also need updated, then. (In reply to Adrian Perez from comment #11) > Some systems may not use PulseAudio at all, and in that case the GStreamer > autoplugging machinery will choose ALSA (or even OSS!) if possible. I think > almost nobody is using OSS on GNU/Linux anymore in 2018, so I think at least > we should bind “/dev/snd” inside the sandbox. That would also cover for cases > like people configuring audio capture to use some particular ALSA device > instead of PulseAudio. One example of the latter that I have witnessed is > having a headset for confcalls that has an USB-audio interface but Pulse > would not pick it up somehow. Michael said it was reasonable to focus on Pulseaudio (at least in context of GTK). The reality is that Pulseaudio is no safer than raw ALSA atm (Pipewire will be our savior perhaps) so I guess its fine to add raw dev access. > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:327 > > + "--dev-bind-try", "/dev/nvidia", "/dev/nvidia", > > While this list is a good initial approach that covers (all?) the Open > Source drivers (which are the sane ones, and have their device nodes > under “/dev/dri”), and a couple of extra cases (nice to see the entries > here already for Nvidia users), there are a few many proprietary drivers > which are not covered. For example we have WPE running on Adreno GPUs > that make “/dev/kgsl-3d0” and “/dev/ion”. > > But the story does not end there: WPE can (and sometimes will) run without > a windowing system, which on some platforms means there is some WPE backend > implementation which may e.g. use old style framebuffer devices (“/dev/fbN”) > or even some custom device nodes. > > I guess there's no better good way of doing this without adding entries > to the whitelist later on as we keep finding more cases of “bad” drivers > which have their device node paths outside of “/dev/dri” :-( Indeed, I'll add the ones you mentioned. (In reply to Patrick Griffis from comment #13) > Michael said it was reasonable to focus on Pulseaudio (at least in context > of GTK). > > The reality is that Pulseaudio is no safer than raw ALSA atm (Pipewire will > be our > savior perhaps) so I guess its fine to add raw dev access. I don't know about /dev/snd. Talking with Patrick, it sounds like the PulseAudio support is a huge hole in the sandbox that will definitely need to be removed once Pipewire is ready to obsolete it. I don't know if there's some way to detect at runtime whether we need to give access to Pulse or not; I guess that logic is buried deep in GStreamer? Created attachment 347367 [details]
Patch
Created attachment 347637 [details]
Patch
Attachment 347637 [details] did not pass style-queue:
WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitWebContext.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/wpe/WebKitWebContext.h"
ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
Total errors found: 1 in 23 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Best uncomment conditions.add('asan') in the jhbuildrc I gave you, because I sree a heap use after free when starting Epiphany. The problem is in resolveAndCreateReadWriteDirectoryForSandboxExtension, where cpath points to the internal buffer of path.utf8(), which is a temporary that has already been destroyed when you check if (!cpath[0]) on the next line. The simplest solution would be to initialize cpath with path.utf8() rather than path.utf8().data(), so that it's a CString rather than a const char*.
But you don't really need it at all, since you can just check path.isEmpty() instead:
String resolveAndCreateReadWriteDirectoryForSandboxExtension(const String& path)
{
if (path.isEmpty())
return { };
if (g_mkdir_with_parents(path.utf8().data(), 0700) == -1) {
g_warning("Could not create directory \"%s\": %s", path.utf8().data(), g_strerror(errno));
return { };
}
return path;
}
BTW, I still think you should duplicate code rather than implement Sandbox Extension.
Comment on attachment 347637 [details] Patch Attachment 347637 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8936884 New failing tests: http/wpt/workers/name-property-enhanced.html Created attachment 347745 [details]
Archive of layout-test-results from ews200 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
(In reply to Michael Catanzaro from comment #18) > Best uncomment conditions.add('asan') in the jhbuildrc I gave you It also needs an LD_PRELOAD when run under jhbuild, which the example config I gave you earlier today was missing: os.environ['LD_PRELOAD'] = '/usr/lib64/libasan.so.5' conditions.add('asan') Also just ignore the Build Bot; it usually only posts false-positives. (In reply to Michael Catanzaro from comment #21) > (In reply to Michael Catanzaro from comment #18) > > Best uncomment conditions.add('asan') in the jhbuildrc I gave you > > It also needs an LD_PRELOAD when run under jhbuild, which the example config > I gave you earlier today was missing: > > os.environ['LD_PRELOAD'] = '/usr/lib64/libasan.so.5' > conditions.add('asan') Ah, that's a bad idea because now it gets injected into CMake tests... quite annoying, there's no good place to put the LD_PRELOAD. :/ Created attachment 348657 [details]
[GTK][WPE] Implement subprocess sandboxing
Created attachment 348658 [details]
[GTK][WPE] Implement subprocess sandboxing
Created attachment 348659 [details]
[GTK][WPE] Implement subprocess sandboxing
Comment on attachment 348659 [details] [GTK][WPE] Implement subprocess sandboxing View in context: https://bugs.webkit.org/attachment.cgi?id=348659&action=review Excellent! We are quite close to being able to land this. I'll probably give r+ after you address the next round of feedback. I finally switched from Ephy Tech Preview to a local JHBuild build with this patch. So far, so good. The only problem I've noticed so far is that YouTube is broken like you mentioned, but like you said, that's not caused by this patch, so not your problem. I tested this for about two days so far and was almost concerned that I haven't noticed anything broken yet. In particular, I was confused that downloads seem to be working fine. Eventually I realized it's because I need to change Epiphany to enable the sandbox. :P So I should really turn it on there and keep testing. Make a note: once this lands, all the new environment variables need to be documented on https://trac.webkit.org/wiki/EnvironmentVariables. I'll have to handle that since only committers are able to edit the wiki. Also important: we have an owners policy for Source/WebKit: changes to cross-platform files outside platform #ifdefs need to be approved by one of the owners from Apple. So we'll need to remember, as the very final step before landing, to get approval for your additions to WebsiteDataStore.[cpp,h]. (All the rest of the changes are platform-specific and can be approved by me.) > Source/WebCore/ChangeLog:8 > + [GTK][WPE] Implement subprocess sandboxing Remove this line (title belongs only at the top) > Source/WebCore/ChangeLog:19 > + DBus, Pipewire, etc become more sandboxed and available. Well you have a restrictive D-Bus proxy now, right? So I would update this commit message to mention the actual weaknesses that are known to exist: * PulseAudio is allowed and that's very bad * GPU access is allowed * Web process still has network access Anything else that should be mentioned? Normally I wouldn't be so strict about commit messages, but this is an important commit and I suspect people will be looking back at it for a long time to come. Actually, I would move these details to the Source/WebKit ChangeLog, and only mention your WebCore changes here. > Source/WebKit/ChangeLog:8 > + [GTK][WPE] Implement subprocess sandboxing Ditto regarding the duplicated title > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1155 > + * Set whether WebKit subprocesses will be sandboxed limiting access to the system. comma after "sandboxed" > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1157 > + * This cannot be changed after processes are launched and is only implemented on Linux. Let's copy the documentation from webkit_web_context_set_process_model(): * This method **must be called before any web process has been created**, * as early as possible in your application. Calling it later will make * your application crash. And make sure the crash actually happens (with g_error() to leave a message, or CRASH() if you're feeling less-generous), because calling this function too late indicates a serious security mistake. > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1159 > + * Since: 2.22 2.24 > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1176 > + * Since: 2.22 2.24 > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:65 > + DBusProxy() { }; Better: DBusProxy() = default; Best: just remove it. You don't need to declare it manually, right? > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:73 > + bool isRunning() { return m_isRunning; }; bool isRunning() const { return m_isRunning; }; > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:75 > + CString proxyPath() { return m_proxyPath; }; > + CString path() { return m_path; }; These should both be const too: CString proxyPath() const { return m_proxyPath; }; CString path() const { return m_path; }; Basically: every function that doesn't modify the object should be const so that it can be used from other const member functions, or on a const object, or via a const pointer to the object. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:77 > + void appendPermissions(std::initializer_list<CString> permissions) Nice! > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:79 > + ASSERT(!m_isRunning); I would use ASSERT_WITH_SECURITY_IMPLICATION() here since any error in the use of this class is security-critical. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:92 > + GUniquePtr<char> proxySocket(makeProxySocket()); > + if (!proxySocket.get()) > + return; Should we make this a fatal error? I imagine WebKit's functionality would be quite degraded if the proxy is not working. We might want to just CRASH() here instead? > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:138 > + if (error.get()) { > + g_warning("%s", error.get()->message); > + return; > + } Ditto? > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:239 > +#if 0 > +static void bindDBusSystem(Vector<CString>& args) I see you have it disabled... what problems is it causing? Looks like this needs to be fixed before the patch can land. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:285 > + GUniquePtr<char> dconfConfigDir(g_build_filename(configDir, "dconf", nullptr)); > + bindIfExists(args, dconfConfigDir.get(), BindFlags::ReadWrite); Hmm, I think you're right that this is too big a hole... please add a FIXME here. I don't remember exactly what we discussed previously, but we should try to find some way to put dconf into a read-only mode so that our subprocesses have access to the settings but can't write them. I assume you already tried binding read-only and it didn't work? (dconf just gained a maintainer, btw, so we should be able to change it if need be.) > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:300 > +static void bindPulse(Vector<CString>& args) Should add a second FIXME regarding just how big a hole Pulse is. I was quite surprised when you taught me about the bad things a malicious subprocess could do with access to the Pulse socket. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:374 > + static DBusProxy proxy; Exit-time destructors are no bueno. Since DBusProxy has a trivial destructor where nothing important is happening, the solution is to use NeverDestroyed to prevent it from being freed: static NeverDestroyed<DBusProxy> proxy; and then proxy.get() to access it. Standard C++ advice is to put only trivially-destructible objects (e.g. plain old data) in static variables to ensure complex destructors aren't running at exit time. I don't know if DBusProxy is trivially-destructible or not (it is if it does not have a custom destructor -- true -- and if none of its member variables have custom destructors), but it's good practice and more robust, so best do it anyway. Should cause people to think very carefully if adding a destructor to the class in the future. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:457 > + // FIXME: GST_PLUGIN_SCANNER You need to check an env var? It sounds simple enough to fix now, right? > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:459 > + // FIXME: There is an env var for this too (and should we allow its dbus access?) Yeah we should allow D-Bus access. I think codec installation is broken currently anyway, though, so I'm OK with leaving that as a FIXME. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:612 > +static void > +ensureDirectoryExists(const String& path) All on one line > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:684 > + // FIXME: Find and add the permissions it needs for this (pacparser?) pacrunner > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:697 > + // "--talk=org.freedesktop.secrets" // FIXME: WebKitGTK uses for HTTP Auth I almost forgot about this. Yeah, this will take some effort to fix. That will be a bit of a project to disentangle, since we don't want to give the web process this access. Plan to handle this in a future patch. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:767 > + if (!g_key_file_load_from_file(infoFile.get(), "/.flatpak-info", G_KEY_FILE_NONE, nullptr)) { Oh no! Blocking I/O! In some cases, it's just necessary in order for the code to be sane. I don't think it's worth changing here. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:777 > +static Vector<CString> getFlatpakArgs(ProcessLauncher::ProcessType processType) This function deserves a substantial comment to explain what it is doing, and that it is not related to any of the previous sandboxing code. Currently it only becomes obvious that it's one or the other sandbox if you scroll down to line 868 and read the condition there. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:929 > + if (m_seccompFd != -1) { > + close(m_seccompFd); > + m_seccompFd = -1; > + } Huh, does the fd really need to be kept open this long? fds are kinda a precious resources and it's a bit sad to use so many this way if they're not needed. They should only be needed before bwrap has execed the WebKit subprocess, right? If that's true, then a FIXME would be warranted, at least if ProcessLauncher doesn't have an easy way to tell when it's safe to close. > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:116 > +#if PLATFORM(GTK) || PLATFORM(WPE) > + WebsiteDataStore& store = processPool().websiteDataStore()->websiteDataStore(); > + store.resolveDirectoriesIfNecessary(); > + launchOptions.extraSandboxPaths.append(store.resolvedNetworkCacheDirectory()); > + > + launchOptions.sandboxEnabled = m_processPool.sandboxEnabled(); > +#endif Good, IMO this is better than implementing SandboxExtension, even if that was quite tempting. > Source/WebKit/UIProcess/WebProcessPool.h:469 > + bool sandboxEnabled() { return m_sandboxEnabled; }; bool sandboxEnabled() const > Source/WebKit/UIProcess/WebProcessPool.h:716 > + HashMap<WebCore::SecurityOriginData, RefPtr<WebProcessProxy>> m_swappedProcesses; Woah, what's this added here for? Merge conflict gone wrong? > Source/cmake/OptionsGTK.cmake:209 > +# Since flatpak has its own sandbox we just ignore this option > +if (EXISTS "/.flatpak-info") > + set(ENABLE_BUBBLEWRAP_SANDBOX OFF) > +endif () Problem is the options list will print that it is enabled, even though it's not, since you change it after the list is finalized in WEBKIT_OPTION_END(). So this needs to be checked earlier: if (CMAKE_SYSTEM_NAME MATCHES "Linux" AND EXISTS "/.flatpak-info") WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_BUBBLEWRAP_SANDBOX PUBLIC ON) else () WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_BUBBLEWRAP_SANDBOX PRIVATE OFF) endif () Then you can do a fatal error at the top of the if (ENABLE_BUBBLEWRAP_SANDBOX) condition below here if (EXISTS "/.flatpak-info"). > ChangeLog:8 > + [GTK][WPE] Implement subprocess sandboxing Remove this duplicated title. Comment on attachment 348659 [details] [GTK][WPE] Implement subprocess sandboxing View in context: https://bugs.webkit.org/attachment.cgi?id=348659&action=review >> Source/cmake/OptionsGTK.cmake:209 >> +endif () > > Problem is the options list will print that it is enabled, even though it's not, since you change it after the list is finalized in WEBKIT_OPTION_END(). So this needs to be checked earlier: > > if (CMAKE_SYSTEM_NAME MATCHES "Linux" AND EXISTS "/.flatpak-info") > WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_BUBBLEWRAP_SANDBOX PUBLIC ON) > else () > WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_BUBBLEWRAP_SANDBOX PRIVATE OFF) > endif () > > Then you can do a fatal error at the top of the if (ENABLE_BUBBLEWRAP_SANDBOX) condition below here if (EXISTS "/.flatpak-info"). Also, since this is a lot of code that will need to be shared with OptionsWPE.cmake as well, please check how we handled this for GStreamer by splitting the code out into separate include files. (There are multiple include files because they need to be included in different places. E.g. options should not changed before WEBKIT_OPTION_END(), and must not be tested until after that point.) The overview is broken, I see a bunch of errors along the lines of: [Error] Failed to load resource: Error opening file /home/mcatanzaro/.cache/epiphany/thumbnails/80bbf48dad9161583129da74239ab0bc.png: No such file or directory (80bbf48dad9161583129da74239ab0bc.png, line 0) But ~/.cache/epiphany should have been mounted because it's the base cache dir specified in WebKitWebsiteDataManager, so something has gone wrong there. I know downloading files is not expected to work yet. My favorite download test is to visit https://www.google.com/, right click on the Google logo, and Save As. If you then click the folder icon in the download popover, gnome-shell crashes in meta_window_wayland_needs_move_resize(). This is going to be a blocker, so you can either report it to the mutter developers, or use it as an opportunity to do some mutter hacking: #0 0x00007f180d450138 in meta_window_wayland_needs_move_resize (window=window@entry=0x0) at wayland/meta-window-wayland.c:923 wl_window = 0x0 #1 0x00007f180d452302 in meta_wayland_xdg_toplevel_commit (surface_role=0x55e0430c7d60 [MetaWaylandXdgToplevel], pending=0x55e04212e6e0 [MetaWaylandPendingState]) at wayland/meta-wayland-xdg-shell.c:630 xdg_toplevel = 0x55e0430c7d60 [MetaWaylandXdgToplevel] xdg_surface = 0x55e0430c7d60 [MetaWaylandXdgToplevel] xdg_surface_priv = 0x55e0430c7d20 surface_role_class = <optimized out> surface = 0x55e04307b4c0 [MetaWaylandSurface] window = 0x0 old_geometry = {x = 0, y = 0, width = 0, height = 0} geometry_changed = <optimized out> #2 0x00007f180d449c78 in meta_wayland_surface_role_commit (pending=0x55e04212e6e0 [MetaWaylandPendingState], surface_role=<optimized out>) at wayland/meta-wayland-surface.h:45 __func__ = "meta_wayland_surface_apply_pending_state" #3 0x00007f180d449c78 in meta_wayland_surface_apply_pending_state (surface=0x55e04307b4c0 [MetaWaylandSurface], pending=0x55e04212e6e0 [MetaWaylandPendingState]) at wayland/meta-wayland-surface.c:720 __func__ = "meta_wayland_surface_apply_pending_state" #4 0x00007f1809c3a03e in ffi_call_unix64 () at ../src/x86/unix64.S:76 #5 0x00007f1809c399ff in ffi_call (cif=<optimized out>, fn=<optimized out>, rvalue=<optimized out>, avalue=<optimized out>) at ../src/x86/ffi64.c:525 classes = {X86_64_INTEGER_CLASS, X86_64_NO_CLASS, 1082533168, 21984} stack = <optimized out> argp = <optimized out> arg_types = <optimized out> gprcount = <optimized out> ssecount = <optimized out> ngpr = 1 nsse = 0 i = <optimized out> avn = <optimized out> ret_in_memory = <optimized out> reg_args = <optimized out> #6 0x00007f1803213f2d in wl_closure_invoke (closure=closure@entry=0x55e042f9be00, flags=flags@entry=2, target=<optimized out>, target@entry=0x55e040862530, opcode=opcode@entry=6, data=<optimized out>, data@entry=0x55e042e1c6a0) at src/connection.c:996 count = <optimized out> cif = {abi = FFI_UNIX64, nargs = 2, arg_types = 0x7ffdc3b83cb0, rtype = 0x7f1809c3a430 <ffi_type_void>, bytes = 0, flags = 0} ffi_types = {0x7f1809c3a310 <ffi_type_pointer>, 0x7f1809c3a310 <ffi_type_pointer>, 0x7f1809c3a390 <ffi_type_uint32>, 0x7f1809c3a370 <ffi_type_sint32>, 0x7f1809c3a370 <ffi_type_sint32>, 0x7f1809c3a370 <ffi_type_sint32>, 0x7f1809c3a370 <ffi_type_sint32>, 0x7f1809c3a390 <ffi_type_uint32>, 0x217e37896, 0x1, 0x7ffdc3b83d30, 0x7f1803212248 <wl_buffer_put+72>, 0x0, 0x7ffdc3b83d90, 0x7ffdc3b83d30, 0x7f1803212874 <wl_connection_read+372>, 0x0, 0x200000000, 0x7ffdc3b83d70, 0x2, 0x7ffdc3b83d90, 0x7f18084af0d0 <wl_surface_requests+144>} ffi_args = {0x7ffdc3b83c80, 0x7ffdc3b83c88, 0x7f18084af0d0 <wl_surface_requests+144>, 0x7f1803212c80 <wl_closure_init+224>, 0x55e04309b4e8, 0x55e040862530, 0x8, 0x55e042e1c6d0, 0x6, 0x7f1803213780 <wl_connection_demarshal+144>, 0x55e042f9bed8, 0x55e0407ab800, 0x55e04309b590, 0x55e042f9be00, 0x55e04309b59c, 0x55e042e1c6d0, 0x8402fd0f0, 0x7f1803213c1b <wl_closure_lookup_objects+171>, 0x55e0407ad810, 0x7f180320e977 <log_closure+71>, 0x7f18084af0d0 <wl_surface_requests+144>, 0x3212d7d} implementation = <optimized out> #7 0x00007f18032103df in wl_client_connection_data (fd=<optimized out>, mask=<optimized out>, data=0x55e042e1c6a0) at src/wayland-server.c:420 client = 0x55e042e1c6a0 connection = 0x55e0407ab800 resource = 0x55e040862530 object = 0x55e040862530 closure = 0x55e042f9be00 message = 0x7f18084af0d0 <wl_surface_requests+144> p = {29, 524294} resource_flags = <optimized out> opcode = 6 size = <optimized out> since = <optimized out> len = <optimized out> #8 0x00007f1803211f02 in wl_event_loop_dispatch (loop=0x55e040101c70, timeout=timeout@entry=0) at src/event-loop.c:641 ep = {{events = 1, data = {ptr = 0x7f17e00220f0, fd = -536731408, u32 = 3758235888, u64 = 139740519211248}}, {events = 0, data = {ptr = 0x7ffdc3b83ed0, fd = -1011335472, u32 = 3283631824, u64 = 140727887085264}}, {events = 1, data = {ptr = 0xc3b83f2000000000, fd = 0, u32 = 0, u64 = 14103091639782211584}}, {events = 32765, data = {ptr = 0x50, fd = 80, u32 = 80, u64 = 80}}, {events = 0, data = {ptr = 0xef02e3800000000, fd = 0, u32 = 0, u64 = 1076411128994594816}}, {events = 32536, data = {ptr = 0x0, fd = 0, u32 = 0, u64 = 0}}, {events = 0, data = {ptr = 0x430dcad000000000, fd = 0, u32 = 0, u64 = 4831740969940418560}}, {events = 21984, data = {ptr = 0x7f180ef31fd1 <g_slice_alloc+33>, fd = 250814417, u32 = 250814417, u64 = 139741306757073}}, {events = 1077581824, data = {ptr = 0xef02e38000055e0, fd = 21984, u32 = 21984, u64 = 1076411128994616800}}, {events = 32536, data = {ptr = 0x55e0403a9800, fd = 1077581824, u32 = 1077581824, u64 = 94421638617088}}, {events = 1124971328, data = {ptr = 0x17e37896000055e0, fd = 21984, u32 = 21984, u64 = 1721352068226110944}}, {events = 2, data = {ptr = 0x0, fd = 0, u32 = 0, u64 = 0}}, {events = 1124977360, data = {ptr = 0x8a3a7400000055e0, fd = 21984, u32 = 21984, u64 = 9960401069232117216}}, {events = 277936694, data = {ptr = 0x55e0403a9800, fd = 1077581824, u32 = 1077581824, u64 = 94421638617088}}, {events = 1101317360, data = {ptr = 0x2000055e0, fd = 21984, u32 = 21984, u64 = 8589956576}}, {events = 0, data = {ptr = 0x1090fa368a3a7400, fd = -1975880704, u32 = 2319086592, u64 = 1193729013407446016}}, {events = 3283632344, data = {ptr = 0x41a4c4f000007ffd, fd = 32765, u32 = 32765, u64 = 4730122043717091325}}, {events = 21984, data = {ptr = 0x2, fd = 2, u32 = 2, u64 = 2}}, {events = 3283632352, data = {ptr = 0xc3b840d800007ffd, fd = 32765, u32 = 32765, u64 = 14103093529567854589}}, {events = 32765, data = {ptr = 0x7fffffff, fd = 2147483647, u32 = 2147483647, u64 = 2147483647}}, {events = 2351699200, data = {ptr = 0x6e2d90100007f17, fd = 32535, u32 = 32535, u64 = 496197507282403095}}, {events = 32536, data = {ptr = 0x7ffdc3b84020, fd = -1011335136, u32 = 3283632160, u64 = 140727887085600}}, {events = 2319086592, data = {ptr = 0xc3b840301090fa36, fd = 277936694, u32 = 277936694, u64 = 14103092808291252790}}, {events = 32765, data = {ptr = 0x7ffdc3bd5bcc <clock_gettime+476>, fd = -1011000372, u32 = 3283966924, u64 = 140727887420364}}, {events = 3283632224, data = {ptr = 0x8a3a740000007ffd, fd = 32765, u32 = 32765, u64 = 9960401069232127997}}, {events = 1, data = {ptr = 0x1, fd = 1, u32 = 1, u64 = 1}}, {events = 2147483647, data = {ptr = 0xc3b840d800000000, fd = 0, u32 = 0, u64 = 14103093529567821824}}, {events = 32765, data = {ptr = 0x0, fd = 0, u32 = 0, u64 = 0}}, {events = 3283632224, data = {ptr = 0xd13a9bc00007ffd, fd = 32765, u32 = 32765, u64 = 942283371969544189}}, {events = 32536, data = {ptr = 0x20, fd = 32, u32 = 32, u64 = 32}}, {events = 8, data = {ptr = 0x4010664000000000, fd = 0, u32 = 0, u64 = 4616302043118698496}}, {events = 21984, data = {ptr = 0x7ffdc3b84070, fd = -1011335056, u32 = 3283632240, u64 = 140727887085680}}} source = <optimized out> i = <optimized out> count = <optimized out> #9 0x00007f180d43347b in wayland_event_source_dispatch (base=<optimized out>, callback=<optimized out>, data=<optimized out>) at wayland/meta-wayland.c:86 source = <optimized out> loop = <optimized out> #10 0x00007f180ef148ad in g_main_dispatch (context=0x55e04012b520) at gmain.c:3177 dispatch = 0x7f180d433460 <wayland_event_source_dispatch> prev_source = 0x0 was_in_call = 0 user_data = 0x0 callback = 0x0 cb_funcs = 0x0 cb_data = 0x0 need_destroy = <optimized out> source = 0x55e040402950 current = 0x55e040143d30 i = 0 __func__ = "g_main_dispatch" #11 0x00007f180ef148ad in g_main_context_dispatch (context=context@entry=0x55e04012b520) at gmain.c:3830 #12 0x00007f180ef14c78 in g_main_context_iterate (context=0x55e04012b520, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at gmain.c:3903 max_priority = 2147483647 timeout = 989 some_ready = 1 nfds = <optimized out> allocated_nfds = 16 fds = 0x7f17e001b8d0 #13 0x00007f180ef14fa2 in g_main_loop_run (loop=0x55e0403e1700) at gmain.c:4099 __func__ = "g_main_loop_run" #14 0x00007f180d3f9f70 in meta_run () at core/main.c:664 #15 0x000055e03ee164d8 in main (argc=<optimized out>, argv=<optimized out>) at ../src/main.c:525 ctx = <optimized out> error = 0x0 ecode = <optimized out> Tested out a few websites: * bugs.webkit.org and https://trac.webkit.org/ both work perfectly under the sandbox * reddit.com, youtube.com, gitlab.gnome.org, and duckduckgo.com are all busted -- no content is rendered at all. (It's really important to use this as your primary browser until the end of the project, or you won't be able to notice such problems.) All of the above are fixed by running with WEBKIT_DISABLE_COMPOSITING_MODE=1, so I conclude that you've broken accelerated compositing mode somehow. For each tab I load, I see the following warning: WaylandCompositorDisplay initialization: failed to connect to the Wayland display: webkitgtk-wayland-compositor-2eb55b57-d9f1-44ac-a2ca-bac977b4d281 That's almost certainly the cause, so that will need fixed. I also see one: ** (epiphany:15576): WARNING **: 12:02:15.413: Empty path passed to sandbox, this is probably a bug but I don't know how I triggered that. I would make it a fatal error (CRASH() or g_error() or RELEASE_ASSERT_NOT_REACHED()) since we really such not tolerate any errors from the sandbox code. Created attachment 348781 [details]
[GTK][WPE] Implement subprocess sandboxing
(In reply to Michael Catanzaro from comment #27) > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:92 > > + GUniquePtr<char> proxySocket(makeProxySocket()); > > + if (!proxySocket.get()) > > + return; > > Should we make this a fatal error? > > I imagine WebKit's functionality would be quite degraded if the proxy is not > working. We might want to just CRASH() here instead? Kinda sucks but yeah. > > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:239 > > +#if 0 > > +static void bindDBusSystem(Vector<CString>& args) > > I see you have it disabled... what problems is it causing? Looks like this > needs to be fixed before the patch can land. We just don't use any system services atm. > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:285 > > + GUniquePtr<char> dconfConfigDir(g_build_filename(configDir, "dconf", nullptr)); > > + bindIfExists(args, dconfConfigDir.get(), BindFlags::ReadWrite); > > Hmm, I think you're right that this is too big a hole... please add a FIXME > here. I don't remember exactly what we discussed previously, but we should > try to find some way to put dconf into a read-only mode so that our > subprocesses have access to the settings but can't write them. I assume you > already tried binding read-only and it didn't work? > > (dconf just gained a maintainer, btw, so we should be able to change it if > need be.) Upstream DConf already planned on working on this but I don't think its far along (no idea if I could help). I didn't actually test making it read-only but I just imagined that doesn't matter when you can talk to the DBus service. I briefly discussed this with mclasen and it seems like a portal to access Gtk Settings would be accepted. I'll let you decide if thats worth working on. (That also means we would depend on xdg-desktop-portal.) > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:459 > > + // FIXME: There is an env var for this too (and should we allow its dbus access?) > > Yeah we should allow D-Bus access. I think codec installation is broken > currently anyway, though, so I'm OK with leaving that as a FIXME. > Added permissions. It works fine when tested manually (and back when I tried MiniBrowser) but not within Epiphany so I think the bug is there. > > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:684 > > + // FIXME: Find and add the permissions it needs for this (pacparser?) > > pacrunner I needed the bus names and possibly to test it. > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:767 > > + if (!g_key_file_load_from_file(infoFile.get(), "/.flatpak-info", G_KEY_FILE_NONE, nullptr)) { > > Oh no! Blocking I/O! > > In some cases, it's just necessary in order for the code to be sane. I don't > think it's worth changing here. I believe `/.flatpak-info` is in tmpfs anyway. > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:777 > > +static Vector<CString> getFlatpakArgs(ProcessLauncher::ProcessType processType) > > This function deserves a substantial comment to explain what it is doing, > and that it is not related to any of the previous sandboxing code. Currently > it only becomes obvious that it's one or the other sandbox if you scroll > down to line 868 and read the condition there. > > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:929 > > + if (m_seccompFd != -1) { > > + close(m_seccompFd); > > + m_seccompFd = -1; > > + } > > Huh, does the fd really need to be kept open this long? No, fixed. It might be cleaner to refactor all of this to use GSubprocessLauncher and use g_subprocess_launcher_take_fd() later. --- I have resolved all other comments in comment #27. Attachment 348781 [details] did not pass style-queue:
ERROR: Source/WebKit/UIProcess/WebProcessPool.h:468: The parameter name "enabled" adds no information, so it should be removed. [readability/parameter_name] [5]
WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/wpe/WebKitWebContext.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitWebContext.h"
ERROR: Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:77: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5]
ERROR: Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:89: Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(). [security/assertion] [5]
Total errors found: 3 in 20 files
If any of these errors are false positives, please file a bug against check-webkit-style.
> Excellent! We are quite close to being able to land this. I'll probably give r+ after you address the next round of feedback.
Hmm, The DBusProxy isn't behaving as expected atm so I just want to say please don't merge this too hastily before I give the OK.
(In reply to Patrick Griffis from comment #33) > We just don't use any system services atm. Let's not leave the #if 0 code then, but add a comment explaining how to add the system proxy if needed in the future. > > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:285 > > > + GUniquePtr<char> dconfConfigDir(g_build_filename(configDir, "dconf", nullptr)); > > > + bindIfExists(args, dconfConfigDir.get(), BindFlags::ReadWrite); > > > > Hmm, I think you're right that this is too big a hole... please add a FIXME > > here. I don't remember exactly what we discussed previously, but we should > > try to find some way to put dconf into a read-only mode so that our > > subprocesses have access to the settings but can't write them. I assume you > > already tried binding read-only and it didn't work? > > > > (dconf just gained a maintainer, btw, so we should be able to change it if > > need be.) > > Upstream DConf already planned on working on this but I don't think its far > along (no idea if I could help). I didn't actually test making it read-only > but I just imagined that doesn't matter when you can talk to the DBus > service. > > I briefly discussed this with mclasen and it seems like a portal to access > Gtk Settings would be accepted. I'll let you decide if thats worth working > on. (That also means we would depend on xdg-desktop-portal.) I think it would fall within the scope of this project, since we really don't want to allow write access to dconf. I don't think it's the highest-priority until functionality regressions are addressed. E.g. credentials access for HTTP auth is still an issue. And drag-and-drop. > > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:459 > > > + // FIXME: There is an env var for this too (and should we allow its dbus access?) > > > > Yeah we should allow D-Bus access. I think codec installation is broken > > currently anyway, though, so I'm OK with leaving that as a FIXME. > > > > Added permissions. It works fine when tested manually (and back when I tried > MiniBrowser) but > not within Epiphany so I think the bug is there. Epiphany doesn't have any support for missing codecs installation. I have a WIP patch in https://gitlab.gnome.org/GNOME/epiphany/commit/7a5e7f98a18e605c20f8b0333d753c3a66495329. At the time I was working on that, it was broken due to bug #147822 (see comment 3 there), so there was no point in adding the code to Epiphany. It's possible something has changed in the past two years, but I'm not sure what it would have been since we really had no idea how to make it work as desired. (In reply to Patrick Griffis from comment #35) > Hmm, The DBusProxy isn't behaving as expected atm so I just want to say > please don't merge this too hastily before I give the OK. When you are ready for it to be committed, you'll need to set the cq? Bugzilla flag, then I can change it to cq+ to trigger the commit. You'll also need to use the r? flag to request review. Created attachment 349776 [details]
[GTK][WPE] Implement subprocess sandboxing
Comment on attachment 349776 [details] [GTK][WPE] Implement subprocess sandboxing View in context: https://bugs.webkit.org/attachment.cgi?id=349776&action=review Excellent. r=me, meaning this has passed my review and is ready to commit without further review after addressing the pending comments. Ordinarily I would set r+ now, but Carlos Garcia actually wanted to review this too, so we're going to require two reviewers on this one (assuming he gets around to it soon). Please read https://webkit.org/code-style-guidelines/#comments regarding all the comments. > Source/WebKit/ChangeLog:4 > + [GTK][WPE] Implement subprocess sandboxing > + https://bugs.webkit.org/show_bug.cgi?id=188568 The ChangeLog should also mention the current limitations and plans for resolving them: * file:// access broken (which we don't know how to handle) * credentials access broken (for immediate follow-up) > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1163 > + * If you use `$XDG_CONFIG_HOME/g_get_prgname()` in your #WebKitUserContentManager > + * you must ensure it exists before subprocess are created. #WebKitWebsiteDataManager subprocesses > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:204 > + static char* makeProxySocket(const char *appRunDir) const char* appRunDir > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:208 > + return nullptr; Omit the return, since it's unreachable and surely not needed to avoid warnings. That will just confuse developers about g_error(). > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:216 > + return nullptr; Ditto. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:536 > +static int setupSeccomp() TODO (after this lands): ideally these would be split from flatpak into some helper library so that WebKit doesn't have to duplicate them. It's too security-sensitive to duplicate here long-term. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:723 > + // FIXME: The network process is used for `file://` URIs and > + // we would have to pass through most paths for this to work Probably the biggest limitation and behavior change, but we don't currently have a plan to fix this, and don't want to block on it. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:733 > + > + No blank lines here. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:737 > + "--talk=org.freedesktop.ScreenSaver", // GStreamer inhibits, FIXME: Use xdg-desktop-portal Actually I already wrote the code for this, it's in SleepDisablerGLib.cpp. Should fall back if org.freedesktop.ScreenSaver is unavailable. Is it not working? > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:742 > + // "--talk=org.freedesktop.secrets" // FIXME: WebKitGTK uses for HTTP Auth Don't leave code commented out. File a new bug report for fixing the credentials access. > Source/cmake/OptionsGTK.cmake:210 > +if (ENABLE_BUBBLEWRAP_SANDBOX) I'm surprised that you clearly tested this for WPE (I see you added the /dev/fb devices) but didn't set up the build system for it to work? This all should be split out into another CMake file to be included by both OptionsGTK.cmake and OptionsWPE.cmake. Anyway, it can be done in a future patch, I suppose. > TODO (after this lands): ideally these would be split from flatpak into some helper library so that WebKit doesn't have to duplicate them. It's too security-sensitive to duplicate here long-term. Last I mentioned pulling any of this out into a lib to Alex he didn't seem interested. > Actually I already wrote the code for this, it's in SleepDisablerGLib.cpp. Should fall back if org.freedesktop.ScreenSaver is unavailable. Is it not working? Works fine, removed. > I'm surprised that you clearly tested this for WPE (I see you added the /dev/fb devices) but didn't set up the build system for it to work? I didn't test it, that's why I have yet to add the option to WPE. I can add it now but I'd want to build it and test it first. Comment on attachment 349776 [details] [GTK][WPE] Implement subprocess sandboxing View in context: https://bugs.webkit.org/attachment.cgi?id=349776&action=review I have no idea about sandboxing, so I'm sorry if I make any stupid comment or question about it. r- mainly because of the memory leaks and coding style issues, my other comments are probably because I don't know who the sandbox works. > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1158 > + * This method **must be called before any web process has been created**, > + * as early as possible in your application. Calling it later is a fatal error. Maybe we could make it a construct only property then, because it doesn't make sense to change it either. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:72 > +static int > +argsToFd(const Vector<CString>& args, const char *name) This should be a single line. The function name is confusing, sounds to me like it's converting arguments to file descriptors. What is it doing exactly? > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:79 > + GRefPtr<GBytes> bytes(g_string_free_to_bytes(buffer)); This is leaked, the reference needs to be adopted. Use GRefPtr<GBytes> bytes = adoptGRef(g_string_free_to_bytes(buffer)); > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:81 > + int memfd = memfd_create(name, MFD_ALLOW_SEALING); Should we also pass MFD_CLOEXEC? > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:86 > + ssize_t ret; Move this below, where it's first needed. Do not use abbreviations for the variable names. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:93 > + if ((size_t)ret != size) Use C++ style casts. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:105 > +class DBusProxy { This is too generic name for this class, I guess it doesn't represent a generic DBus proxy. What is this for? > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:120 > + CString proxyPath() const { return m_proxyPath; }; const CString& > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:121 > + CString path() const { return m_path; }; Ditto. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:127 > + void appendPermissions(std::initializer_list<CString> permissions) > + { > + RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(!m_isRunning); > + > + Vector<CString> newPermissions(permissions); Why doesn't this receive a Vector<CString> directly? > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:137 > + const char* runDir = g_get_user_runtime_dir(); > + GUniquePtr<char> appRunDir(g_build_filename(runDir, g_get_prgname(), nullptr)); I would use g_get_user_runtime_dir() directly, since runDir isn't reused, right? > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:141 > + m_proxyPath = proxySocket.get(); We could avoid another copy here if m_proxyPath was a GUniquePtr<char> instead of a CString. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:143 > + Vector<CString> bwrapArgs = { Why are we using Vector<CString>? that means we are duplicating every string (most of them are static or already owned by the class), and then copied again in argsToFd. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:157 > + "--bind-try", m_path, m_path, I guess this should be m_path.data() in both cases. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:193 > + if (!g_spawn_async(NULL, argv, NULL, G_SPAWN_LEAVE_DESCRIPTORS_OPEN, NULL, NULL, &m_pid, &error.outPtr())) { Use nullptr instead of NULL. Could we use GSubprocess API? since this patch is adding new deps, I think it's fine to also make the faue depend on a newer glib too. Why do we leave the descriptors open? >> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:204 >> + static char* makeProxySocket(const char *appRunDir) > > const char* appRunDir This should return a GUniquePtr<char> >> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:208 >> + return nullptr; > > Omit the return, since it's unreachable and surely not needed to avoid warnings. That will just confuse developers about g_error(). Why are all the errors fatal in this patch? > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:211 > + char* proxySocketTemplate = g_build_filename(appRunDir, "dbus-proxy-XXXXXX", nullptr); GUniquePtr > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:220 > + return proxySocketTemplate; I'm confused about this method too, it sounds like it's creating a proxy socket, but it's just creating a temp file. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:226 > + bool m_isRunning = false; bool m_isRunning { false }; > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:227 > + GPid m_pid; I wonder if we could make this a std::optional and then we don't need m_isRunning > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:228 > + Vector<CString> m_permissions = { }; Vector<CString> m_permissions; > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:231 > +static char* dbusAddressToPath(const char* address, bool abstract = false) This should return a GUniquePtr<char>. Use an enum instead of a boolean parameter. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:234 > + const char* path; > + const char* path_end; Move this where they are fist needed. path_end -> pathEnd > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:242 > + path = strstr(address, abstract ? "abstract=" : "path="); const char* path = > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:247 > + path_end = path; const char* pathEnd = > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:260 > +static void bindIfExists(Vector<CString>& args, const char* path, BindFlags bindFlags = BindFlags::ReadOnly) I also find this name confusing, I guess appendBindArguments or something like that would be more accurate, no? What should exist? we only check if path is nullptr or not > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:265 > + const char* type; bindType? > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:271 > + type = "--bind-try"; ah, the if exists is because of the -try suffix? > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:287 > + proxy.launch(); So, this is the one launching the bwrap executable. Should this be launchWhateverIfNeeded instead? or ensureWhatever? > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:356 > + if (pulseConfig) > + bindIfExists(args, pulseConfig); Are we ever actually passing a nullptr as path? What is the if exists for then? > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:413 > + static DBusProxy proxy; So, the proxy is only for a11y? > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:417 > + GRefPtr<GDBusConnection> sessionBus(g_bus_get_sync(G_BUS_TYPE_SESSION, nullptr, nullptr)); This is leaked too, you need to adopt the returned ref. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:421 > + GRefPtr<GDBusMessage> msg(g_dbus_message_new_method_call("org.a11y.Bus", "/org/a11y/bus", "org.a11y.Bus", "GetAddress")); This is leaked too, you need to adopt the returned ref. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:423 > + GRefPtr<GDBusMessage> reply(g_dbus_connection_send_message_with_reply_sync( This is leaked too, you need to adopt the returned ref. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:436 > + g_warning("Can't find a11y bus: %s", error.get()->message); error->message > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:563 > + struct scmp_arg_cmp clone_arg = SCMP_A0(SCMP_CMP_MASKED_EQ, CLONE_NEWUSER, CLONE_NEWUSER); clone_arg -> cloneArg > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:564 > + struct scmp_arg_cmp tty_arg = SCMP_A1(SCMP_CMP_EQ, (int)TIOCSTI); tty_arg -> ttyArg > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:568 > + } syscall_blacklist[] = { syscall_blankclist -> syscallBlacklist > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:613 > + g_error("Failed to init seccomp"); > + return -1; Return after g_error again here. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:626 > + g_error("Failed to add seccomp rule"); > + seccomp_release(seccomp); > + return -1; Return and release after g_error here. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:634 > + if (tmpfd == -1) { > + g_error("Failed to create memfd: %s", g_strerror(errno)); > + seccomp_release(seccomp); > + return -1; This can't happen, memfd_create already handles all errors caling g_error > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:641 > + g_error("Failed to export seccomp bpf"); > + seccomp_release(seccomp); > + close(tmpfd); > + return -1; More g_error here > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:659 > +static void ensureDirectoryExists(const String& path) > +{ > + if (path.isEmpty()) > + return; > + > + // FIXME: Blocking > + if (g_mkdir_with_parents(path.utf8().data(), 0700) == -1 && errno != ENOTDIR) > + g_warning("Could not create directory \"%s\": %s", path.utf8().data(), g_strerror(errno)); > +} You can use Filesystem::makeAllDirectories() > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:673 > + // NOTE: This is not a great solution but we just assume that applications create this directory > + // ahead of time if they require it > + GUniquePtr<char> configDir(g_build_filename(g_get_user_config_dir(), g_get_prgname(), nullptr)); Shouldn't we add API to allow applications pass their config/data dirs instead? > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:716 > + // We would have to parse ld config files for more info Comments should finish with period. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:810 > + Vector<CString> bwrapArgs = { I'm confused again, wasn't this done by the proxy launch method? > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:858 > +#endif I think all the sandbox code should be moved to its own file > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:931 > + int seccompFd = -1; > + int bwrapFd = -1; std::optional > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:936 > + Vector<CString> sandboxArgs = { }; { } is not needed, Vector is already empty. > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:116 > +#if PLATFORM(GTK) || PLATFORM(WPE) > + WebsiteDataStore& store = processPool().websiteDataStore()->websiteDataStore(); > + store.resolveDirectoriesIfNecessary(); > + launchOptions.extraSandboxPaths.append(store.resolvedNetworkCacheDirectory()); > + > + launchOptions.sandboxEnabled = m_processPool.sandboxEnabled(); > +#endif I think we should add a platformGetLaunchOptions or something like that instead of adding platform ifdefs here. Should this depend on ENABLE(BUBBLEWRAP_SANDBOX) or OS(LINUX) instead of GTK|WPE? > Source/WebKit/UIProcess/Storage/StorageProcessProxy.cpp:95 > + WebsiteDataStore& store = m_processPool.websiteDataStore()->websiteDataStore(); > + store.resolveDirectoriesIfNecessary(); > + launchOptions.extraSandboxPaths.append(store.resolvedIndexedDatabaseDirectory()); Should we do this even when sandbox is disabled? > Source/cmake/WebKitFeatures.cmake:88 > + WEBKIT_OPTION_DEFINE(ENABLE_BUBBLEWRAP_SANDBOX "Toggle bubblewrap sandboxing support" PRIVATE OFF) Why is this private? (In reply to Michael Catanzaro from comment #38) > Comment on attachment 349776 [details] > [GTK][WPE] Implement subprocess sandboxing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=349776&action=review > > Excellent. r=me, meaning this has passed my review and is ready to commit > without further review after addressing the pending comments. Ordinarily I > would set r+ now, but Carlos Garcia actually wanted to review this too, so > we're going to require two reviewers on this one (assuming he gets around to > it soon). > This patch adds new API, so it requires approval of two GTK+ reviewers in any case. Note that it also requires approval of a WebKit2 owner. Created attachment 349885 [details]
[GTK][WPE] Implement subprocess sandboxing
Attachment 349885 [details] did not pass style-queue:
WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/wpe/WebKitWebContext.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit/UIProcess/API/gtk/WebKitWebContext.h"
ERROR: Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:201: One line control clauses should not use braces. [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:221: One line control clauses should not use braces. [whitespace/braces] [4]
ERROR: Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:624: One line control clauses should not use braces. [whitespace/braces] [4]
Total errors found: 3 in 21 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 349888 [details]
[GTK][WPE] Implement subprocess sandboxing
(In reply to Carlos Garcia Campos from comment #40) > Comment on attachment 349776 [details] > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:72 > > +static int > > +argsToFd(const Vector<CString>& args, const char *name) > > This should be a single line. The function name is confusing, sounds to me > like it's converting arguments to file descriptors. What is it doing exactly? The list of permissions can get quite long and instead of passing 100s of args to command lines you can pass args as an fd, so it shoves the args in a memfd. > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:81 > > + int memfd = memfd_create(name, MFD_ALLOW_SEALING); > > Should we also pass MFD_CLOEXEC? We are passing the fd down two layers, bwrap -> dbus proxy, so it doesn't work here. > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:105 > > +class DBusProxy { > > This is too generic name for this class, I guess it doesn't represent a > generic DBus proxy. What is this for? It just contains the state to launch a DBus proxy. > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:127 > > + void appendPermissions(std::initializer_list<CString> permissions) > > + { > > + RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(!m_isRunning); > > + > > + Vector<CString> newPermissions(permissions); > > Why doesn't this receive a Vector<CString> directly? I just found it cleaner to pass {"initializer", "lists"} around than create vectors. > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:141 > > + m_proxyPath = proxySocket.get(); > > We could avoid another copy here if m_proxyPath was a GUniquePtr<char> > instead of a CString. Doesn't it then have to make a copy anyway when returning it in `proxyPath()`? > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:143 > > + Vector<CString> bwrapArgs = { > > Why are we using Vector<CString>? that means we are duplicating every string > (most of them are static or already owned by the class), and then copied > again in argsToFd. No reason, do you mean it should be a Vector<char*>? > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:193 > > + if (!g_spawn_async(NULL, argv, NULL, G_SPAWN_LEAVE_DESCRIPTORS_OPEN, NULL, NULL, &m_pid, &error.outPtr())) { > > Use nullptr instead of NULL. Could we use GSubprocess API? since this patch > is adding new deps, I think it's fine to also make the faue depend on a > newer glib too. Why do we leave the descriptors open? Sure we could. Same FD comment as a above, the `bwrap` command passes an fd to its child `xdg-dbus-proxy`. We could just not sandbox `xdg-dbus-proxy` and clean this up a bit. The extra layer of sandboxing surely can't hurt but its probably safe and `flatpak` doesn't do it. > >> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:208 > >> + return nullptr; > > > > Omit the return, since it's unreachable and surely not needed to avoid warnings. That will just confuse developers about g_error(). > > Why are all the errors fatal in this patch? Michael asked for them to be. Really they can all be handled but the end process should fail to start. > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:260 > > +static void bindIfExists(Vector<CString>& args, const char* path, BindFlags bindFlags = BindFlags::ReadOnly) > > I also find this name confusing, I guess appendBindArguments or something > like that would be more accurate, no? What should exist? we only check if > path is nullptr or not > ah, the if exists is because of the -try suffix? Yes. `bindIfPathExists()` sound better to you? > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:287 > > + proxy.launch(); > > So, this is the one launching the bwrap executable. Should this be > launchWhateverIfNeeded instead? or ensureWhatever? `proxy.launchProxy()` ? I'm not sure the duplication is needed. > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:413 > > + static DBusProxy proxy; > > So, the proxy is only for a11y? There are 3 proxies per UI process. 1 for a11y and 2 for session bus for web and network processes. (At a later point another for system bus probably.) > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:673 > > + // NOTE: This is not a great solution but we just assume that applications create this directory > > + // ahead of time if they require it > > + GUniquePtr<char> configDir(g_build_filename(g_get_user_config_dir(), g_get_prgname(), nullptr)); > > Shouldn't we add API to allow applications pass their config/data dirs > instead? Michael decided not to for now. > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:810 > > + Vector<CString> bwrapArgs = { > > I'm confused again, wasn't this done by the proxy launch method? We are launching multiple processes. > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:858 > > +#endif > > I think all the sandbox code should be moved to its own file Maybe, what do you think Michael? > > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:116 > > +#if PLATFORM(GTK) || PLATFORM(WPE) > > + WebsiteDataStore& store = processPool().websiteDataStore()->websiteDataStore(); > > + store.resolveDirectoriesIfNecessary(); > > + launchOptions.extraSandboxPaths.append(store.resolvedNetworkCacheDirectory()); > > + > > + launchOptions.sandboxEnabled = m_processPool.sandboxEnabled(); > > +#endif > > I think we should add a platformGetLaunchOptions or something like that > instead of adding platform ifdefs here. Should this depend on > ENABLE(BUBBLEWRAP_SANDBOX) or OS(LINUX) instead of GTK|WPE? I don't know if `platformGetLaunchOptions()` would be much better. Could change the ifdef though. > > Source/cmake/WebKitFeatures.cmake:88 > > + WEBKIT_OPTION_DEFINE(ENABLE_BUBBLEWRAP_SANDBOX "Toggle bubblewrap sandboxing support" PRIVATE OFF) > > Why is this private? Because it only supports Linux, why show it on non-Linux. --- I addressed all other comments. Errors initializing the sandbox are ultimately errors in the UI process, so it makes sense to me that the UI process should terminate. But I suppose it's OK to not do that so long as the secondary process is guaranteed to not start and the WebKitWebView emits the crashed/terminated signals. (In reply to Patrick Griffis from comment #45) > > I think all the sandbox code should be moved to its own file > > Maybe, what do you think Michael? Whatever Carlos prefers. It's iffy. I considered requesting this myself, but decided in the end that it was fine in ProcessLauncher because it's intimately tied to the process launching. It's impossible to reason about or debug process launching without reading the sandboxing code, and if we split it into a separate file, the sandboxing code is ultimately just going to return a bunch of args to pass to g_spawn_async(). So keeping it in ProcessLauncher seemed fine to me. > > > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:116 > > > +#if PLATFORM(GTK) || PLATFORM(WPE) > > > + WebsiteDataStore& store = processPool().websiteDataStore()->websiteDataStore(); > > > + store.resolveDirectoriesIfNecessary(); > > > + launchOptions.extraSandboxPaths.append(store.resolvedNetworkCacheDirectory()); > > > + > > > + launchOptions.sandboxEnabled = m_processPool.sandboxEnabled(); > > > +#endif > > > > I think we should add a platformGetLaunchOptions or something like that > > instead of adding platform ifdefs here. Should this depend on > > ENABLE(BUBBLEWRAP_SANDBOX) or OS(LINUX) instead of GTK|WPE? > > I don't know if `platformGetLaunchOptions()` would be much better. Could > change the ifdef though. If it's easy to split the code into a platformGetLaunchOptions(), that would be nice/idiomatic. P.S. I'd encourage you to test with asan ('set-webkit-configuration --debug --asan') to catch those leaks you introduced. It's extremely difficult to avoid introducing leaks if you're not developing with asan. (In reply to Patrick Griffis from comment #45) > (In reply to Carlos Garcia Campos from comment #40) > > Comment on attachment 349776 [details] > > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:72 > > > +static int > > > +argsToFd(const Vector<CString>& args, const char *name) > > > > This should be a single line. The function name is confusing, sounds to me > > like it's converting arguments to file descriptors. What is it doing exactly? > > The list of permissions can get quite long and instead of passing 100s of > args > to command lines you can pass args as an fd, so it shoves the args in a > memfd. so writeArgumentsToMemory() is what the function does, right? > > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:81 > > > + int memfd = memfd_create(name, MFD_ALLOW_SEALING); > > > > Should we also pass MFD_CLOEXEC? > > We are passing the fd down two layers, bwrap -> dbus proxy, so it doesn't > work > here. > > > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:105 > > > +class DBusProxy { > > > > This is too generic name for this class, I guess it doesn't represent a > > generic DBus proxy. What is this for? > > It just contains the state to launch a DBus proxy. So, DBusProxyLauncher? I still think of something like GDBusProxy instead. What about XDGDBusProxyLauncher? > > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:127 > > > + void appendPermissions(std::initializer_list<CString> permissions) > > > + { > > > + RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(!m_isRunning); > > > + > > > + Vector<CString> newPermissions(permissions); > > > > Why doesn't this receive a Vector<CString> directly? > > I just found it cleaner to pass {"initializer", "lists"} around than create > vectors. Can't you use { } if the function receives a Vector<>&& ? > > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:141 > > > + m_proxyPath = proxySocket.get(); > > > > We could avoid another copy here if m_proxyPath was a GUniquePtr<char> > > instead of a CString. > > Doesn't it then have to make a copy anyway when returning it in > `proxyPath()`? No, it would just return a const char* instead of CString, and then the caller decide what to do with it. > > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:143 > > > + Vector<CString> bwrapArgs = { > > > > Why are we using Vector<CString>? that means we are duplicating every string > > (most of them are static or already owned by the class), and then copied > > again in argsToFd. > > No reason, do you mean it should be a Vector<char*>? Yes, or even char** allocated with g_newa, but we don't need to duplicate all the strings > > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:193 > > > + if (!g_spawn_async(NULL, argv, NULL, G_SPAWN_LEAVE_DESCRIPTORS_OPEN, NULL, NULL, &m_pid, &error.outPtr())) { > > > > Use nullptr instead of NULL. Could we use GSubprocess API? since this patch > > is adding new deps, I think it's fine to also make the faue depend on a > > newer glib too. Why do we leave the descriptors open? > > Sure we could. Same FD comment as a above, the `bwrap` command passes an fd > to its child `xdg-dbus-proxy`. > > We could just not sandbox `xdg-dbus-proxy` and clean this up a bit. The > extra layer of sandboxing surely > can't hurt but its probably safe and `flatpak` doesn't do it. I still don't understand what xdg-dbus-proxy is for :-P > > >> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:208 > > >> + return nullptr; > > > > > > Omit the return, since it's unreachable and surely not needed to avoid warnings. That will just confuse developers about g_error(). > > > > Why are all the errors fatal in this patch? > > Michael asked for them to be. Really they can all be handled but the end > process should fail to start. > > > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:260 > > > +static void bindIfExists(Vector<CString>& args, const char* path, BindFlags bindFlags = BindFlags::ReadOnly) > > > > I also find this name confusing, I guess appendBindArguments or something > > like that would be more accurate, no? What should exist? we only check if > > path is nullptr or not > > ah, the if exists is because of the -try suffix? > > Yes. `bindIfPathExists()` sound better to you? addBindArgumentIfPathExists() > > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:287 > > > + proxy.launch(); > > > > So, this is the one launching the bwrap executable. Should this be > > launchWhateverIfNeeded instead? or ensureWhatever? > > `proxy.launchProxy()` ? I'm not sure the duplication is needed. I don't know, I don't understand how it works to decide on the API or name. > > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:413 > > > + static DBusProxy proxy; > > > > So, the proxy is only for a11y? > > There are 3 proxies per UI process. 1 for a11y and 2 for session bus for web > and network processes. > (At a later point another for system bus probably.) But there can be more than one web process. What happens when a process finishes or crashes? Theses proxies are all static. > > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:673 > > > + // NOTE: This is not a great solution but we just assume that applications create this directory > > > + // ahead of time if they require it > > > + GUniquePtr<char> configDir(g_build_filename(g_get_user_config_dir(), g_get_prgname(), nullptr)); > > > > Shouldn't we add API to allow applications pass their config/data dirs > > instead? > > Michael decided not to for now. Then, let's not add this for now either. > > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:810 > > > + Vector<CString> bwrapArgs = { > > > > I'm confused again, wasn't this done by the proxy launch method? > > We are launching multiple processes. So, we have different ways of launching the bwrap thing? > > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:858 > > > +#endif > > > > I think all the sandbox code should be moved to its own file > > Maybe, what do you think Michael? > > > > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:116 > > > +#if PLATFORM(GTK) || PLATFORM(WPE) > > > + WebsiteDataStore& store = processPool().websiteDataStore()->websiteDataStore(); > > > + store.resolveDirectoriesIfNecessary(); > > > + launchOptions.extraSandboxPaths.append(store.resolvedNetworkCacheDirectory()); > > > + > > > + launchOptions.sandboxEnabled = m_processPool.sandboxEnabled(); > > > +#endif > > > > I think we should add a platformGetLaunchOptions or something like that > > instead of adding platform ifdefs here. Should this depend on > > ENABLE(BUBBLEWRAP_SANDBOX) or OS(LINUX) instead of GTK|WPE? > > I don't know if `platformGetLaunchOptions()` would be much better. Could > change the ifdef though. Yes, it's always better than adding platform ifdefs in cross-platform files. > > > Source/cmake/WebKitFeatures.cmake:88 > > > + WEBKIT_OPTION_DEFINE(ENABLE_BUBBLEWRAP_SANDBOX "Toggle bubblewrap sandboxing support" PRIVATE OFF) > > > > Why is this private? > > Because it only supports Linux, why show it on non-Linux. Ah, it's public in linux then? > --- > > I addressed all other comments. Thanks! (In reply to Michael Catanzaro from comment #46) > Errors initializing the sandbox are ultimately errors in the UI process, so > it makes sense to me that the UI process should terminate. But I suppose > it's OK to not do that so long as the secondary process is guaranteed to not > start and the WebKitWebView emits the crashed/terminated signals. > > (In reply to Patrick Griffis from comment #45) > > > I think all the sandbox code should be moved to its own file > > > > Maybe, what do you think Michael? > > Whatever Carlos prefers. It's iffy. I considered requesting this myself, but > decided in the end that it was fine in ProcessLauncher because it's > intimately tied to the process launching. It's impossible to reason about or > debug process launching without reading the sandboxing code, and if we split > it into a separate file, the sandboxing code is ultimately just going to > return a bunch of args to pass to g_spawn_async(). So keeping it in > ProcessLauncher seemed fine to me. I think we could have a file with a BwrapLauncher class or something like that, used by the process launcher. > > > > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:116 > > > > +#if PLATFORM(GTK) || PLATFORM(WPE) > > > > + WebsiteDataStore& store = processPool().websiteDataStore()->websiteDataStore(); > > > > + store.resolveDirectoriesIfNecessary(); > > > > + launchOptions.extraSandboxPaths.append(store.resolvedNetworkCacheDirectory()); > > > > + > > > > + launchOptions.sandboxEnabled = m_processPool.sandboxEnabled(); > > > > +#endif > > > > > > I think we should add a platformGetLaunchOptions or something like that > > > instead of adding platform ifdefs here. Should this depend on > > > ENABLE(BUBBLEWRAP_SANDBOX) or OS(LINUX) instead of GTK|WPE? > > > > I don't know if `platformGetLaunchOptions()` would be much better. Could > > change the ifdef though. > > If it's easy to split the code into a platformGetLaunchOptions(), that would > be nice/idiomatic. > > P.S. I'd encourage you to test with asan ('set-webkit-configuration --debug > --asan') to catch those leaks you introduced. It's extremely difficult to > avoid introducing leaks if you're not developing with asan. Comment on attachment 349888 [details] [GTK][WPE] Implement subprocess sandboxing View in context: https://bugs.webkit.org/attachment.cgi?id=349888&action=review > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1158 > + * This method **must be called before any web process has been created**, > + * as early as possible in your application. Calling it later is a fatal error. Maybe we could make it a construct only property then, because it doesn't make sense to change it either. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:31 > +#include "PlatformDisplay.h" WebCore headers should be included as <WebCore/Header.h> from WebKit layer. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:66 > + return syscall(__NR_memfd_create, name, flags); Is this always available? doesn't it depend on a kernel version or libc? > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:139 > + int sync_fds[2] = { -1 }; sync_fds -> syncFds > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:197 > + if (!g_spawn_async(NULL, argv, NULL, G_SPAWN_LEAVE_DESCRIPTORS_OPEN, NULL, NULL, &pid, &error.outPtr())) Use nullptr instead of NULL. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:220 > + char* proxySocketTemplate = g_build_filename(appRunDir, "dbus-proxy-XXXXXX", nullptr); Use GUniquePtr here, and you don't need to free in case of early return > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:223 > + g_free(proxySocketTemplate); Like this one. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:293 > + const char* sessionAddress = g_getenv("DBUS_SESSION_BUS_ADDRESS"); > + if (!sessionAddress) > + return; > + > + GUniquePtr<char> sessionPath = dbusAddressToPath(sessionAddress); > + if (!sessionPath.get()) > + return; This is going to be the same for any received proxy. I think it would be simpler if DBusProxy class received only the address and the type and called dbusAddressToPath itself. DBusProxy::setAddress() instead of setSocket. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:296 > + proxy.setSocket(sessionAddress, sessionPath.get()); > + proxy.launch(); These always go together, so maybe launch could simply receive the address and type instead of set + launch. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:301 > + args.appendVector(Vector<CString>({ > + "--bind", proxy.proxyPath(), proxy.path(), > + })); So, I would leave this function to only add the argument, like all other bind methods, and the caller, who created the proxy can call launch(). > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:456 > + GUniquePtr<char> a11yPath = dbusAddressToPath(a11yAddress.get(), DBusAddressType::Abstract); > + if (!a11yAddress.get()) > + return; I would do the same here, I would move the code to get the address to its own function. > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:668 > +static std::tuple<Vector<CString>, std::optional<int>, std::optional<int>> getBwrapArgs(ProcessLauncher::ProcessType processType, const Vector<String>& extraPaths) We don't use get unless return values are out parameters. Also avoid using abbreviations in function and variables names in general. > Source/cmake/OptionsGTK.cmake:215 > + find_program(BWRAP_EXECUTABLE bwrap) > + if (NOT BWRAP_EXECUTABLE) > + message(FATAL_ERROR "bwrap executable is needed for ENABLE_BUBBLEWRAP_SANDBOX") > + endif () > + add_definitions(-DBWRAP_EXECUTABLE="${BWRAP_EXECUTABLE}") hmm, this executable is not actually a build dep, but a runtime one, no? Wouldn't it be possible to not have the executable at build time, for example when cross-compiling? (In reply to Carlos Garcia Campos from comment #47) > (In reply to Patrick Griffis from comment #45) > > (In reply to Carlos Garcia Campos from comment #40) > > > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:127 > > > > + void appendPermissions(std::initializer_list<CString> permissions) > > > > + { > > > > + RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(!m_isRunning); > > > > + > > > > + Vector<CString> newPermissions(permissions); > > > > > > Why doesn't this receive a Vector<CString> directly? > > > > I just found it cleaner to pass {"initializer", "lists"} around than create > > vectors. > > Can't you use { } if the function receives a Vector<>&& ? Last I tried no. > > Sure we could. Same FD comment as a above, the `bwrap` command passes an fd > > to its child `xdg-dbus-proxy`. > > > > We could just not sandbox `xdg-dbus-proxy` and clean this up a bit. The > > extra layer of sandboxing surely > > can't hurt but its probably safe and `flatpak` doesn't do it. > > I still don't understand what xdg-dbus-proxy is for :-P > > > > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:287 > > > > + proxy.launch(); > > > > > > So, this is the one launching the bwrap executable. Should this be > > > launchWhateverIfNeeded instead? or ensureWhatever? > > > > `proxy.launchProxy()` ? I'm not sure the duplication is needed. > > I don't know, I don't understand how it works to decide on the API or name. So normally you would just mount `$rundir/bus` in the sandbox for dbus to work but that means you have access to every dbus service on the host which is obviously bad. `xdg-dbus-proxy` opens a connection to the host bus, then creates a proxy bus which is put into the sandbox. The proxy allows you to filter which bus names, interfaces, and paths that are allowed through and are otherwise rewritten. > > > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:413 > > > > + static DBusProxy proxy; > > > > > > So, the proxy is only for a11y? > > > > There are 3 proxies per UI process. 1 for a11y and 2 for session bus for web > > and network processes. > > (At a later point another for system bus probably.) > > But there can be more than one web process. What happens when a process > finishes or crashes? Theses proxies are all static. The web processes all have the same permissions so always share the same proxy, I don't *think* that is a problem.(In reply to Carlos Garcia Campos from comment #49) > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:66 > > + return syscall(__NR_memfd_create, name, flags); > > Is this always available? doesn't it depend on a kernel version or libc? Linux 3.17 (~4 years ago) > > Source/cmake/OptionsGTK.cmake:215 > > + find_program(BWRAP_EXECUTABLE bwrap) > > + if (NOT BWRAP_EXECUTABLE) > > + message(FATAL_ERROR "bwrap executable is needed for ENABLE_BUBBLEWRAP_SANDBOX") > > + endif () > > + add_definitions(-DBWRAP_EXECUTABLE="${BWRAP_EXECUTABLE}") > > hmm, this executable is not actually a build dep, but a runtime one, no? > Wouldn't it be possible to not have the executable at build time, for > example when cross-compiling? Yes it is a runtime dep. The value of it being a build time check is we get an absolute path and we get to check the version (0.3.1 isn't released, so it would commonly fail atm). (In reply to Patrick Griffis from comment #50) > (In reply to Carlos Garcia Campos from comment #47) > > > > Can't you use { } if the function receives a Vector<>&& ? > > Last I tried no. > My mistake, yes Vector<>&& works. (In reply to Carlos Garcia Campos from comment #47) > so writeArgumentsToMemory() is what the function does, right? Certainly not. It's a file descriptor. I would say either writeArgumentsToFileDescriptor() or writeArgumentsToMemfd(). (In reply to Carlos Garcia Campos from comment #49) > The web processes all have the same permissions so always share the same > proxy, I don't *think* that is a problem. It's not a problem. We don't want a proxy process running for every web process. > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:66 > > + return syscall(__NR_memfd_create, name, flags); > > Is this always available? doesn't it depend on a kernel version or libc? It's added in glibc 2.27, which too recent. That's why it's better to call syscall() than to use it via glibc. It occurs to me that Red Hat will probably be running this code with the 3.10 kernel, but you handle that by calling g_error() if memfd_create() fails, which seems fine to me. People using ancient kernels should just disable the sandbox. (In reply to Patrick Griffis from comment #50) > Yes it is a runtime dep. The value of it being a build time check is we get > an absolute path and we get to check the version (0.3.1 isn't released, so > it would commonly fail atm). The risk is that we need to make sure cross-compiling works. I guess the check is fine, since bwrap will probably be installed in the same place on both host and target? (In reply to Michael Catanzaro from comment #52) > (In reply to Carlos Garcia Campos from comment #47) > > so writeArgumentsToMemory() is what the function does, right? > > Certainly not. It's a file descriptor. I would say either > writeArgumentsToFileDescriptor() or writeArgumentsToMemfd(). Arguments are written to memory and the file descriptor to read that memory is returned, or am I wrong? > (In reply to Carlos Garcia Campos from comment #49) > > The web processes all have the same permissions so always share the same > > proxy, I don't *think* that is a problem. > > It's not a problem. We don't want a proxy process running for every web > process. Ok, go it. Should we use NeverDestroyed then? > > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:66 > > > + return syscall(__NR_memfd_create, name, flags); > > > > Is this always available? doesn't it depend on a kernel version or libc? > > It's added in glibc 2.27, which too recent. That's why it's better to call > syscall() than to use it via glibc. But I guess __NR_memfd_create needs to be defined somewhere? > It occurs to me that Red Hat will probably be running this code with the > 3.10 kernel, but you handle that by calling g_error() if memfd_create() > fails, which seems fine to me. People using ancient kernels should just > disable the sandbox. > > (In reply to Patrick Griffis from comment #50) > > Yes it is a runtime dep. The value of it being a build time check is we get > > an absolute path and we get to check the version (0.3.1 isn't released, so > > it would commonly fail atm). > > The risk is that we need to make sure cross-compiling works. I guess the > check is fine, since bwrap will probably be installed in the same place on > both host and target? I don't know. (In reply to Carlos Garcia Campos from comment #47) > (In reply to Patrick Griffis from comment #45) > > (In reply to Carlos Garcia Campos from comment #40) > > > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:143 > > > > + Vector<CString> bwrapArgs = { > > > > > > Why are we using Vector<CString>? that means we are duplicating every string > > > (most of them are static or already owned by the class), and then copied > > > again in argsToFd. > > > > No reason, do you mean it should be a Vector<char*>? > > Yes, or even char** allocated with g_newa, but we don't need to duplicate > all the strings Nevermind, I see now that we have other cases where the string is local to a helper function, so I guess we have to copy them anyway. (In reply to Carlos Garcia Campos from comment #53) > > It's added in glibc 2.27, which too recent. That's why it's better to call > > syscall() than to use it via glibc. > > But I guess __NR_memfd_create needs to be defined somewhere? Good point. That's probably a good thing since it will lead to a build failure, rather than a runtime failure. We're surely not interested in supporting the sandbox in systems that lack __NR_memfd_create. (In reply to Carlos Garcia Campos from comment #53) > Ok, go it. Should we use NeverDestroyed then? No, because then the proxy subprocess will never be killed when the UI process quits. That said, good catch, we have exit-time destructors here still. It will need to be refactored to avoid that... somehow. (In reply to Michael Catanzaro from comment #55) > (In reply to Carlos Garcia Campos from comment #53) > > > It's added in glibc 2.27, which too recent. That's why it's better to call > > > syscall() than to use it via glibc. > > > > But I guess __NR_memfd_create needs to be defined somewhere? > > Good point. That's probably a good thing since it will lead to a build > failure, rather than a runtime failure. We're surely not interested in > supporting the sandbox in systems that lack __NR_memfd_create. Why can't we simply use SharedMemory? I think we could even have a SharedMemory impl based on memfd when available and falling back to shm_open otherwise. > (In reply to Carlos Garcia Campos from comment #53) > > Ok, go it. Should we use NeverDestroyed then? > > No, because then the proxy subprocess will never be killed when the UI > process quits. > > That said, good catch, we have exit-time destructors here still. It will > need to be refactored to avoid that... somehow. Shouldn't they be per web process pool instead of static to the UI process? (In reply to Carlos Garcia Campos from comment #56) > > (In reply to Carlos Garcia Campos from comment #53) > > > Ok, go it. Should we use NeverDestroyed then? > > > > No, because then the proxy subprocess will never be killed when the UI > > process quits. > > > > That said, good catch, we have exit-time destructors here still. It will > > need to be refactored to avoid that... somehow. > > Shouldn't they be per web process pool instead of static to the UI process? In practice I don't think it matters. The proxies would be the same so you just end up launching more than needed. The only benefit is if a pool with sandboxing was destroyed and then a new pool without sandboxing was made you would drop the unused processes. You have to get rid of the exit-time destructors somehow. This is one of my least-favorite things in C++. Tying the DBusProxy to the lifetime of the WebProcessPool seems like a good idea to me. Well, almost. That leads to disaster (proxy running forever) if a WebProcessPool is ever leaked. I think the default WebProcessPool is actually leaked in this way, right? So maybe we can't do that. We could use prctl(PR_SET_PDEATHSIG) somewhere to give some chance of shutting down the DBusProxy in that case, and we probably want it anyway for the event of a UI process crash. I think currently the secondary processes kill themselves when they detect I/O hangup. Presumably the D-Bus proxy needs something more? Try testing by killing the UI process and seeing if the proxy process hangs around. (In reply to Carlos Garcia Campos from comment #56) > Why can't we simply use SharedMemory? I think we could even have a > SharedMemory impl based on memfd when available and falling back to shm_open > otherwise. Because bubblewrap is specifically expecting a memfd. We can implement SharedMemory using memfd, but in this case we should avoid using that abstraction and rather explicitly write the code that bubblewrap expects. > > (In reply to Carlos Garcia Campos from comment #53) > Shouldn't they be per web process pool instead of static to the UI process? Good idea. (In reply to Michael Catanzaro from comment #58) > You have to get rid of the exit-time destructors somehow. This is one of my > least-favorite things in C++. Tying the DBusProxy to the lifetime of the > WebProcessPool seems like a good idea to me. > > Well, almost. That leads to disaster (proxy running forever) if a > WebProcessPool is ever leaked. I think the default WebProcessPool is > actually leaked in this way, right? So maybe we can't do that. Ok so it looks like Flatpak ties the lifetime of the proxy to a pipe between the processes, that would remove relying on destructors and just be more reliable in general. I'm not sure the cleanest way to tie that to the processpool though. (In reply to Michael Catanzaro from comment #60) > (In reply to Carlos Garcia Campos from comment #56) > > Why can't we simply use SharedMemory? I think we could even have a > > SharedMemory impl based on memfd when available and falling back to shm_open > > otherwise. > > Because bubblewrap is specifically expecting a memfd. We can implement > SharedMemory using memfd, but in this case we should avoid using that > abstraction and rather explicitly write the code that bubblewrap expects. Really? I think it just expects a file descriptor to read from, being memfd or not shouldn't make any different, or am I missing something? > > > (In reply to Carlos Garcia Campos from comment #53) > > Shouldn't they be per web process pool instead of static to the UI process? > > Good idea. (In reply to Carlos Garcia Campos from comment #62) > (In reply to Michael Catanzaro from comment #60) > > (In reply to Carlos Garcia Campos from comment #56) > > > Why can't we simply use SharedMemory? I think we could even have a > > > SharedMemory impl based on memfd when available and falling back to shm_open > > > otherwise. > > > > Because bubblewrap is specifically expecting a memfd. We can implement > > SharedMemory using memfd, but in this case we should avoid using that > > abstraction and rather explicitly write the code that bubblewrap expects. > > Really? I think it just expects a file descriptor to read from, being memfd > or not shouldn't make any different, or am I missing something? https://github.com/projectatomic/bubblewrap/blob/master/utils.c#L513 Comment on attachment 349888 [details]
[GTK][WPE] Implement subprocess sandboxing
Also noticed that this patch is adding new API without a test. I guess it's not easy to test this with a unit test? In any case, I think it would be a good idea to add a --enable-sandbox (or similar) to MiniBrowser to manually test this.
(In reply to Carlos Garcia Campos from comment #64) > Comment on attachment 349888 [details] > [GTK][WPE] Implement subprocess sandboxing > > Also noticed that this patch is adding new API without a test. I guess it's > not easy to test this with a unit test? In any case, I think it would be a > good idea to add a --enable-sandbox (or similar) to MiniBrowser to manually > test this. We have an env var `WEBKIT_ENABLE_SANDBOX=1` (probably should get a better name) for testing. Now that your infrastructure is moving to Flatpak it actually becomes impossible to automatically test there. Er, `WEBKIT_SANDBOX_ENABLED=1` BTW the set_sandbox_enabled() API parallels all the other functions of WebKitWebContext that must be called before the web process is started, and none of those are properties, so I don't think there's any need to add a construct-only sandbox-enabled property: I would just add the new setter function. (In reply to Carlos Garcia Campos from comment #64) > Comment on attachment 349888 [details] > [GTK][WPE] Implement subprocess sandboxing > > Also noticed that this patch is adding new API without a test. I guess it's > not easy to test this with a unit test? The test will be disabled on the bots anyway, since it can't run in flatpak. That's the big flaw in this plan. We can't test anything under the sandbox if we're switching the infrastructure to flatpak. > In any case, I think it would be a > good idea to add a --enable-sandbox (or similar) to MiniBrowser to manually > test this. We could add it to minibrowser, but remember it won't work via run-minibrowser, because that will use flatpak. The envvar might be enough for MiniBrowser. (In reply to Patrick Griffis from comment #61) > Ok so it looks like Flatpak ties the lifetime of the proxy to a pipe between > the processes, that would remove relying on destructors and just be more > reliable in general. > > I'm not sure the cleanest way to tie that to the processpool though. It doesn't need to be tied to the process pool. Just make sure it doesn't linger after the UI process quits. The sandbox also probably won't work inside any form of container, not just flatpak. So it really needs to be disabled for testing regardless. :/ Created attachment 350383 [details]
[GTK][WPE] Implement subprocess sandboxing
I believe I've addressed most comments. I did test using SharedMemory but it didn't seem to like the shm fd but I didn't look into why. Created attachment 350385 [details]
[GTK][WPE] Implement subprocess sandboxing
Every EWS is red (In reply to Patrick Griffis from comment #70) > I believe I've addressed most comments. > > I did test using SharedMemory but it didn't seem to like the shm fd but I > didn't look into why. Could you elaborate? what's exactly the issue? How did you use SharedMemory? (In reply to Michael Catanzaro from comment #67) > BTW the set_sandbox_enabled() API parallels all the other functions of > WebKitWebContext that must be called before the web process is started, and > none of those are properties, so I don't think there's any need to add a > construct-only sandbox-enabled property: I would just add the new setter > function. Most of those functions where added when we only had the default context (it's not possible to create the default web context explicitly). I agree we need the setter if we want to allow sandboxing on the default web context. > (In reply to Carlos Garcia Campos from comment #64) > > Comment on attachment 349888 [details] > > [GTK][WPE] Implement subprocess sandboxing > > > > Also noticed that this patch is adding new API without a test. I guess it's > > not easy to test this with a unit test? > > The test will be disabled on the bots anyway, since it can't run in flatpak. > That's the big flaw in this plan. We can't test anything under the sandbox > if we're switching the infrastructure to flatpak. > > > In any case, I think it would be a > > good idea to add a --enable-sandbox (or similar) to MiniBrowser to manually > > test this. > > We could add it to minibrowser, but remember it won't work via > run-minibrowser, because that will use flatpak. The envvar might be enough > for MiniBrowser. > > (In reply to Patrick Griffis from comment #61) > > Ok so it looks like Flatpak ties the lifetime of the proxy to a pipe between > > the processes, that would remove relying on destructors and just be more > > reliable in general. > > > > I'm not sure the cleanest way to tie that to the processpool though. > > It doesn't need to be tied to the process pool. Just make sure it doesn't > linger after the UI process quits. (In reply to Carlos Garcia Campos from comment #74) > Most of those functions where added when we only had the default context > (it's not possible to create the default web context explicitly). I agree we > need the setter if we want to allow sandboxing on the default web context. Let's add just the setter for now. If we want to add properties for all of these, we can do them all together in a future patch. It's not essential here. Comment on attachment 350385 [details]
[GTK][WPE] Implement subprocess sandboxing
r- due to broken builds
(In reply to Carlos Garcia Campos from comment #73) > Could you elaborate? what's exactly the issue? How did you use SharedMemory? It was a bit ugly but something like (out of memory not actual code): auto sharedMemory = SharedMemory.create(data, size, SharedMemory::Protection::ReadOnly); SharedMemory::Handle handle; sharedMemory.createHandle(handle, SharedMemory::Protection::ReadOnly); int fd = handle.releaseAttachment().releaseFileDescriptor(); Upon usage it was just a vague error like `invalid fd`. This also was within the same scope as the `spawn()` call so I don't think anything was destroyed. Created attachment 350637 [details]
[GTK][WPE] Implement subprocess sandboxing
Created attachment 350638 [details]
[GTK][WPE] Implement subprocess sandboxing
Alex, Youenn, could you please review the bits that require an owner? All the big diffs are in platform-specific code, but there are many small diffs here in cross-platform files. Created attachment 350654 [details]
[GTK][WPE] Implement subprocess sandboxing
Created attachment 350655 [details]
[GTK][WPE] Implement subprocess sandboxing
Created attachment 350657 [details]
[GTK][WPE] Implement subprocess sandboxing
Created attachment 350781 [details]
[GTK][WPE] Implement subprocess sandboxing
Comment on attachment 350781 [details] [GTK][WPE] Implement subprocess sandboxing View in context: https://bugs.webkit.org/attachment.cgi?id=350781&action=review > Source/WebKit/UIProcess/Launcher/ProcessLauncher.h:74 > +#endif Why not using extraInitializationData instead? > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:158 > + const String& resolvedLocalStorageDirectory() const { return m_resolvedConfiguration.localStorageDirectory; } resolvedLocalStorageDirectory() does not seem used anywhere. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:159 > + Extra line. > Source/WebKit/UIProcess/glib/NetworkProcessProxyGLib.cpp:35 > + launchOptions.extraSandboxPaths.append(store.resolvedNetworkCacheDirectory()); Do you need this at the moment of creating the process? Or can the UIProcess pass a sandbox extension to the NetworkProcess at a later point? One potential issue is that the NetworkProcess might need to handle several sessions, hence several WebsiteDataStores, hence several network cache directories. > Source/WebKit/UIProcess/glib/StorageProcessProxyGLib.cpp:35 > + WebsiteDataStore& store = m_processPool.websiteDataStore()->websiteDataStore(); s/WebsiteDataStore/auto/ Comment on attachment 350781 [details] [GTK][WPE] Implement subprocess sandboxing View in context: https://bugs.webkit.org/attachment.cgi?id=350781&action=review Sorry, I don't think I'm an appropriate reviewer for this patch :-/ > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:117 > + extra line here too Created attachment 351231 [details]
Patch
(In reply to youenn fablet from comment #85) > Comment on attachment 350781 [details] > [GTK][WPE] Implement subprocess sandboxing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=350781&action=review > > > Source/WebKit/UIProcess/Launcher/ProcessLauncher.h:74 > > +#endif > > Why not using extraInitializationData instead? Well we need a list of strings and a bool, probably wouldn't want to shove those in a string. > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:158 > > + const String& resolvedLocalStorageDirectory() const { return m_resolvedConfiguration.localStorageDirectory; } > > resolvedLocalStorageDirectory() does not seem used anywhere. Fixed. > > > Source/WebKit/UIProcess/glib/NetworkProcessProxyGLib.cpp:35 > > + launchOptions.extraSandboxPaths.append(store.resolvedNetworkCacheDirectory()); > > Do you need this at the moment of creating the process? > Or can the UIProcess pass a sandbox extension to the NetworkProcess at a > later point? > > One potential issue is that the NetworkProcess might need to handle several > sessions, hence several WebsiteDataStores, hence several network cache > directories. We discussed this a bit but yes all paths are required at launch time and cannot be modified after. For now we will just assert everything needed is set up ahead of time. Created attachment 351370 [details]
Patch
(In reply to Patrick Griffis from comment #88) > (In reply to youenn fablet from comment #85) > > Comment on attachment 350781 [details] > > [GTK][WPE] Implement subprocess sandboxing > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=350781&action=review > > > > > Source/WebKit/UIProcess/Launcher/ProcessLauncher.h:74 > > > +#endif > > > > Why not using extraInitializationData instead? > > Well we need a list of strings and a bool, probably wouldn't want to shove > those in a string. extraInitializationData is a HashMap<String, String> so this seems to be good enough for your use case. Since we have one way of doing things, we should probably either use it or refactor it if needed. Comment on attachment 351370 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351370&action=review > Source/WebKit/UIProcess/glib/NetworkProcessProxyGLib.cpp:33 > + WebsiteDataStore& store = processPool().websiteDataStore()->websiteDataStore(); s/WebsiteDataStore/auto/ or remove the local variable since it is used only once. > Source/WebKit/UIProcess/glib/NetworkProcessProxyGLib.cpp:34 > + store.resolveDirectoriesIfNecessary(); Not sure this call is needed either. > Source/WebKit/UIProcess/glib/StorageProcessProxyGLib.cpp:31 > +void StorageProcessProxy::platformGetLaunchOptions(ProcessLauncher::LaunchOptions& launchOptions) StorageProcess is now gone so no need to handle that. Instead, store.resolvedIndexedDatabaseDirectory and store.resolvedServiceWorkerRegistrationDirectory should be passed to NetworkProcessProxy. > Source/WebKit/UIProcess/glib/StorageProcessProxyGLib.cpp:39 > + launchOptions.extraSandboxPaths.append(store.resolvedServiceWorkerRegistrationDirectory()); extraSandboxPaths does not really state that this is for read/write access to the filesystem. It would be nice to make it clear here. grantedReadWriteDirectories maybe? The alternative would be to use extraInitializationData this way: extraInitializationData.add("indexedDatabaseDirectory"_s, store.resolvedIndexedDatabaseDirectory()); extraInitializationData.add("localStorageDirectory"_s, store.resolvedLocalStorageDirectory()); I slightly prefer this approach :) compared to a GTK specific Vector<String>. > Source/WebKit/UIProcess/glib/WebProcessProxyGLib.cpp:44 > + websiteDataStore().resolveDirectoriesIfNecessary(); Is resolveDirectoriesIfNecessary call needed? Shouldn't already be done by WebProcessPool? Patch does not apply probably due to Storage Process removal. Comment on attachment 351370 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351370&action=review > Source/WebKit/UIProcess/Launcher/ProcessLauncher.h:74 > +#if PLATFORM(GTK) || PLATFORM(WPE) > + Vector<String> extraSandboxPaths; > + bool sandboxEnabled { false }; > +#endif Ideally we would reuse extraInitializationData, but that can be addressed later. > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:103 > + m_permissions = permissions; Use WTFMove() here. > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:131 > + proxyArgs.appendVector(m_permissions); > + > + > + int proxyFd = argsToFd(proxyArgs, "dbus-proxy"); Extra vertical whitespace. > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:181 > + char* proxySocketTemplate = g_build_filename(appRunDir, "dbus-proxy-XXXXXX", nullptr); Can this filename already be wrapped in GUniquePtr<char> here? Comment on attachment 351370 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351370&action=review >> Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:181 >> + char* proxySocketTemplate = g_build_filename(appRunDir, "dbus-proxy-XXXXXX", nullptr); > > Can this filename already be wrapped in GUniquePtr<char> here? Yes, better to declare it properly here than wrap it later at the last minute. Created attachment 352043 [details]
[GTK][WPE] Implement subprocess sandboxing
(In reply to youenn fablet from comment #91) > The alternative would be to use extraInitializationData this way: > extraInitializationData.add("indexedDatabaseDirectory"_s, > store.resolvedIndexedDatabaseDirectory()); > extraInitializationData.add("localStorageDirectory"_s, > store.resolvedLocalStorageDirectory()); > I slightly prefer this approach :) compared to a GTK specific Vector<String>. Done. > > Source/WebKit/UIProcess/glib/WebProcessProxyGLib.cpp:44 > > + websiteDataStore().resolveDirectoriesIfNecessary(); > > Is resolveDirectoriesIfNecessary call needed? Shouldn't already be done by > WebProcessPool? In my testing yes it was required: before resolve: "" after resolve: "/home/tingping/.cache/epiphany/applications" I'm going to commit this on Monday. It will break all GTK bots because the dependencies have not been installed. I requested this several weeks ago, so I think we just need to commit and break things to get it fixed. BTW the jsc-armv7 and wincairo failures are unrelated. I've pinged our infrastructure people one last time to remind them to install deps on the bots. LGTM too. Great to see that happening! Don't roll this out due to GTK/WPE bot breakage. Fix the bots instead. (If it unexpectedly breaks other bots, then a rollout would be OK.) Committed r237107: <https://trac.webkit.org/changeset/237107> (In reply to Michael Catanzaro from comment #100) > Don't roll this out due to GTK/WPE bot breakage. Fix the bots instead. > > (If it unexpectedly breaks other bots, then a rollout would be OK.) Likely, I can take a look at the bots between tomorrow and Wednesday, if nobody does before. |