Bug 193571 – [GTK][WPE] Add API to add paths to sandbox

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Patrick Griffis]

[GTK][WPE] Add API to add paths to sandbox

[Patrick Griffis]

Created attachment 359482 [details]
Patch

[EWS Watchlist]

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

[Patrick Griffis]

Created attachment 359484 [details]
Patch

[Adrian Perez]

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.

[Patrick Griffis]

(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.

[Michael Catanzaro]

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!

[Adrian Perez]

(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+!

[Michael Catanzaro]

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?

[Patrick Griffis]

(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.

[Michael Catanzaro]

(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.)

[Patrick Griffis]

(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.

[Michael Catanzaro]

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

[Michael Catanzaro]

(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. :)

[Patrick Griffis]

(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.

[Patrick Griffis]

Created attachment 359754 [details]
Patch

[Michael Catanzaro]

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.)

[Michael Catanzaro]

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.

[Patrick Griffis]

(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.

[Michael Catanzaro]

OK, please ask Carlos Garcia to do final API review.

[Patrick Griffis]

Created attachment 359878 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Michael Catanzaro]

(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.

[Carlos Garcia Campos]

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.

[Patrick Griffis]

Created attachment 360011 [details]
Patch

[Patrick Griffis]

(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).

[Patrick Griffis]

Created attachment 360012 [details]
Patch

[youenn fablet]

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?

[Patrick Griffis]

(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?

[Patrick Griffis]

(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.

[Patrick Griffis]

Created attachment 360018 [details]
Patch

[youenn fablet]

(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.

[Patrick Griffis]

(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.

[Carlos Garcia Campos]

(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?

[Patrick Griffis]

(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.

[Carlos Garcia Campos]

(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.

[Patrick Griffis]

(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.

[Michael Catanzaro]

Comment on attachment 360018 [details]
Patch

I think all arguments are addressed, now?

[Michael Catanzaro]

(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.

[WebKit Commit Bot]

Comment on attachment 360018 [details]
Patch

Clearing flags on attachment: 360018

Committed r240473: <https://trac.webkit.org/changeset/240473>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

> 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...

[Patrick Griffis]

(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.

[Michael Catanzaro]

(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.