GNOME Bugzilla – Bug 784947
Evince's DVI exporter is susceptible to a command injection attack
Last modified: 2017-11-27 21:24:25 UTC
With bug 441319 came a DVI backend. It's exporter (which seems to be triggered when printing to a PDF file) eventually calls g_spawn_command_line_sync with user supplied input, i.e. the filename of the file. command_line = g_strdup_printf ("dvipdfm %s -o %s \"%s\"", /* dvipdfm -s 1,2,.., -o exporter_filename dvi_filename */ dvi_document->exporter_opts->str, dvi_document->exporter_filename, dvi_document->context->filename); If the file is cleverly named, it might be able to cause a command injection. $ cat boom.tex \documentclass{article} \begin{document} Boom \end{document} $ dvilualatex boom.tex ... $ cp boom.dvi '/tmp/foo";touch boom;bar"' $ evince /tmp/foo*boom*\;bar\" Thread 1 "evince" hit Breakpoint 1, g_spawn_command_line_sync ( command_line=0x55a324e9eb40 "dvipdfm -s 1, -o /tmp/evince_print.pdf.0ZO72Y \"/tmp/foo\";touch boom;bar\"\"", standard_output=0x0, standard_error=0x0, exit_status=0x7fff1f9f8d8c, error=0x7fff1f9f8d90) at /build/glib2.0-prJhLS/glib2.0-2.48.2/./glib/gspawn.c:716 716 /build/glib2.0-prJhLS/glib2.0-2.48.2/./glib/gspawn.c: No such file or directory. (gdb) p command_line $1 = (const gchar *) 0x55a324e9eb40 "dvipdfm -s 1, -o /tmp/evince_print.pdf.0ZO72Y \"/tmp/foo\";touch boom;bar\"\"" (gdb) g_spawn_command_line_sync seems to call g_shell_parse_argv () which then results in something like [pid 666] execve("/usr/bin/dvipdfm", ["dvipdfm", "-s", "1,", "-o", "/tmp/evince_print.pdf.U5B12Y", "/tmp/foo;touch", "boom;bar"], [/* 76 vars */]) = 0 Now it only added an unexpected parameter. But it seems likely that dvipdfm's -D switch is able to cause more harm (quoting from the documentation http://texdoc.net/texmf-dist/doc/dvipdfm/dvipdfm.pdf): The user must specify the command line required to invoke an external program to perform this conversion. The command line required to invoke the conversion program is specified using the -D command line (or configuration file) option. The string passed to the -D command line option is a C-style string that is parsed by dvipdfm . Within the string, expansions are performed as described in Table 5. For example, to use GhostScript, one might use the command line -D "cat %i | gs -q -sDEVICE=pdfwrite -sOutputFile=%o - -c quit" So if we managed to rename our document to something including -D and a scary command line, we might be screwed. An easy mitigation for now, I think, is to call g_shell_quote instead of manually trying to escape as it's done now. In [74]: fmt = "dvipdfm %s -o %s \"%s\"" In [75]: fn = '/tmp/foo";$(touch boom);bar"' In [76]: GLib.shell_parse_argv(fmt % (1,2,fn)) Out[76]: (True, argvp=['dvipdfm', '1', '-o', '2', '/tmp/foo;$(touch', 'boom);bar']) In [77]: GLib.shell_parse_argv(fmt % (1,2,GLib.shell_quote(fn))) Out[77]: (True, argvp=['dvipdfm', '1', '-o', '2', "'/tmp/foo;$(touch", "boom);bar'"]) In [78]: In addition, it seems to be clever to using absolute file paths instead in order to prevent a file named '-D foo' sneaking in. Because the g_shell_quote wouldn't prevent dvipdfm being called with a file name '-D foo' which then might cause the trouble mentioned above. NB: g_spawn_command_line_sync does not seem to actually go through /bin/sh like a system() call would. Instead it seems to set up its own argv and calls execve.
Created attachment 355583 [details] [review] potential patch. It's untested, however. I've quickly cooked up this patch. It seems innocent to me. I couldn't build it due to missing dependencies *sigh*. I also didn't use g_autofree which might be appropriate here. But I don't know how well that is indeed supported.
(In reply to comment #0) > in order to prevent a file named '-D foo' sneaking in. Because the > g_shell_quote wouldn't prevent dvipdfm being called with a file name > '-D foo' which then might cause the trouble mentioned above. dvipdfm (xdvipdfmx, from Debian package "textlive-binaries") seems to support the usual "--" to stop parsing options. So maybe one just needs to do: dvipdfm %s -o %s -- '%s'
comment #0: > Now it only added an unexpected parameter. But it seems likely that > dvipdfm's -D switch is able to cause more harm Yup. $ cat inclusion.tex foo bar baz \special{psfile=/etc/hosts} \bye $ dviluatex inclusion.tex $ cp inclusion.dvi lame.dvi 'inclusion.dvi' -> 'lame.dvi' $ cp inclusion.dvi 'lame.dvi" -D "gnome-calculator' $ evince 'lame.dvi" -D "gnome-calculator' In Evince, open the print dialog and click "Preview". Watch the calculator pop up. The patch you posted does break the injection. However, as mentioned, the command should be "dvipdfm %s -o %s -- %s". It's not strictly necessary to patch this bug (even if the input file is named "-Dfoo", the lack of another argument to act as input file will make dvipdfm try to read from stdin and get nothing), but it's the correct thing to do.
Created attachment 356237 [details] [review] [PATCH 1/3] dvi: Use g_spawn_sync to call dvipdfm This is an alternative patch in which I took what seems to me the safest route and used g_spawn_sync, instead of trying to escape any argument. Additionally, I added a hackish error reporting mechanism more intended as an RFC than as a definitive solution.
Created attachment 356238 [details] [review] [PATCH 2/3] dvi: Rename a few exporter-related variables for clarity Tried to clarify the code a bit by tweaking the comments and renaming some variables. (Also fixed some whitespace brokenness nearby.)
Created attachment 356239 [details] [review] [PATCH 3/3][RFC] Add a mechanism for exporters to report failure Probably not for merging as is. Trying the same injection with the patched binary you will see the previewer pop in and immediately out and a Gtk-CRITICAL in the console. Apparently this is because the "distiller" invoked by dvipdfm fails to parse /etc/hosts as a PostScript file causing dvipdfm itself to fail, and when it does so, it removes the output file. Evince still assumes that export went OK and that the file exists: there doesn't seem to be a way for the exporter in the backend to inform the frontend that the export failed. This hacky patch lets Evince show an error message.
comment #4: > Additionally, I added a hackish error reporting mechanism more intended > as an RFC than as a definitive solution. This is wrong, the error reporting thing is in patch 3 only.
(In reply to astian from comment #3) > comment #0: > > Now it only added an unexpected parameter. But it seems likely that > > dvipdfm's -D switch is able to cause more harm > > Yup. thanks for confirming both the existence of the vulnerability and the soundness of the proposed patch. > The patch you posted does break the injection. However, as mentioned, > the command should be "dvipdfm %s -o %s -- %s". With either escaped arguments or a manually built argv, of course. You seem to opt for the latter. I think it's too complex for what it achieves but YMMV. Let me attach a patch which includes the "--" separation so that we have the options at hand to discuss them. I like your idea about propagating the error. I feel it should rather be a separate issue to discuss so that we can concentrate on the injection vulnerability here. Do you mind creating a new issue and proposing a patch there?
Created attachment 356282 [details] [review] patch
bump
Comment on attachment 356282 [details] [review] patch >From 1040ffa506ffa35c2ee4959b090c1b1cb554cd86 Mon Sep 17 00:00:00 2001 >From: Tobias Mueller <muelli@cryptobitch.de> >Date: Fri, 14 Jul 2017 12:52:14 +0200 >Subject: [PATCH] dvi: Mitigate command injection attacks by quoting filename > >With commit 1fcca0b8041de0d6074d7e17fba174da36c65f99 came a DVI backend. >It exports to PDF via the dvipdfm tool. >It calls that tool with the filename of the currently loaded document. >If that filename is cleverly crafted, it can escape the currently >used manual quoting of the filname. filname -> filename. > Instead of manually quoting the >filename, we use g_shell_quote. To further harden the call to dvipdfm >we use "--" to separate options from filenames. > >https://bugzilla.gnome.org/show_bug.cgi?id=784947 >--- > backend/dvi/dvi-document.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > >diff --git a/backend/dvi/dvi-document.c b/backend/dvi/dvi-document.c >index cbb3f8a..6b93de7 100644 >--- a/backend/dvi/dvi-document.c >+++ b/backend/dvi/dvi-document.c >@@ -300,12 +300,14 @@ dvi_document_file_exporter_end (EvFileExporter *exporter) > gboolean success; > > DviDocument *dvi_document = DVI_DOCUMENT(exporter); >+ gchar* quoted_filename = g_shell_quote (dvi_document->context->filename); > >- command_line = g_strdup_printf ("dvipdfm %s -o %s \"%s\"", /* dvipdfm -s 1,2,.., -o exporter_filename dvi_filename */ >+ command_line = g_strdup_printf ("dvipdfm %s -o %s -- %s", /* dvipdfm -s 1,2,.., -o exporter_filename dvi_filename */ > dvi_document->exporter_opts->str, > dvi_document->exporter_filename, >- dvi_document->context->filename); >- >+ quoted_filename); >+ g_free (quoted_filename); >+ > success = g_spawn_command_line_sync (command_line, > NULL, > NULL, >-- >2.7.4 > Ok, this looks like a simpler solution. I agree we could probably handle and report errors, but I would move that to a different bug.
pushed as 350404c76dc8601e2cdd2636490e2afc83d3090e It would be a shame for the other effort to bitrot, so please open a new bug so that we can discuss this further.
Comment on attachment 356238 [details] [review] [PATCH 2/3] dvi: Rename a few exporter-related variables for clarity marking this patch as obsolete for this bug report. Please open a new bug with these patches so that we can discuss the good idea of error propagation.
Comment on attachment 356237 [details] [review] [PATCH 1/3] dvi: Use g_spawn_sync to call dvipdfm marking this patch as obsolete for this bug report. Please open a new bug with these patches so that we can discuss the good idea of error propagation.
Comment on attachment 356239 [details] [review] [PATCH 3/3][RFC] Add a mechanism for exporters to report failure marking this patch as obsolete for this bug report. Please open a new bug with these patches so that we can discuss the good idea of error propagation.
This issue got CVE-2017-1000159 assigned.