[GTK][WPE] Explicitly blacklist problematic directories for sandbox
Created attachment 373238 [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 373240 [details] Patch
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?
(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.
Created attachment 373243 [details] Patch
Created attachment 373244 [details] Patch
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.
(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}
Created attachment 373250 [details] Patch
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 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 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.
(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.
Created attachment 373349 [details] Patch
Comment on attachment 373349 [details] Patch Clearing flags on attachment: 373349 Committed r247076: <https://trac.webkit.org/changeset/247076>
All reviewed patches have been landed. Closing bug.