GNOME Bugzilla – Bug 774497
Is it feasible to sandbox all thumbnailers with Bubblewrap?
Last modified: 2018-07-09 08:23:39 UTC
Is it feasible to sandbox all thumbnailers with bubblewrap, to protect the user from thumnbnailer-based exploits? If the thumbnailer process is sandboxed, instead of being able to do whatever it wants, it'd have a read-only access to the file being thumbnailed (or rather the directory where the file is, because bwrap --file copies the file, and we don't want to do that for large video files for example) and write access only to the temp location where the thumbnail file needs to be saved. No network access, no access to anywhere else in the filesystem. Of course this doesn't protect the user from pixbuf exploits, but sandboxing *that* is a little bit more complicated and I know people are working on it already. So, what do you think? is it feasible to do this? does it break any known assumptions that thumbnailers make today that makes them require networking / full access to the filesystem, etc? would patches for that be accepted?
Out-of-process thumbnailers just get run from the single call to g_spawn_command_line_sync() in gnome_desktop_thumbnail_factory_generate_thumbnail(). As you say, it should be easy to bubblewrap the thumbnailer scripts. It is up to each thumbnailer whether it needs access to the file system or network - try them and see! Requiring networking sounds like a bug (no wifi - no thumbnails!?). Requiring FS access (for precomputed whatever?) sounds solvable; maybe the .thumbnailer files can specify a whitelist of program-supplied helper files, or the programs can be modified to include that data in their binaries.
Is bubblewrap the right tool for the job though? since bubblewrap can't be run inside a bubblewrap, it'd mean aplications that are already flatpak'd won't be able to use thumbnails, and skipping bubblewrap for an app that is flatpak'd means we won't be able to isolate the thumbnailer from the network if the app isn't... so a potential malicious file might not be able to use an insecure thumbnailer to compromise the entire system, but they could still use it to compromise the app and get access to anything the app has access to (including network, the user's files, etc etc). Of course using bubblewrap where we can and just falling back to normal, unsandboxed processes where we can't is probably still more secure than what we have now, but maybe there's a better way? Also, what's the best way to check "are we in a sandbox right now"? Trying bubblewrap and if it fails launching the process normally? that sounds a bit fragile - how can we tell the difference between the actual process returning a nonzero exit code and bubblewrap returning a nonzero exit code?
If you want to solve this with bubblewrap anyway, how about a DBus-activated bubblewrap'ed background process creating all the thumbnails? This way both bubblewrap'ed and non-bubblewrap'ed applications could (re-)use the same thumbnailer daemon.
Today, an exploit has been published which could have been prevented by writing better code or sandboxing the thumbnailers: http://news.dieweltistgarnichtso.net/posts/gnome-thumbnailer-msi-fail.html
(In reply to Christian Stadelmann from comment #4) > Today, an exploit has been published which could have been prevented by > writing better code or sandboxing the thumbnailers: > http://news.dieweltistgarnichtso.net/posts/gnome-thumbnailer-msi-fail.html For the record, this is not a GNOME vulnerability, it's a vulnerability in a 3rd party thumbnailer, and that page is full of factual errors. "Don't use GNOME Files" is not a valid security advice. Yes, this vulnerability could be prevented by better sandboxing, but it still doesn't mean it's GNOME's fault. We can't protect the user from every broken 3rd party script. Even if we had better sandboxing, I don't think the script shown on that page would even *work* in a sandbox, since it uses wine and if you give wine a tmpfs instead of $HOME it'll spend a lot of time generating the "wine prefix" every time you'd want to thumbnail a file, and that would probably time-out, especially on slower systems.
(In reply to Elad Alfassa from comment #5) > For the record, this is not a GNOME vulnerability, it's a vulnerability in a > 3rd party thumbnailer, and that page is full of factual errors. "Don't use > GNOME Files" is not a valid security advice. That's true. I've contacted the author because of these false claims. Any yes, the 3rd party thumbnailer is pretty broken. Sandboxing is no perfect solution. Is there a better one? Probably only to not run so much code from untrusted sources.
*** Bug 785049 has been marked as a duplicate of this bug. ***
Try to stick to the factual and technical aspects please. It's GNOME's job to protect itself and its users against horribly written thumbnailers. I'm sure people will find ways of written thumbnailers that do work inside the sandbox, even if it means going through hoops to make it happen.
Created attachment 356074 [details] [review] thumbnail: Split off running the script Move most of the script command generation to a separate file, making the function return a GBytes from a successful thumbnailer run, so as to avoid having to clean up temporary files from the thumbnailer run. Note that it changes a few subtle things which shouldn't be a problem in practice, but, as a corner case, might have been used by applications: - Thumbnailers must output PNG images. pixbuf_new_from_bytes() could have been made more complicated to handle all images, and then we would restrict the thumbnailer output format separately, but it makes no sense to write complicated code to remove it in the next commit. - URIs which have no backing path are not supported. This will likely cause problems for thumbnailing remote shares on OSes which lack gvfsd-fuse. Support could be re-added in the future.
Created attachment 356075 [details] [review] thumbnail: Sandbox thumbnailers on Linux On Linux systems, bubblewrap is now required to launch thumbnailers in a restricted environment. - Only /usr and the compilation ${prefix} of the gnome-desktop library will be available to the thumbnailer as read-only - The network is disabled - The filename of the file to thumbnail is hidden - Bubblewrap is not used if the application is already sandboxed in Flatpak as all privileges to create a new namespace are dropped when the initial one is created.
Seccomp would be coming afterwards: https://bugzilla.gnome.org/show_bug.cgi?id=785197
Review of attachment 356074 [details] [review]: Everything looks fine to me from a correctness perspective, but added some pedantic nits should you be interested. ::: libgnome-desktop/gnome-desktop-thumbnail-script.c @@ +108,3 @@ +{ + GPtrArray *array; + g_auto(GStrv) cmd_elems; I follow the philosophy of all g_auto() being assigned to NULL in case someone changes the code later. Especially when goto's are in play. It looks safe in it's current form though. @@ +171,3 @@ +{ + ScriptExec *exec; + g_autoptr(GFile) file; Same. @@ +186,3 @@ + goto bail; + close (fd); + exec->outfile = tmpname; You might consider autofree and g_steal_pointer() to be explicit about ownership transfer. @@ +202,3 @@ +{ + g_autofree char *error_out = NULL; + g_auto(GStrv) expanded_script; Same @@ +214,3 @@ + goto out; + + g_debug ("About to launch script: "); These look like they were transformed from a g_printerr() or soemthing. Not sure the tailing space makes much sense as a g_debug(). @@ +220,3 @@ + + ret = g_spawn_sync (NULL, expanded_script, NULL, G_SPAWN_SEARCH_PATH, + NULL, NULL, NULL, &error_out, error_out doesn't seem to be used. G_SPAWN_STDERR_TO_DEV_NULL and NULL perhaps? @@ +222,3 @@ + NULL, NULL, NULL, &error_out, + &exit_status, error); + if (ret && exit_status == 0) Maybe use g_spawn_check_exit_status() on exit_status. I don't think error is automatically set based on exit status codes, at least if I'm reading the documentation correctly. ::: libgnome-desktop/gnome-desktop-thumbnail.c @@ +993,3 @@ + GError **error) +{ + g_autoptr(GdkPixbufLoader) loader; Same @@ +1003,3 @@ + g_bytes_get_size (bytes), + error)) + { None of the other conditionals in this function use {} for child scope.
Review of attachment 356075 [details] [review]: ::: libgnome-desktop/gnome-desktop-thumbnail-script.c @@ -101,2 +108,4 @@ } +/* From https://github.com/flatpak/flatpak/blob/master/common/flatpak-run.c */ +static void You might do: G_GNUC_NULL_TERMINATED static void for some future proofing. @@ -103,0 +110,58 @@ +/* From https://github.com/flatpak/flatpak/blob/master/common/flatpak-run.c */ +static void +add_args (GPtrArray *argv_array, ...) ... 55 more ... You might want to assert the validity of script up front here so you don't accidentally NULL terminate the argv. @@ -103,0 +110,66 @@ +/* From https://github.com/flatpak/flatpak/blob/master/common/flatpak-run.c */ +static void +add_args (GPtrArray *argv_array, ...) ... 63 more ... Perhaps use O_CLOEXEC in case the another thread does a fork()/exec()? It looks like F_SETFD is already clearing the flag post-fork, which I think would give the intended semantics. @@ -159,2 +276,5 @@ { g_free (exec->infile); + if (exec->outdir) + { + g_rmdir (exec->outdir); Do we expect outdir to be empty? Or does it just not matter if we succeed at removing it? If it's going to be empty, maybe add a comment about the expectation. I think that is the case given the bwrap bindings, but not clear to me. @@ -177,1 +309,3 @@ exec = g_new0 (ScriptExec, 1); +#ifdef HAVE_BWRAP + if (!g_file_test ("/.flatpak-info", G_FILE_TEST_IS_REGULAR)) Maybe add a comment about not being able to run bwrap from inside flatpak. @@ +323,3 @@ + { + char *tmpl; + g_autofree char *ext; Set to NULL since you'll need to break out of g_mkdtemp() failure below. @@ -184,5 +321,11 @@ - fd = g_file_open_tmp (".gnome_desktop_thumbnail.XXXXXX", &tmpname, NULL); - if (fd == -1) - goto bail; ... 2 more ... +#ifdef HAVE_BWRAP + if (exec->sandbox) + { ... 8 more ... g_mkdtemp() can fail. NULL is returned and errno is set. So probably g_file_error_from_errno() or something like that.
Review of attachment 356074 [details] [review]: ::: libgnome-desktop/gnome-desktop-thumbnail-script.c @@ +108,3 @@ +{ + GPtrArray *array; + g_auto(GStrv) cmd_elems; I agree. I would also recommend a = NULL here.
Review of attachment 356074 [details] [review]: ::: libgnome-desktop/gnome-desktop-thumbnail-script.c @@ +214,3 @@ + goto out; + + g_debug ("About to launch script: "); I've if 0'd this section. It was a g_print(). @@ +220,3 @@ + + ret = g_spawn_sync (NULL, expanded_script, NULL, G_SPAWN_SEARCH_PATH, + NULL, NULL, NULL, &error_out, I added debug in the error case for this. ::: libgnome-desktop/gnome-desktop-thumbnail.c @@ +1003,3 @@ + g_bytes_get_size (bytes), + error)) + { We use braces when the conditional or the block is multi-line.
Created attachment 356108 [details] [review] thumbnail: Split off running the script Move most of the script command generation to a separate file, making the function return a GBytes from a successful thumbnailer run, so as to avoid having to clean up temporary files from the thumbnailer run. Note that it changes a few subtle things which shouldn't be a problem in practice, but, as a corner case, might have been used by applications: - Thumbnailers must output PNG images. pixbuf_new_from_bytes() could have been made more complicated to handle all images, and then we would restrict the thumbnailer output format separately, but it makes no sense to write complicated code to remove it in the next commit. - URIs which have no backing path are not supported. This will likely cause problems for thumbnailing remote shares on OSes which lack gvfsd-fuse. Support could be re-added in the future.
Created attachment 356109 [details] [review] thumbnail: Sandbox thumbnailers on Linux On Linux systems, bubblewrap is now required to launch thumbnailers in a restricted environment. - Only /usr and the compilation ${prefix} of the gnome-desktop library will be available to the thumbnailer as read-only - The network is disabled - The filename of the file to thumbnail is hidden - Bubblewrap is not used if the application is already sandboxed in Flatpak as all privileges to create a new namespace are dropped when the initial one is created.
Attachment 356108 [details] pushed as c1956f3 - thumbnail: Split off running the script Attachment 356109 [details] pushed as 8b1db18 - thumbnail: Sandbox thumbnailers on Linux
Review of attachment 356075 [details] [review]: ::: libgnome-desktop/gnome-desktop-thumbnail-script.c @@ -103,0 +110,58 @@ +/* From https://github.com/flatpak/flatpak/blob/master/common/flatpak-run.c */ +static void +add_args (GPtrArray *argv_array, ...) ... 55 more ... Added some guards at the top of the function. @@ -103,0 +110,66 @@ +/* From https://github.com/flatpak/flatpak/blob/master/common/flatpak-run.c */ +static void +add_args (GPtrArray *argv_array, ...) ... 63 more ... Done. @@ -159,2 +276,5 @@ { g_free (exec->infile); + if (exec->outdir) + { + g_rmdir (exec->outdir); wrong order... outfile is the generated thumbnail, outdir is the directory we bind mount as /tmp in the sandbox.
Lots of people are using openscad with a perl or python script to create thumbnails for .stl and .scad files (3d printer files). It does not works any more since thumbnailer are bwraped. Openscad needs access to X11 display and OpenGL (could not use xvfb-run) to render png from stl/scad. Is there a chance you let custom thumbnailers access current user X11 display with bwrap options : --bind /tmp/.X11-unix/X0 /tmp/.X11-unix/X0 --setenv DISPLAY :0 Thanks
(In reply to Laurent Haond from comment #20) > Is there a chance you let custom thumbnailers access current user X11 > display with bwrap options : --bind /tmp/.X11-unix/X0 /tmp/.X11-unix/X0 > --setenv DISPLAY :0 Although I understand the use case, having a thumbnailer access the current display sounds like a pretty wrong thing to do. Adding access to the X11 display completely breaks the concept of sandboxing as X11 has no security at all allowing manipulation of any application's user input, GUI and probably other dangerous things.
Ok, i get you point so i'm trying to run it with xvfb-run : /usr/bin/xvfb-run -a -s "-screen 0 1400x900x24 -ac +extension GLX +render -noreset" \ /usr/bin/openscad -o /tmp/gnome-desktop-thumbnailer.png --imgsize=256,256 \ /tmp/gnome-desktop-file-to-thumbnail.scad but i got an error : Can't exec "/usr/bin/xvfb-run": No such file or directory at /usr/local/bin/stl_thumb.pl line 66 (#1) (W exec) A system(), exec(), or piped open call could not execute the named program for the indicated reason. Can you tell me or point me to a documentation where i can find a way to have all the requiered programs available when running my thumbnailer bwraped ? I need perl, bash, xvfb/xvfb-run and openscad to be available. Thanks.
Another try after copying all stuff needed in /usr/local/bin (it seems all /usr is available in sandbox) but still doesnt work : xvfb-run: error: Xvfb failed to start I think Xvfb needs tcp/network (at least on loopback). Any chance you add access to /bin (bash,awk,mktemp,...) and loopback networking in the thumbnail sandbox ? Thanks.