[GTK][WPE] Add API to add paths to sandbox
Created attachment 359482 [details] Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Created attachment 359484 [details] Patch
Comment on attachment 359484 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359484&action=review Informally reviewing... Patch overall looks good, there's only one change I would request before landing it. > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1195 > + g_error("Sandbox paths cannot be changed after subprocesses were spawned."); If it is an error to add paths after a subprocess was spawned, I think we want a an early “return” here to avoid calling “appendSandboxPaths()” below to modify the list when there are already processes running -- otherwise the processes spawned afterwards will have a different set of paths applied to them.
(In reply to Adrian Perez from comment #4) > If it is an error to add paths after a subprocess was spawned, I think we > want a an early “return” here to avoid calling “appendSandboxPaths()” below > to modify the list when there are already processes running -- otherwise the > processes spawned afterwards will have a different set of paths applied to > them. Well `g_error()` is fatal so that won't ever happen. I don't have a strong opinion of making it not fatal but it matches the other function.
Comment on attachment 359484 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359484&action=review >> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1195 >> + g_error("Sandbox paths cannot be changed after subprocesses were spawned."); > > If it is an error to add paths after a subprocess was spawned, I think we > want a an early “return” here to avoid calling “appendSandboxPaths()” below > to modify the list when there are already processes running -- otherwise the > processes spawned afterwards will have a different set of paths applied to > them. g_error() is fatal: the ultimate early return!
(In reply to Patrick Griffis from comment #5) > (In reply to Adrian Perez from comment #4) > > If it is an error to add paths after a subprocess was spawned, I think we > > want a an early “return” here to avoid calling “appendSandboxPaths()” below > > to modify the list when there are already processes running -- otherwise the > > processes spawned afterwards will have a different set of paths applied to > > them. > > Well `g_error()` is fatal so that won't ever happen. I don't have a strong > opinion of making it not fatal but it matches the other function. True, I keep forgetting this one! r+!
Comment on attachment 359484 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359484&action=review Good! > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1182 > + * Adds a path to be mounted in the sandbox. @path must exist before any web process > + * has been created otherwise it will be silently ignored. It is a fatal error to Maybe we should attempt a g_mkdir_with_parents(), so we don't have to worry about this if the directory is writable? Bad idea? We could add a flags parameter to control the behavior? > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1189 > +void webkit_web_context_add_path_to_sandbox(WebKitWebContext* context, const char* path, gboolean read_only) Add an enum. Join the inquisition against boolean parameters! > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1197 > + context->priv->processPool->appendSandboxPaths(String(path), read_only); I think you can just pass path and let it be implicitly be converted toString. > Source/WebKit/UIProcess/WebProcessPool.h:474 > + void appendSandboxPaths(const String& path, bool readOnly) { m_extraSandboxPaths.set(path, readOnly); }; bool isn't OK here either; there should be a different new enum (since this is below the API layer and can't use the new enum you'll add to the API). There really is an inquisition, you know. Be the change you want to see in function signatures! > Source/WebKit/UIProcess/WebProcessPool.h:733 > + HashMap<String, bool> m_extraSandboxPaths; Then you can use the new enum here too. The code really will be more readable. > Source/WebKit/UIProcess/glib/WebProcessProxyGLib.cpp:56 > + String extraPaths; > + for (const auto& entry : m_processPool->sandboxPaths()) { > + if (!extraPaths.isEmpty()) > + extraPaths.append('\0'); > + extraPaths.append(entry.key); > + extraPaths.append(entry.value ? ":ro" : ":rw"); > + } > + > + launchOptions.extraInitializationData.add("extra-sandbox-paths", extraPaths); Why are you doing this instead of just adding a second HashMap to struct LaunchOptions? Would be much simpler, right?
(In reply to Michael Catanzaro from comment #8) > Comment on attachment 359484 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=359484&action=review > > Good! > > > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1182 > > + * Adds a path to be mounted in the sandbox. @path must exist before any web process > > + * has been created otherwise it will be silently ignored. It is a fatal error to > > Maybe we should attempt a g_mkdir_with_parents(), so we don't have to worry > about this if the directory is writable? Bad idea? > > We could add a flags parameter to control the behavior? Well its blocking IO... > > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1189 > > +void webkit_web_context_add_path_to_sandbox(WebKitWebContext* context, const char* path, gboolean read_only) > > Add an enum. Join the inquisition against boolean parameters! > > > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1197 > > + context->priv->processPool->appendSandboxPaths(String(path), read_only); > > I think you can just pass path and let it be implicitly be converted > toString. > > > Source/WebKit/UIProcess/WebProcessPool.h:474 > > + void appendSandboxPaths(const String& path, bool readOnly) { m_extraSandboxPaths.set(path, readOnly); }; > > bool isn't OK here either; there should be a different new enum (since this > is below the API layer and can't use the new enum you'll add to the API). > > There really is an inquisition, you know. Be the change you want to see in > function signatures! > > > Source/WebKit/UIProcess/WebProcessPool.h:733 > > + HashMap<String, bool> m_extraSandboxPaths; > > Then you can use the new enum here too. The code really will be more > readable. Alright, enums. > > Source/WebKit/UIProcess/glib/WebProcessProxyGLib.cpp:56 > > + String extraPaths; > > + for (const auto& entry : m_processPool->sandboxPaths()) { > > + if (!extraPaths.isEmpty()) > > + extraPaths.append('\0'); > > + extraPaths.append(entry.key); > > + extraPaths.append(entry.value ? ":ro" : ":rw"); > > + } > > + > > + launchOptions.extraInitializationData.add("extra-sandbox-paths", extraPaths); > > Why are you doing this instead of just adding a second HashMap to struct > LaunchOptions? Would be much simpler, right? Because Youenn was very strongly against adding things to LaunchOptions last review since an extra data field already existed... I just avoided touching it from the start but I agree just adding it there would be much cleaner.
(In reply to Patrick Griffis from comment #9) > > Maybe we should attempt a g_mkdir_with_parents(), so we don't have to worry > > about this if the directory is writable? Bad idea? > > > > We could add a flags parameter to control the behavior? > > Well its blocking IO... Before the web context is initialized, though. This is basically the only time it's OK! I just wonder whether applications will be more annoyed that we create directories for them, or more annoyed when they break because the directory just happens to not exist early enough. > > Why are you doing this instead of just adding a second HashMap to struct > > LaunchOptions? Would be much simpler, right? > > Because Youenn was very strongly against adding things to LaunchOptions last > review since an extra data field already existed... I just avoided touching > it from the start but I agree just adding it there would be much cleaner. Oh, I forgot about that, sorry. Adding platform-specific members to cross-platform structures is good to avoid, but we do it when needed. Here it really makes sense, because the alternative you've implemented -- serializing what should be a HashMap into a string and then manually deserializing it -- isn't great. Youenn? (Anyway, if that's what Youenn wants, it's what we'll do.)
(In reply to Michael Catanzaro from comment #10) > (In reply to Patrick Griffis from comment #9) > > > Maybe we should attempt a g_mkdir_with_parents(), so we don't have to worry > > > about this if the directory is writable? Bad idea? > > > > > > We could add a flags parameter to control the behavior? > > > > Well its blocking IO... > > Before the web context is initialized, though. This is basically the only > time it's OK! I just wonder whether applications will be more annoyed that > we create directories for them, or more annoyed when they break because the > directory just happens to not exist early enough. > I hate blocking IO as much as you hate bool args :P. I think its never OK and we can't control when an application uses this and if its not during the initial window creation it will be noticeable and awful. If its acceptable to block they can do it themselves.
Comment on attachment 359484 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359484&action=review >>> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1182 >>> + * has been created otherwise it will be silently ignored. It is a fatal error to >> >> Maybe we should attempt a g_mkdir_with_parents(), so we don't have to worry about this if the directory is writable? Bad idea? >> >> We could add a flags parameter to control the behavior? > > Well its blocking IO... OK, no blocking I/O! > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:763 > + // Extra paths are stored as a single string and need to be manually split > + // They are in the format <path>:<rw|ro>\0 (see WebProcessProxyGLib.cpp) Well here is how we'll make a final determination about sandbox paths. Paths can contain < and > and : and | and even embedded \0 characters. This is a good argument for adding an #ifdef in the LaunchOptions. Also, be sure to pass the path through WebCore::Filesystem::stringFromFilesystemRepresentation. Programming is hard; too many things you have to remember. :S
(In reply to Michael Catanzaro from comment #12) > Also, be sure to > pass the path through > WebCore::Filesystem::stringFromFilesystemRepresentation. I'm not even sure our implementation of that function is right, but best at least use it where we're supposed to. :)
(In reply to Michael Catanzaro from comment #12) > Well here is how we'll make a final determination about sandbox paths. Paths > can contain < and > and : and | and even embedded \0 characters. I guess my description wasn't great. The only assumption I made was no \0. > Also, be sure to > pass the path through > WebCore::Filesystem::stringFromFilesystemRepresentation. We can convert it back and forth for cleanliness but the end usage we still want fs encoding.
Created attachment 359754 [details] Patch
Comment on attachment 359754 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359754&action=review > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1182 > + * has been created otherwise it will be silently ignored. It is a fatal error to created; otherwise, > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1189 > +void webkit_web_context_add_path_to_sandbox(WebKitWebContext* context, const char* path, gboolean read_only) Please add an enum or the public API, too. WebKitSandboxPermission or something like that. > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:763 > + const CString& path = WebCore::FileSystem::fileSystemRepresentation(pathAndPermission.key); It returns a copy, not a reference, so: CString path = ...; (Seeing the reference had me checking the function to figure out how it could possibly work safely.)
Also, since the application now gets to choose which paths get whitelisted, we should g_warning() if the path doesn't exist to let the application know about the bug. That can be done in the web process to avoid sync UI process API.
(In reply to Michael Catanzaro from comment #16) > Comment on attachment 359754 [details] > > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1189 > > +void webkit_web_context_add_path_to_sandbox(WebKitWebContext* context, const char* path, gboolean read_only) > > Please add an enum or the public API, too. WebKitSandboxPermission or > something like that. Feels very not GObjecty. (In reply to Michael Catanzaro from comment #17) > Also, since the application now gets to choose which paths get whitelisted, > we should g_warning() if the path doesn't exist to let the application know > about the bug. That can be done in the web process to avoid sync UI process > API. We don't actually know this since it failing or not happens in `bwrap`. We can check if it exists in the sandbox after everything launched but I dunno, feels like a lot of work for a warning.
OK, please ask Carlos Garcia to do final API review.
Created attachment 359878 [details] Patch
Comment on attachment 359878 [details] Patch Attachment 359878 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/10858299 New failing tests: imported/w3c/web-platform-tests/css/css-grid/abspos/orthogonal-positioned-grid-descendants-015.html
Created attachment 359889 [details] Archive of layout-test-results from ews104 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
(In reply to Michael Catanzaro from comment #19) > OK, please ask Carlos Garcia to do final API review. By the way: r=me. It just needs the second review now.
Comment on attachment 359878 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359878&action=review > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1189 > +void webkit_web_context_add_path_to_sandbox(WebKitWebContext* context, const char* path, gboolean read_only) read_only -> readOnly > Source/WebKit/UIProcess/Launcher/ProcessLauncher.h:49 > +#if PLATFORM(GTK) || PLATFORM(WPE) > +enum class SandboxPermission { > + ReadOnly, > + ReadWrite, > +}; > +#endif I think this belongs to WebProcessPool.h, where the api is defined. > Source/WebKit/UIProcess/Launcher/ProcessLauncher.h:79 > + HashMap<String, SandboxPermission> extraSandboxPaths; This could be a ref to avoid copying the map. > Source/WebKit/UIProcess/WebProcessPool.h:474 > + void appendSandboxPaths(const String& path, SandboxPermission permission) { m_extraSandboxPaths.set(path, permission); }; Why Paths? it takes only one path. > Source/WebKit/UIProcess/WebProcessPool.h:475 > + HashMap<String, SandboxPermission> sandboxPaths() { return m_extraSandboxPaths; }; This should const and return a const reference. > Source/WebKit/UIProcess/glib/WebProcessProxyGLib.cpp:47 > + launchOptions.extraSandboxPaths = m_processPool->sandboxPaths(); Could we make the Vector<CString> here instead of passing the map to the launcher? That way we don't copy the map for sure.
Created attachment 360011 [details] Patch
(In reply to Carlos Garcia Campos from comment #24) > Comment on attachment 359878 [details] > > Source/WebKit/UIProcess/Launcher/ProcessLauncher.h:49 > > +#if PLATFORM(GTK) || PLATFORM(WPE) > > +enum class SandboxPermission { > > + ReadOnly, > > + ReadWrite, > > +}; > > +#endif > > I think this belongs to WebProcessPool.h, where the api is defined. I avoided this since it complicated header include order. I think its fine since ProcessLauncher is the end API imo. > > Source/WebKit/UIProcess/glib/WebProcessProxyGLib.cpp:47 > > + launchOptions.extraSandboxPaths = m_processPool->sandboxPaths(); > > Could we make the Vector<CString> here instead of passing the map to the > launcher? That way we don't copy the map for sure. Seems like a strange point to do it. The contents of the args are very backend specific (we do technically have two Flatpak and Bubblewrap).
Created attachment 360012 [details] Patch
Comment on attachment 360011 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=360011&action=review > Source/WebKit/ChangeLog:4 > + https://bugs.webkit.org/show_bug.cgi?id=193571 Can you describe the use cases for this API? Which folders are to be sandboxed? > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:763 > + sandboxArgs.appendVector(Vector<CString>({ Do you need "({" or just one of these. > Source/WebKit/UIProcess/WebProcessPool.h:474 > + void appendSandboxPath(const CString& path, SandboxPermission permission) { m_extraSandboxPaths.set(path, permission); }; One usually append to a vector, not a map. I would rename it to addSandboxPath and use 'add' instead of 'set' which is slightly cheaper. > Source/WebKit/UIProcess/WebProcessPool.h:475 > + const HashMap<CString, SandboxPermission>& sandboxPaths() { return m_extraSandboxPaths; }; Should be a const method. > Source/WebKit/UIProcess/glib/WebProcessProxyGLib.cpp:47 > + launchOptions.extraSandboxPaths = m_processPool->sandboxPaths(); The extra sandbox paths seem specific to web processes. Should the name be made explicit with that regards?
(In reply to youenn fablet from comment #28) > Comment on attachment 360011 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=360011&action=review > > > Source/WebKit/ChangeLog:4 > > + https://bugs.webkit.org/show_bug.cgi?id=193571 > > Can you describe the use cases for this API? > Which folders are to be sandboxed? This exposes a public api for users to add custom paths which could be anything. > > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:763 > > + sandboxArgs.appendVector(Vector<CString>({ > > Do you need "({" or just one of these. Seems to require both. > > Source/WebKit/UIProcess/glib/WebProcessProxyGLib.cpp:47 > > + launchOptions.extraSandboxPaths = m_processPool->sandboxPaths(); > > The extra sandbox paths seem specific to web processes. > Should the name be made explicit with that regards? Sure what would be the convention for that?
(In reply to Patrick Griffis from comment #29) > > > Source/WebKit/UIProcess/glib/WebProcessProxyGLib.cpp:47 > > > + launchOptions.extraSandboxPaths = m_processPool->sandboxPaths(); > > > > The extra sandbox paths seem specific to web processes. > > Should the name be made explicit with that regards? > > Sure what would be the convention for that? Nevermind, misread that as should be named platform specific.
Created attachment 360018 [details] Patch
(In reply to Patrick Griffis from comment #29) > (In reply to youenn fablet from comment #28) > > Comment on attachment 360011 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=360011&action=review > > > > > Source/WebKit/ChangeLog:4 > > > + https://bugs.webkit.org/show_bug.cgi?id=193571 > > > > Can you describe the use cases for this API? > > Which folders are to be sandboxed? > > This exposes a public api for users to add custom paths which could be > anything. Can you describe what would be this 'anything'? I would expect WebKit to already know several paths that the WebProcess will use. For instance, you might know the exact service worker registration directories beforehand. Should it be used for these? There are shortcomings of course as Mac/iOS ports often use sandbox extensions. That leads to the question whether this API is a temporary solution to ease migrating apps to being sandboxed or if it will have more permanent usage.
(In reply to youenn fablet from comment #32) > (In reply to Patrick Griffis from comment #29) > > (In reply to youenn fablet from comment #28) > > > Comment on attachment 360011 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=360011&action=review > > > > > > > Source/WebKit/ChangeLog:4 > > > > + https://bugs.webkit.org/show_bug.cgi?id=193571 > > > > > > Can you describe the use cases for this API? > > > Which folders are to be sandboxed? > > > > This exposes a public api for users to add custom paths which could be > > anything. > > Can you describe what would be this 'anything'? > > I would expect WebKit to already know several paths that the WebProcess will > use. > For instance, you might know the exact service worker registration > directories beforehand. > Should it be used for these? > There are shortcomings of course as Mac/iOS ports often use sandbox > extensions. > > That leads to the question whether this API is a temporary solution to ease > migrating apps to being sandboxed or if it will have more permanent usage. As long as WebExtensions exist which run arbitrary user provided code inside the sandbox applications will want to add custom paths to the sandbox for various functionality. A simple example is Epiphany stores adblock data that the web process has to read. This data does not fall under any normal WebKit directories.
(In reply to Patrick Griffis from comment #26) > (In reply to Carlos Garcia Campos from comment #24) > > Comment on attachment 359878 [details] > > > Source/WebKit/UIProcess/Launcher/ProcessLauncher.h:49 > > > +#if PLATFORM(GTK) || PLATFORM(WPE) > > > +enum class SandboxPermission { > > > + ReadOnly, > > > + ReadWrite, > > > +}; > > > +#endif > > > > I think this belongs to WebProcessPool.h, where the api is defined. > > I avoided this since it complicated header include order. I think its fine > since ProcessLauncher is the end API imo. It's ok if it simplifies things, I guess. > > > Source/WebKit/UIProcess/glib/WebProcessProxyGLib.cpp:47 > > > + launchOptions.extraSandboxPaths = m_processPool->sandboxPaths(); > > > > Could we make the Vector<CString> here instead of passing the map to the > > launcher? That way we don't copy the map for sure. > > Seems like a strange point to do it. The contents of the args are very > backend specific (we do technically have two Flatpak and Bubblewrap). Yes, you are right, what happens with these paths when flatpak launcher is used then?
(In reply to Carlos Garcia Campos from comment #34) > > Seems like a strange point to do it. The contents of the args are very > > backend specific (we do technically have two Flatpak and Bubblewrap). > > Yes, you are right, what happens with these paths when flatpak launcher is > used then? Well currently nothing; Flatpak needs some work.
(In reply to Patrick Griffis from comment #35) > (In reply to Carlos Garcia Campos from comment #34) > > > Seems like a strange point to do it. The contents of the args are very > > > backend specific (we do technically have two Flatpak and Bubblewrap). > > > > Yes, you are right, what happens with these paths when flatpak launcher is > > used then? > > Well currently nothing; Flatpak needs some work. We should ensure it works in all the cases before adding the new API I would say.
(In reply to Carlos Garcia Campos from comment #36) > (In reply to Patrick Griffis from comment #35) > > Well currently nothing; Flatpak needs some work. > > We should ensure it works in all the cases before adding the new API I would > say. It works because the Flatpak backend does not currently limit directory access.
Comment on attachment 360018 [details] Patch I think all arguments are addressed, now?
(In reply to youenn fablet from comment #32) > That leads to the question whether this API is a temporary solution to ease > migrating apps to being sandboxed or if it will have more permanent usage. It's permanent. Currently our sandbox strictly limits the paths that are allowed: anything under the standard XDG data dirs/prgname, e.g.: ~/.cache/epiphany ~/.config/epiphany ~/.local/share/epiphany which is simultaneously too restrictive AND too permissive. E.g. in Epiphany we need to be able to whitelist temporary profiles under /tmp, which look like: /tmp/epiphany-mcatanzaro-fPO5Qw The only way to make that work currently would be for WebKit to unconditionally whitelist all of /tmp, which is no good, or for Epiphany to move them under /tmp/epiphany and then WebKit could whitelist /tmp/prgname, which is still too permissive because then unrelated Epiphany web processes will be able to see each others' data. So we can't really enable the sandbox until we have a fine-grained way to specify what paths exactly should be mounted inside the sandbox. With this API, mounting just /tmp/epiphany-mcatanzaro-fPO5Qw is no problem. And we can segregate profile data under ~/.cache and ~/.local/share such that web processes of separate instances can't see each others' data.
Comment on attachment 360018 [details] Patch Clearing flags on attachment: 360018 Committed r240473: <https://trac.webkit.org/changeset/240473>
All reviewed patches have been landed. Closing bug.
> As long as WebExtensions exist which run arbitrary user provided code inside > the sandbox applications will want to add custom paths to the sandbox for > various functionality. A simple example is Epiphany stores adblock data that > the web process has to read. This data does not fall under any normal WebKit > directories. In that particular case, the UIProcess could open this adblock data file and pass the file handle to the WebProcess. The WebProcess could then read the content (or if not feasible, the content would be sent through IPC either raw or processed). Given JavaScript is run in WebProcess, I am not sure it is safe for arbitrary user provided code to run in it and provide priviledges to enable it to run. (In reply to Michael Catanzaro from comment #39) > (In reply to youenn fablet from comment #32) > > That leads to the question whether this API is a temporary solution to ease > > migrating apps to being sandboxed or if it will have more permanent usage. > > It's permanent. Currently our sandbox strictly limits the paths that are > allowed: anything under the standard XDG data dirs/prgname, e.g.: > > ~/.cache/epiphany > ~/.config/epiphany > ~/.local/share/epiphany > > which is simultaneously too restrictive AND too permissive. E.g. in Epiphany > we need to be able to whitelist temporary profiles under /tmp, which look > like: > > /tmp/epiphany-mcatanzaro-fPO5Qw Another approach would be to make WebKit aware of these code paths. Something like, please use "/tmp/epiphany-mcatanzaro-fPO5Qw" as temporary folder for that browsing session. Then, WebKit would whitelist this folder at process launch time. As an added bonus, WebKit could for instance monitor the size of "/tmp/epiphany-mcatanzaro-fPO5Qw", clear the folder when appropriate...
(In reply to youenn fablet from comment #42) > > As long as WebExtensions exist which run arbitrary user provided code inside > > the sandbox applications will want to add custom paths to the sandbox for > > various functionality. A simple example is Epiphany stores adblock data that > > the web process has to read. This data does not fall under any normal WebKit > > directories. > > In that particular case, the UIProcess could open this adblock data file and > pass the file handle to the WebProcess. The WebProcess could then read the > content (or if not feasible, the content would be sent through IPC either > raw or processed). Yes but it can get out of hand, what if you want the ability to write arbitrary cache files to a private directory. I also think for that plan to be realistic WebKitGTK needs to grow new APIs to easily expose private IPC (DBus) as I believe the WebKit IPC exposed is only one-way (correct me if I'm wrong). > Given JavaScript is run in WebProcess, I am not sure it is safe for > arbitrary user provided code to run in it and provide priviledges to enable > it to run. The goal is certainly to minimize it but the example of an adblocker is entirely safe to run in that context IMO. Yes the application must understand that adding directories increases risk.
(In reply to youenn fablet from comment #42) > In that particular case, the UIProcess could open this adblock data file and > pass the file handle to the WebProcess. The WebProcess could then read the > content (or if not feasible, the content would be sent through IPC either > raw or processed). That's true, but it doesn't help for cases like /tmp. > Given JavaScript is run in WebProcess, I am not sure it is safe for > arbitrary user provided code to run in it and provide priviledges to enable > it to run. We have an entire web process API and it's a feature used by many applications, so it can't just go away. > (In reply to Michael Catanzaro from comment #39) > Another approach would be to make WebKit aware of these code paths. > Something like, please use "/tmp/epiphany-mcatanzaro-fPO5Qw" as temporary > folder for that browsing session. > Then, WebKit would whitelist this folder at process launch time. > As an added bonus, WebKit could for instance monitor the size of > "/tmp/epiphany-mcatanzaro-fPO5Qw", clear the folder when appropriate... Problem is that only works for directories controlled by WebKit itself. E.g. we were already whitelisting all paths required by WebsiteDataStore. We could get it working with hacks like setting base-data-dir to be /tmp/epiphany-mcatanzaro-fPO5Qw but it's really better to just let the UI process decide what web process should be allowed to touch.