Bug 193571 - [GTK][WPE] Add API to add paths to sandbox
Summary: [GTK][WPE] Add API to add paths to sandbox
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-01-18 07:22 PST by Patrick Griffis
Modified: 2019-03-25 09:34 PDT (History)
11 users (show)

See Also:


Attachments
Patch (9.93 KB, patch)
2019-01-18 07:23 PST, Patrick Griffis
no flags Details | Formatted Diff | Diff
Patch (10.78 KB, patch)
2019-01-18 07:30 PST, Patrick Griffis
no flags Details | Formatted Diff | Diff
Patch (11.03 KB, patch)
2019-01-22 11:09 PST, Patrick Griffis
no flags Details | Formatted Diff | Diff
Patch (11.03 KB, patch)
2019-01-23 07:13 PST, Patrick Griffis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.64 MB, application/zip)
2019-01-23 08:28 PST, Build Bot
no flags Details
Patch (10.88 KB, patch)
2019-01-24 07:18 PST, Patrick Griffis
no flags Details | Formatted Diff | Diff
Patch (10.88 KB, patch)
2019-01-24 07:25 PST, Patrick Griffis
no flags Details | Formatted Diff | Diff
Patch (11.01 KB, patch)
2019-01-24 09:43 PST, Patrick Griffis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Griffis 2019-01-18 07:22:17 PST
[GTK][WPE] Add API to add paths to sandbox
Comment 1 Patrick Griffis 2019-01-18 07:23:15 PST
Created attachment 359482 [details]
Patch
Comment 2 Build Bot 2019-01-18 07:26:01 PST
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
Comment 3 Patrick Griffis 2019-01-18 07:30:14 PST
Created attachment 359484 [details]
Patch
Comment 4 Adrian Perez 2019-01-18 08:29:12 PST
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.
Comment 5 Patrick Griffis 2019-01-18 09:15:11 PST
(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 6 Michael Catanzaro 2019-01-18 09:15:36 PST
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!
Comment 7 Adrian Perez 2019-01-18 14:49:36 PST
(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 8 Michael Catanzaro 2019-01-20 19:52:55 PST
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?
Comment 9 Patrick Griffis 2019-01-21 05:39:36 PST
(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.
Comment 10 Michael Catanzaro 2019-01-21 12:54:25 PST
(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.)
Comment 11 Patrick Griffis 2019-01-22 05:19:02 PST
(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 12 Michael Catanzaro 2019-01-22 08:44:50 PST
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
Comment 13 Michael Catanzaro 2019-01-22 08:45:52 PST
(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. :)
Comment 14 Patrick Griffis 2019-01-22 10:21:12 PST
(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.
Comment 15 Patrick Griffis 2019-01-22 11:09:57 PST
Created attachment 359754 [details]
Patch
Comment 16 Michael Catanzaro 2019-01-22 12:19:28 PST
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.)
Comment 17 Michael Catanzaro 2019-01-22 12:36:46 PST
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.
Comment 18 Patrick Griffis 2019-01-22 14:08:07 PST
(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.
Comment 19 Michael Catanzaro 2019-01-22 14:51:19 PST
OK, please ask Carlos Garcia to do final API review.
Comment 20 Patrick Griffis 2019-01-23 07:13:37 PST
Created attachment 359878 [details]
Patch
Comment 21 Build Bot 2019-01-23 08:28:51 PST
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
Comment 22 Build Bot 2019-01-23 08:28:53 PST
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
Comment 23 Michael Catanzaro 2019-01-23 08:57:23 PST
(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 24 Carlos Garcia Campos 2019-01-24 03:42:31 PST
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.
Comment 25 Patrick Griffis 2019-01-24 07:18:30 PST
Created attachment 360011 [details]
Patch
Comment 26 Patrick Griffis 2019-01-24 07:22:01 PST
(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).
Comment 27 Patrick Griffis 2019-01-24 07:25:47 PST
Created attachment 360012 [details]
Patch
Comment 28 youenn fablet 2019-01-24 07:31:28 PST
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?
Comment 29 Patrick Griffis 2019-01-24 09:21:54 PST
(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?
Comment 30 Patrick Griffis 2019-01-24 09:31:21 PST
(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.
Comment 31 Patrick Griffis 2019-01-24 09:43:26 PST
Created attachment 360018 [details]
Patch
Comment 32 youenn fablet 2019-01-24 11:09:58 PST
(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.
Comment 33 Patrick Griffis 2019-01-24 12:14:59 PST
(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.
Comment 34 Carlos Garcia Campos 2019-01-25 00:28:44 PST
(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?
Comment 35 Patrick Griffis 2019-01-25 05:28:24 PST
(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.
Comment 36 Carlos Garcia Campos 2019-01-25 05:36:27 PST
(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.
Comment 37 Patrick Griffis 2019-01-25 06:05:55 PST
(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 38 Michael Catanzaro 2019-01-25 07:28:32 PST
Comment on attachment 360018 [details]
Patch

I think all arguments are addressed, now?
Comment 39 Michael Catanzaro 2019-01-25 07:34:31 PST
(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 40 WebKit Commit Bot 2019-01-25 07:55:00 PST
Comment on attachment 360018 [details]
Patch

Clearing flags on attachment: 360018

Committed r240473: <https://trac.webkit.org/changeset/240473>
Comment 41 WebKit Commit Bot 2019-01-25 07:55:02 PST
All reviewed patches have been landed.  Closing bug.
Comment 42 youenn fablet 2019-01-25 09:36:44 PST
> 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...
Comment 43 Patrick Griffis 2019-01-25 10:13:56 PST
(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.
Comment 44 Michael Catanzaro 2019-01-25 11:06:18 PST
(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.