Dear all, This is an important topic since it involves security. I'd appreciate it if you spent some time on understanding the issue.
I see three options for what to do about the minted + shell-escape issue: 1. Revert the recently added minted support. 2. Keep the current state of master. 3. Apply the patch at [1] (also attached for convenience). We do not have unanimous agreement on what to do and I would like to have a vote, since this topic involves security. Another relevant piece of news is that the minted author is interested in making it so -shell-escape is not needed [2]. That work could be done in minted within a few months (see the Github issue for details), and perhaps we could incorporate this into LyX 2.4.0. For more background, see the mega-thread at [3]. See, in particular, the useful summary from Guillaume's perspective at [4]. I can see reasonable arguments for all three options. When voting, I ask you to focus specifically on the issue of security. Although adding minted support is a nice feature, if we think it will decrease security, we should not do it. The question I focus on when deciding which option I think is best is "with which option is the average user least likely to end up running malicious code"? Scott [1] https://www.mail-archive.com/search?l=mid&q=20170620203439.GC1008%40GIOVE [2] https://github.com/gpoore/minted/issues/166 [3] https://www.mail-archive.com/search?l=mid&q=20170529215308.ojd3b42tkyqmnl4l%40steph [4] https://www.mail-archive.com/search?l=mid&q=oi9r49%24hn2%241%40blaine.gmane.org
diff --git a/src/Converter.cpp b/src/Converter.cpp index 6e10b18704..45b725e8c6 100644 --- a/src/Converter.cpp +++ b/src/Converter.cpp @@ -19,14 +19,18 @@ #include "Encoding.h" #include "ErrorList.h" #include "Format.h" +#include "InsetList.h" #include "Language.h" #include "LaTeX.h" #include "LyXRC.h" #include "Mover.h" +#include "ParagraphList.h" #include "Session.h" #include "frontends/alert.h" +#include "insets/InsetInclude.h" + #include "support/debug.h" #include "support/FileNameList.h" #include "support/filetools.h" @@ -279,20 +283,38 @@ OutputParams::FLAVOR Converters::getFlavor(Graph::EdgePath const & path, } -bool Converters::checkAuth(Converter const & conv, string const & doc_fname) +bool Converters::checkAuth(Converter const & conv, string const & doc_fname, + bool use_shell_escape) { - if (!conv.need_auth()) + if (!conv.latex()) + use_shell_escape = false; + if (!conv.need_auth() && !use_shell_escape) return true; - const docstring security_warning = bformat( - _("<p>The requested operation requires the use of a converter from " - "%2$s to %3$s:</p>" + string conv_command = conv.command(); + bool const has_shell_escape = contains(conv_command, "-shell-escape"); + size_t const token_pos = conv_command.find("$$"); + bool const has_token = token_pos != string::npos; + string const command = use_shell_escape && !has_shell_escape + ? (has_token ? conv_command.insert(token_pos, "-shell-escape ") + : conv_command.append(" -shell-escape")) + : conv_command; + docstring const security_warning = (use_shell_escape + ? bformat(_("<p>This document uses minted listings, so the LaTeX " + "backend needs to be able to launch external programs:</p>" + "<center><p><tt>%1$s</tt></p></center>" + "<p>The external programs can execute arbitrary commands on " + "your system, including dangerous ones, if instructed to do " + "so by a maliciously crafted LyX document.</p>"), + from_utf8(command)) + : bformat(_("<p>The requested operation requires the use of a " + "converter from %2$s to %3$s:</p>" "<blockquote><p><tt>%1$s</tt></p></blockquote>" - "<p>This external program can execute arbitrary commands on your " - "system, including dangerous ones, if instructed to do so by a " - "maliciously crafted .lyx document.</p>"), - from_utf8(conv.command()), from_utf8(conv.from()), - from_utf8(conv.to())); - if (lyxrc.use_converter_needauth_forbidden) { + "<p>This external program can execute arbitrary commands on " + "your system, including dangerous ones, if instructed to do " + "so by a maliciously crafted LyX document.</p>"), + from_utf8(command), from_utf8(conv.from()), + from_utf8(conv.to()))); + if (lyxrc.use_converter_needauth_forbidden && !use_shell_escape) { frontend::Alert::error( _("An external converter is disabled for security reasons"), security_warning + _( @@ -302,29 +324,43 @@ bool Converters::checkAuth(Converter const & conv, string const & doc_fname) "Forbid needauth converters</i>.)"), false); return false; } - if (!lyxrc.use_converter_needauth) + if (!lyxrc.use_converter_needauth && !use_shell_escape) return true; - static const docstring security_title = - _("An external converter requires your authorization"); + docstring const security_title = use_shell_escape + ? _("A LaTeX backend requires your authorization") + : _("An external converter requires your authorization"); int choice; - const docstring security_warning2 = security_warning + - _("<p>Would you like to run this converter?</p>" - "<p><b>Only run if you trust the origin/sender of the LyX " - "document!</b></p>"); + docstring const security_warning2 = security_warning + (use_shell_escape + ? _("<p>Should LaTeX backends be allowed to run external " + "programs?</p><p><b>Allow them only if you trust the " + "origin/sender of the LyX document!</b></p>") + : _("<p>Would you like to run this converter?</p>" + "<p><b>Only run if you trust the origin/sender of the LyX " + "document!</b></p>")); + docstring const no = use_shell_escape + ? _("Do ¬ allow") : _("Do ¬ run"); + docstring const yes = use_shell_escape ? _("A&llow") : _("&Run"); + docstring const always = use_shell_escape + ? _("&Always allow for this document") + : _("&Always run for this document"); if (!doc_fname.empty()) { LYXERR(Debug::FILES, "looking up: " << doc_fname); - std::set<std::string> & auth_files = theSession().authFiles().authFiles(); + std::set<std::string> & auth_files = use_shell_escape + ? theSession().shellescapeFiles().shellescapeFiles() + : theSession().authFiles().authFiles(); if (auth_files.find(doc_fname) == auth_files.end()) { - choice = frontend::Alert::prompt(security_title, security_warning2, - 0, 0, _("Do ¬ run"), _("&Run"), _("&Always run for this document")); + choice = frontend::Alert::prompt(security_title, + security_warning2, + 0, 0, no, yes, always); if (choice == 2) auth_files.insert(doc_fname); } else { choice = 1; } } else { - choice = frontend::Alert::prompt(security_title, security_warning2, - 0, 0, _("Do ¬ run"), _("&Run")); + choice = frontend::Alert::prompt(security_title, + security_warning2, + 0, 0, no, yes); } return choice != 0; } @@ -459,7 +495,30 @@ bool Converters::convert(Buffer const * buffer, "tmpfile.out")); } - if (!checkAuth(conv, buffer ? buffer->absFileName() : string())) + bool use_shell_escape = false; + if (buffer && buffer->params().use_minted) { + // Check for minted listings insets + for (Paragraph const & par : buffer->paragraphs()) { + InsetList const & insets = par.insetList(); + pos_type lstpos = insets.find(LISTINGS_CODE, 0); + pos_type incpos = insets.find(INCLUDE_CODE, 0); + if (incpos >= 0) { + InsetInclude const * include = + static_cast<InsetInclude *> + (insets.get(incpos)); + if (include->params().getCmdName() != + "inputminted") { + incpos = -1; + } + } + if (lstpos >= 0 || incpos >= 0) { + use_shell_escape = true; + break; + } + } + } + if (!checkAuth(conv, buffer ? buffer->absFileName() : string(), + use_shell_escape)) return false; if (conv.latex()) { @@ -470,6 +529,8 @@ bool Converters::convert(Buffer const * buffer, command = subst(command, token_from, ""); command = subst(command, token_latex_encoding, buffer->params().encoding().latexName()); + if (use_shell_escape && !contains(command, "-shell-escape")) + command += " -shell-escape "; LYXERR(Debug::FILES, "Running " << command); if (!runLaTeX(*buffer, command, runparams, errorList)) return false; diff --git a/src/Converter.h b/src/Converter.h index 1ea73279b2..1963980c4f 100644 --- a/src/Converter.h +++ b/src/Converter.h @@ -193,8 +193,14 @@ public: /// able to execute arbitrary code, tagged with the 'needauth' option, /// authorization is: always denied if lyxrc.use_converter_needauth_forbidden /// is enabled; always allowed if the lyxrc.use_converter_needauth - /// is disabled; user is prompted otherwise - bool checkAuth(Converter const & conv, std::string const & doc_fname); + /// is disabled; user is prompted otherwise. + /// However, if use_shell_escape is true and a LaTeX backend is + /// going to be executed, both lyxrc.use_converter_needauth and + /// lyxrc.use_converter_needauth_forbidden are ignored, because in + /// this case the backend has to be executed and LyX will add the + /// -shell-escape option, so that user consent is always needed. + bool checkAuth(Converter const & conv, std::string const & doc_fname, + bool use_shell_escape = false); private: /// diff --git a/src/Session.cpp b/src/Session.cpp index 310aa74c9b..347426a0fe 100644 --- a/src/Session.cpp +++ b/src/Session.cpp @@ -35,6 +35,7 @@ string const sec_session = "[session info]"; string const sec_toolbars = "[toolbars]"; string const sec_lastcommands = "[last commands]"; string const sec_authfiles = "[auth files]"; +string const sec_shellescape = "[shell escape files]"; } // anon namespace @@ -422,6 +423,8 @@ void Session::readFile() lastCommands().read(is); else if (tmp == sec_authfiles) authFiles().read(is); + else if (tmp == sec_shellescape) + shellescapeFiles().read(is); else LYXERR(Debug::INIT, "LyX: Warning: unknown Session section: " << tmp); @@ -442,6 +445,7 @@ void Session::writeFile() const lastCommands().write(os); bookmarks().write(os); authFiles().write(os); + shellescapeFiles().write(os); } else LYXERR(Debug::INIT, "LyX: Warning: unable to save Session: " << session_file); @@ -480,4 +484,34 @@ void AuthFilesSection::write(ostream & os) const } +void ShellEscapeSection::read(istream & is) +{ + string s; + do { + char c = is.peek(); + if (c == '[') + break; + getline(is, s); + c = s[0]; + if (c == 0 || c == '#' || c == ' ' || !FileName::isAbsolute(s)) + continue; + + // read shellescape files + FileName const file(s); + if (file.exists() && !file.isDirectory()) + shellescape_files_.insert(s); + else + LYXERR(Debug::INIT, "LyX: Warning: Ignore shellescape file: " << s); + } while (is.good()); +} + + +void ShellEscapeSection::write(ostream & os) const +{ + os << '\n' << sec_shellescape << '\n'; + copy(shellescape_files_.begin(), shellescape_files_.end(), + ostream_iterator<std::string>(os, "\n")); +} + + } diff --git a/src/Session.h b/src/Session.h index f471f4d28e..1698ba3537 100644 --- a/src/Session.h +++ b/src/Session.h @@ -341,6 +341,27 @@ private: }; +class ShellEscapeSection : SessionSection +{ +public: + /// + explicit ShellEscapeSection() {}; + + /// + void read(std::istream & is); + + /// + void write(std::ostream & os) const; + + /// + std::set<std::string> & shellescapeFiles() { return shellescape_files_; } + +private: + /// set of document files authorized for external conversion + std::set<std::string> shellescape_files_; +}; + + class Session { public: @@ -373,6 +394,10 @@ public: AuthFilesSection & authFiles() { return auth_files; } /// AuthFilesSection const & authFiles() const { return auth_files; } + /// + ShellEscapeSection & shellescapeFiles() { return shellescape_files; } + /// + ShellEscapeSection const & shellescapeFiles() const { return shellescape_files; } private: friend class LyX; @@ -402,6 +427,8 @@ private: LastCommandsSection last_commands; /// AuthFilesSection auth_files; + /// + ShellEscapeSection shellescape_files; }; /// This is a singleton class. Get the instance. diff --git a/src/frontends/qt4/GuiView.cpp b/src/frontends/qt4/GuiView.cpp index a2ef74bf53..18debc73b6 100644 --- a/src/frontends/qt4/GuiView.cpp +++ b/src/frontends/qt4/GuiView.cpp @@ -50,6 +50,7 @@ #include "Format.h" #include "FuncStatus.h" #include "FuncRequest.h" +#include "InsetList.h" #include "Intl.h" #include "Layout.h" #include "Lexer.h" @@ -66,6 +67,8 @@ #include "Toolbars.h" #include "version.h" +#include "insets/InsetInclude.h" + #include "support/convert.h" #include "support/debug.h" #include "support/ExceptionMessage.h" @@ -2905,6 +2908,38 @@ bool GuiView::closeBuffer(Buffer & buf) // buffer, we can close or release the child buffers here too. bool success = true; if (!closing_) { + if (buf.params().use_minted && buf.isClean()) { + std::set<std::string> & shellescape_files = + theSession().shellescapeFiles().shellescapeFiles(); + std::set<std::string>::iterator it = + shellescape_files.find(buf.absFileName()); + if (it != shellescape_files.end()) { + // Check for minted listings insets + bool found_minted_listings = false; + for (Paragraph const & par : buf.paragraphs()) { + InsetList const & insets = par.insetList(); + pos_type lstpos = insets.find(LISTINGS_CODE, 0); + pos_type incpos = insets.find(INCLUDE_CODE, 0); + if (incpos >= 0) { + InsetInclude const * include = + static_cast<InsetInclude *> + (insets.get(incpos)); + if (include->params().getCmdName() + != "inputminted") { + incpos = -1; + } + } + if (lstpos >= 0 && incpos >= 0) { + found_minted_listings = true; + break; + } + } + // If the document doesn't use minted listings + // revoke permission to use shell-escape + if (!found_minted_listings) + shellescape_files.erase(it); + } + } ListOfBuffers clist = buf.getChildren(); ListOfBuffers::const_iterator it = clist.begin(); ListOfBuffers::const_iterator const bend = clist.end();
signature.asc
Description: PGP signature