Bug 199367 – [GTK][WPE] Explicitly blacklist problematic directories for sandbox

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

[Patrick Griffis]

[GTK][WPE] Explicitly blacklist problematic directories for sandbox

[Patrick Griffis]

Created attachment 373238 [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 373240 [details]
Patch

[Carlos Alberto Lopez Perez]

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?

[Patrick Griffis]

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

[Patrick Griffis]

Created attachment 373243 [details]
Patch

[Patrick Griffis]

Created attachment 373244 [details]
Patch

[Michael Catanzaro]

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.

[Patrick Griffis]

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

[Patrick Griffis]

Created attachment 373250 [details]
Patch

[Michael Catanzaro]

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;
}

[Carlos Garcia Campos]

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?

[Michael Catanzaro]

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.

[Michael Catanzaro]

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

[Patrick Griffis]

Created attachment 373349 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 373349 [details]
Patch

Clearing flags on attachment: 373349

Committed r247076: <https://trac.webkit.org/changeset/247076>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.