Bug 199367 - [GTK][WPE] Explicitly blacklist problematic directories for sandbox
Summary: [GTK][WPE] Explicitly blacklist problematic directories for 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-07-01 09:06 PDT by Patrick Griffis
Modified: 2019-07-02 16:05 PDT (History)
8 users (show)

See Also:


Attachments
Patch (2.75 KB, patch)
2019-07-01 09:09 PDT, Patrick Griffis
no flags Details | Formatted Diff | Diff
Patch (2.74 KB, patch)
2019-07-01 09:36 PDT, Patrick Griffis
no flags Details | Formatted Diff | Diff
Patch (2.86 KB, patch)
2019-07-01 09:55 PDT, Patrick Griffis
no flags Details | Formatted Diff | Diff
Patch (2.87 KB, patch)
2019-07-01 10:09 PDT, Patrick Griffis
no flags Details | Formatted Diff | Diff
Patch (2.95 KB, patch)
2019-07-01 13:23 PDT, Patrick Griffis
no flags Details | Formatted Diff | Diff
Patch (2.96 KB, patch)
2019-07-02 11:57 PDT, 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-07-01 09:06:54 PDT
[GTK][WPE] Explicitly blacklist problematic directories for sandbox
Comment 1 Patrick Griffis 2019-07-01 09:09:20 PDT
Created attachment 373238 [details]
Patch
Comment 2 Build Bot 2019-07-01 09:11:00 PDT
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-07-01 09:36:21 PDT
Created attachment 373240 [details]
Patch
Comment 4 Carlos Alberto Lopez Perez 2019-07-01 09:40:51 PDT
Comment on attachment 373240 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1200
> +    /* These are backend specific though the blacklist covers all for consistent support */
> +    const Vector<CString> blacklistedPrefixes = {
> +        "/tmp", /* This doesn't work with flatpak-spawn */
> +        /* The rest of these are re-created by the bwrap sandbox in both cases and don't make sense */
> +        "/sys", "/proc", "/dev",
> +    };
> +
> +    for (const CString& prefix : blacklistedPrefixes) {
> +        if (g_str_has_prefix(path, prefix.data()))
> +            return false;
> +    }

Shouldn't this paths be matched with a trailing "/"?
Otherwise if there is a path named "/system" it looks like is going to match here with "/sys" (for example), isn't it?
Comment 5 Patrick Griffis 2019-07-01 09:43:30 PDT
(In reply to Carlos Alberto Lopez Perez from comment #4)
> Comment on attachment 373240 [details]
> 
> Shouldn't this paths be matched with a trailing "/"?
> Otherwise if there is a path named "/system" it looks like is going to match
> here with "/sys" (for example), isn't it?

Good catch. Yeah we need to handle `/sys` as well as `/sys/foo` though.
Comment 6 Patrick Griffis 2019-07-01 09:55:22 PDT
Created attachment 373243 [details]
Patch
Comment 7 Patrick Griffis 2019-07-01 10:09:34 PDT
Created attachment 373244 [details]
Patch
Comment 8 Michael Catanzaro 2019-07-01 10:59:39 PDT
Comment on attachment 373244 [details]
Patch

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

Good catch Carlos!

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1199
> +    /* NOTE: Due to previous check there is always 1 leading `/` */

one

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1201
> +    return blacklistedPrefixes.find(splitPath.get()[1]) == WTF::notFound;

What happens if path is "/"? g_strsplit returns a strv with two empty strings and it doesn't crash? Seems unlikely?

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1225
> +    g_return_if_fail(pathIsNotBlacklisted(path));

This is OK, but g_critical() with an error message would be nicer.
Comment 9 Patrick Griffis 2019-07-01 13:15:03 PDT
(In reply to Michael Catanzaro from comment #8)
> Comment on attachment 373244 [details]
> > Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1201
> > +    return blacklistedPrefixes.find(splitPath.get()[1]) == WTF::notFound;
> 
> What happens if path is "/"? g_strsplit returns a strv with two empty
> strings and it doesn't crash? Seems unlikely?

Sure does: g_stv_length() == 2 and it contains {"", "", NULL}
Comment 10 Patrick Griffis 2019-07-01 13:23:30 PDT
Created attachment 373250 [details]
Patch
Comment 11 Michael Catanzaro 2019-07-01 14:11:19 PDT
Comment on attachment 373250 [details]
Patch

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

OK, only nits remaining from me.

I'll wait until tomorrow before giving r+ in case Carlos Garcia wants any changes.

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1190
> +    /* These are backend specific though the blacklist covers all for consistent support */

// The directories that won't work are specific to the sandbox backends, but the blacklist covers all of them for consistency.

Note: //

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1195
> +        "tmp", /* This doesn't work with flatpak-spawn */
> +        /* The rest of these are re-created by the bwrap sandbox in both cases and don't make sense */
> +        "sys", "proc", "dev",
> +        /* A value of just `/` also doesn't make sense */

Besides changing the comments from /* to //, also be sure to end them with a period to form a complete sentence. This is a WebKit style requirement.

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1199
> +    /* NOTE: Due to previous check there is always one leading `/` */

Ditto.

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1214
> + * Paths in directories such as `/tmp`, `/sys`, `/proc`, and `/dev` or all of `/` are not valid.

Wrap at 80 chars

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1227
> +        g_critical("Path added to sandbox is blacklisted (%s)", path);

How about: "Attempted to add disallowed path to sandbox: %s"

Might as well also first do:

if (!g_path_is_absolute(path)) {
    g_critical("Attempted to add relative path to sandbox, but paths must be absolute");
    return;
}
Comment 12 Carlos Garcia Campos 2019-07-02 01:28:49 PDT
Comment on attachment 373250 [details]
Patch

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

> Source/WebKit/ChangeLog:7
> +

Explain the change here, please. What are problematic directories?

> Source/WebKit/ChangeLog:9
> +        (path_is_not_blacklisted):

pathIsBlacklisted

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1191
> +    const Vector<CString> blacklistedPrefixes = {

static? You can also pass the inline capacity to avoid heap allocation, since the vector size is fixed. 

static const Vector<CString, 5> blacklistedPrefixes = {

>> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1199
>> +    /* NOTE: Due to previous check there is always one leading `/` */
> 
> Ditto.

What previous check? g_path_is_absolute in the caller? I would add an assert here, it's self documented and ensures that's the case. Or we can move the check to this function and consider relative paths to be blacklisted too.

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1201
> +    return blacklistedPrefixes.find(splitPath.get()[1]) != WTF::notFound;

This is Vector::contains()

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1228
> +        return;

Ok, so we don't allow to add certain paths to the sandbox explicitly. This should be testable, no?
Comment 13 Michael Catanzaro 2019-07-02 05:00:18 PDT
Comment on attachment 373250 [details]
Patch

We agreed on IRC that /tmp needs to not be blacklisted and the flatpak-spawn subsandbox just needs to go away since it doesn't fit our requirements to allow whitelisting under /tmp.
Comment 14 Michael Catanzaro 2019-07-02 05:01:54 PDT
(In reply to Carlos Garcia Campos from comment #12) 
> Ok, so we don't allow to add certain paths to the sandbox explicitly. This
> should be testable, no?

I thought about that, but it's not easily testable because the test would cause a critical (crashing the test). It could be done using g_test_trap_subprocess() to ensure that passing a blacklisted path causes the test to crash, but it seems like a low-value test.
Comment 15 Patrick Griffis 2019-07-02 11:57:37 PDT
Created attachment 373349 [details]
Patch
Comment 16 WebKit Commit Bot 2019-07-02 16:05:13 PDT
Comment on attachment 373349 [details]
Patch

Clearing flags on attachment: 373349

Committed r247076: <https://trac.webkit.org/changeset/247076>
Comment 17 WebKit Commit Bot 2019-07-02 16:05:15 PDT
All reviewed patches have been landed.  Closing bug.