After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 774497 - Is it feasible to sandbox all thumbnailers with Bubblewrap?
Is it feasible to sandbox all thumbnailers with Bubblewrap?
Status: RESOLVED FIXED
Product: gnome-desktop
Classification: Core
Component: Thumbnail
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Desktop Maintainers
Desktop Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-11-15 22:38 UTC by Elad Alfassa
Modified: 2018-07-09 08:23 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
thumbnail: Split off running the script (14.48 KB, patch)
2017-07-20 23:19 UTC, Bastien Nocera
none Details | Review
thumbnail: Sandbox thumbnailers on Linux (8.29 KB, patch)
2017-07-20 23:19 UTC, Bastien Nocera
none Details | Review
thumbnail: Split off running the script (14.69 KB, patch)
2017-07-21 11:22 UTC, Bastien Nocera
committed Details | Review
thumbnail: Sandbox thumbnailers on Linux (8.91 KB, patch)
2017-07-21 11:22 UTC, Bastien Nocera
committed Details | Review

Description Elad Alfassa 2016-11-15 22:38:21 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?
Comment 1 Federico Mena Quintero 2016-11-22 14:15:53 UTC
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.
Comment 2 Elad Alfassa 2016-12-09 12:17:03 UTC
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?
Comment 3 Christian Stadelmann 2016-12-17 02:52:37 UTC
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.
Comment 4 Christian Stadelmann 2017-07-18 19:42:54 UTC
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
Comment 5 Elad Alfassa 2017-07-18 20:13:06 UTC
(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.
Comment 6 Christian Stadelmann 2017-07-18 21:41:44 UTC
(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.
Comment 7 Bastien Nocera 2017-07-20 01:44:01 UTC
*** Bug 785049 has been marked as a duplicate of this bug. ***
Comment 8 Bastien Nocera 2017-07-20 01:56:50 UTC
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.
Comment 9 Bastien Nocera 2017-07-20 23:19:19 UTC
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.
Comment 10 Bastien Nocera 2017-07-20 23:19:26 UTC
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.
Comment 11 Bastien Nocera 2017-07-20 23:23:32 UTC
Seccomp would be coming afterwards:
https://bugzilla.gnome.org/show_bug.cgi?id=785197
Comment 12 Christian Hergert 2017-07-20 23:43:09 UTC
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.
Comment 13 Christian Hergert 2017-07-21 00:05:39 UTC
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.
Comment 14 Matthias Clasen 2017-07-21 00:26:54 UTC
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.
Comment 15 Bastien Nocera 2017-07-21 11:04:31 UTC
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.
Comment 16 Bastien Nocera 2017-07-21 11:22:44 UTC
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.
Comment 17 Bastien Nocera 2017-07-21 11:22:50 UTC
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.
Comment 18 Bastien Nocera 2017-07-21 11:25:05 UTC
Attachment 356108 [details] pushed as c1956f3 - thumbnail: Split off running the script
Attachment 356109 [details] pushed as 8b1db18 - thumbnail: Sandbox thumbnailers on Linux
Comment 19 Bastien Nocera 2017-07-21 11:26:04 UTC
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.
Comment 20 Laurent Haond 2018-07-08 11:15:43 UTC
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
Comment 21 Christian Stadelmann 2018-07-08 12:25:35 UTC
(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.
Comment 22 Laurent Haond 2018-07-09 06:42:22 UTC
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.
Comment 23 Laurent Haond 2018-07-09 08:23:39 UTC
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.