[LyX/2.3.x] Security hardening backport for LyX 2.3.x
Pavel Sanda <[email protected]>
| Newsgroups | gmane.editors.lyx.cvs |
|---|---|
| Message-ID | <[email protected]> |
commit e1086ed96c483fd1463fb884a92bd20653821498 Author: Pavel Sanda <[email protected]> Date: Wed Jul 15 13:53:39 2026 +0200 Security hardening backport for LyX 2.3.x Accumulated backport of the coordinated LyX security release, encompassing Tier 00 variants. Folded hardening cases (per-case detail in the advisory): 00a kpsewhich filename -> shell command open/export -> exec 00b lyx2lyx invocation filename open -> exec 00c graphics filename extension -> os.system() open -> exec 00d \bibtex_command (preview + export) open/export -> exec 00e \index_command (whitelist + <> redirection) export -> exec 00g mangled graphics filename extension export -> exec 00h document basename -> conversion helpers open/import -> exec [DiD] 00i document basename backtick in "..." export -> exec 00k \paperwidth/\paperheight -> parsecmd redirect export -> file write 00de processing consent gate (biber/xindy/xindex) authorization guard 00h ships as defence-in-depth on 2.3: reviewed and applied, but the pre-2.4 proof-of-concept does not reproduce here (a working exploit is nonetheless likely - cold analysis found further sinks of the same class). The authorization gate is LyX's guard for tools that run document-embedded code under their default command; the real fixes are upstream (biber 2.22, xindex 1.07, coordinated TeX Live xindy update). The gate relaxes for backported biber 2.22. Assisted-by: Claude Opus 4.8 --- src/Buffer.cpp | 18 ++++- src/BufferParams.cpp | 26 ++++++- src/Converter.cpp | 32 +++++++- src/LaTeX.cpp | 150 ++++++++++++++++++++++++++++++++++++- src/LaTeX.h | 5 ++ src/graphics/GraphicsConverter.cpp | 9 ++- src/graphics/PreviewLoader.cpp | 7 +- src/support/FileName.cpp | 12 ++- src/support/filetools.cpp | 19 ++++- 9 files changed, 262 insertions(+), 16 deletions(-) diff --git a/src/Buffer.cpp b/src/Buffer.cpp index ec480f7584..877af07496 100644 --- a/src/Buffer.cpp +++ b/src/Buffer.cpp @@ -1333,12 +1333,22 @@ Buffer::ReadStatus Buffer::convertLyXFormat(FileName const & fn, // Run lyx2lyx: // $python$ "$lyx2lyx$" -t $LYX_FORMAT$ -o "$tempfile$" "$filetoread$" + + // guard against command expansion in filename strings on linux, + // keep " on windows + auto sh_quote = [](string const & s) -> string { +#ifdef _WIN32 + return quoteName(s); +#else + return '\'' + subst(s, "'", "'\\''") + '\''; +#endif + }; ostringstream command; command << os::python() - << ' ' << quoteName(lyx2lyx.toFilesystemEncoding()) + << ' ' << sh_quote(lyx2lyx.toFilesystemEncoding()) << " -t " << convert<string>(LYX_FORMAT) - << " -o " << quoteName(tmpfile.toSafeFilesystemEncoding()) - << ' ' << quoteName(fn.toSafeFilesystemEncoding()); + << " -o " << sh_quote(tmpfile.toSafeFilesystemEncoding()) + << ' ' << sh_quote(fn.toSafeFilesystemEncoding()); string const command_str = command.str(); LYXERR(Debug::INFO, "Running '" << command_str << '\''); @@ -4512,6 +4522,8 @@ Buffer::ExportStatus Buffer::doExport(string const & target, bool put_in_tempdir theFormats().extension(backend_format)); LYXERR(Debug::FILES, "filename=" << filename); + // (00i-wide) makeLatexName keep-set is now shell-safe; no re-sanitize here. + // Plain text backend if (backend_format == "text") { runparams.flavor = OutputParams::TEXT; diff --git a/src/BufferParams.cpp b/src/BufferParams.cpp index 51aab82aeb..adae2ee530 100644 --- a/src/BufferParams.cpp +++ b/src/BufferParams.cpp @@ -1021,8 +1021,18 @@ string BufferParams::readToken(Lexer & lex, string const & token, lcolor.setColor("boxbgcolor", color); } else if (token == "\\paperwidth") { lex >> paperwidth; + if (!paperwidth.empty() && !isValidLength(paperwidth)) { + lyxerr << "Rejecting non-Length \\paperwidth value: " + << paperwidth << endl; + paperwidth.clear(); + } } else if (token == "\\paperheight") { lex >> paperheight; + if (!paperheight.empty() && !isValidLength(paperheight)) { + lyxerr << "Rejecting non-Length \\paperheight value: " + << paperheight << endl; + paperheight.clear(); + } } else if (token == "\\leftmargin") { lex >> leftmargin; } else if (token == "\\topmargin") { @@ -3628,8 +3638,20 @@ string const BufferParams::getBibtexCommand(string const cmd, bool const warn) c string const BufferParams::bibtexCommand(bool const warn) const { // Return document-specific setting if available - if (bibtex_command != "default") - return getBibtexCommand(bibtex_command, warn); + if (bibtex_command != "default") { + + // Block redirection on the export bibtex call. + // Temporary hotfix, longterm solution needs structural + // split between program and options. + static char const * const SUSPECT_CHARS = "<>\"\\\t\n"; + if (bibtex_command.find_first_of(SUSPECT_CHARS) == string::npos) + return getBibtexCommand(bibtex_command, warn); + if (warn) + frontend::Alert::warning( + _("Requested bibliography command rejected"), + _("The bibliography processor command contains prohibited characters.")); + // fall through to the lyxrc-driven selection below + } // If we have "default" in document settings, consult the prefs // 1. Japanese (uses a specific processor) diff --git a/src/Converter.cpp b/src/Converter.cpp index 666e61c30b..6d754533d4 100644 --- a/src/Converter.cpp +++ b/src/Converter.cpp @@ -468,8 +468,35 @@ bool Converters::convert(Buffer const * buffer, && buffer->params().encoding().package() == Encoding::japanese; runparams.use_indices = buffer->params().use_indices; runparams.bibtex_command = buffer->params().bibtexCommand(true); - runparams.index_command = (buffer->params().index_command == "default") ? - string() : buffer->params().index_command; + + // Accept only programs from fixed known list + string accepted_index_cmd; + if (buffer->params().index_command != "default" + && !buffer->params().index_command.empty()) { + + // Do not allow redirection in index commands + bool const has_redirect = + buffer->params().index_command.find_first_of("<>") != string::npos; + if (!has_redirect) { + string supplied_prog; + split(buffer->params().index_command, supplied_prog, ' '); + for (auto const & alt : lyxrc.index_alternatives) { + string alt_prog; + split(alt, alt_prog, ' '); + if (!supplied_prog.empty() + && supplied_prog == alt_prog) { + accepted_index_cmd = buffer->params().index_command; + break; + } + } + } + if (accepted_index_cmd.empty()) + LYXERR0("Document-supplied index command '" + << buffer->params().index_command << "' is not a recognised " + "index processor; falling back to default."); + } + + runparams.index_command = accepted_index_cmd; runparams.document_language = buffer->params().language->babel(); runparams.only_childbibs = !buffer->params().useBiblatex() && !buffer->params().useBibtopic() @@ -810,6 +837,7 @@ bool Converters::runLaTeX(Buffer const & buffer, string const & command, string const name = buffer.latexName(); LaTeX latex(command, runparams, FileName(makeAbsPath(name)), buffer.filePath(), buffer.layoutPos(), + buffer.absFileName(), buffer.lastPreviewError()); TeXErrors terr; // The connection closes itself at the end of the scope when latex is diff --git a/src/LaTeX.cpp b/src/LaTeX.cpp index 4be0dad4ab..3febcb1df1 100644 --- a/src/LaTeX.cpp +++ b/src/LaTeX.cpp @@ -30,9 +30,14 @@ #include "support/Systemcall.h" #include "support/os.h" +#include "Session.h" + +#include "frontends/alert.h" + #include "support/regex.h" #include <fstream> +#include <map> #include <stack> @@ -58,6 +63,10 @@ docstring runMessage(unsigned int count) return bformat(_("Waiting for LaTeX run number %1$d"), count); } +bool isProcessorGated(std::string const & command); +bool checkProcessorAuth(std::string const & doc_fname, + std::string const & command); + } // namespace /* @@ -94,8 +103,9 @@ bool operator!=(AuxInfo const & a, AuxInfo const & o) LaTeX::LaTeX(string const & latex, OutputParams const & rp, FileName const & f, string const & p, string const & lp, - bool const clean_start) - : cmd(latex), file(f), path(p), lpath(lp), runparams(rp), biber(false) + string const & dfname, bool const clean_start) + : cmd(latex), file(f), path(p), lpath(lp), doc_fname(dfname), + runparams(rp), biber(false) { num_errors = 0; // lualatex can still produce a DVI with --output-format=dvi. However, @@ -455,6 +465,12 @@ bool LaTeX::runMakeIndex(string const & f, OutputParams const & runparams, if (!runparams.index_command.empty()) tmp = runparams.index_command; + // Gate the resolved index processor `tmp` that will actually run: + // meant for xindy/texindy/xindex, override or default. + if (isProcessorGated(tmp) + && !checkProcessorAuth(doc_fname, tmp)) + return false; + LYXERR(Debug::LATEX, "idx file has been made, running index processor (" << tmp << ") on file " << f); @@ -619,10 +635,140 @@ void LaTeX::updateBibtexDependencies(DepTable & dep, } +namespace { + +// One row per processor we can clear without gating: either a non-interpreter +// tool that is never dangerous (safe = true, no probe), or a code-capable tool +// at/above a version whose sinks are fixed (safe = false + version probe). +// +// A processor *absent* from this table is always gated (the default both for +// code-capable tools with no acceptable version yet - xindy, xindex - and as a +// fail-safe). +struct RequiredProcessor { + char const * prog; // first-token basename to match + bool safe; // true = not code-capable (makeindex-class): + // never gate, skip the version probe + char const * version_arg; // argument that prints the version + char const * version_re; // regex capturing (major)(minor) + int min_major; // minimum version not requiring the gate + int min_minor; +}; + +RequiredProcessor const required_processors[] = { + // Non-interpreter index processors (makeindex-class): they cannot execute + // document-controlled code, so never gate them and skip the probe. + { "makeindex", true, nullptr, nullptr, 0, 0 }, + { "upmendex", true, nullptr, nullptr, 0, 0 }, + // biber: code-capable; fixed upstream at 2.22 + { "biber", false, "--version", "version:\\s*([0-9]+)\\.([0-9]+)", 2, 22 }, +}; + +// False only when >= required version. +// True for an unknown tool, an unparseable version, or a failed probe a +// Caches one `--version` probe per processor per session. +bool isProcessorGated(string const & command) +{ + string prog; + split(command, prog, ' '); // first whitespace token only + prog = onlyFileName(prog); // strip any directory part + if (prog.empty()) + return true; + + static map<string, bool> cache; + map<string, bool>::const_iterator const it = cache.find(prog); + if (it != cache.end()) + return it->second; + + bool gated = true; // fail-safe default + for (RequiredProcessor const & p : required_processors) { + if (prog != p.prog) + continue; + if (p.safe) { // non-interpreter: never gate, no probe + gated = false; + break; + } + //safe because prog was matched against the table + cmd_ret const r = + runCommand(quoteName(prog) + ' ' + p.version_arg); + smatch m; + regex const re(p.version_re); + if (r.first == 0 && regex_search(r.second, m, re)) { + int const maj = convert<int>(m.str(1)); + int const min = convert<int>(m.str(2)); + gated = maj < p.min_major + || (maj == p.min_major && min < p.min_minor); + } + break; // matched the table row + } + cache[prog] = gated; + return gated; +} + +// Per-document trust gate; this only handles consent. +// +// Deliberately reuses Converters::checkAuth's machinery so the trust +// decision is shared: the same per-document authorization set +// (theSession().authFiles()), the same global prompt switch +// (lyxrc.use_converter_needauth), and the same persisted "Always run for this +// document". A document trusted for a needauth converter is therefore also +// trusted here, and vice versa - one "do you trust this document?" decision. +// +// Unlike checkAuth it does NOT honour use_converter_needauth_forbidden: that +// pref defaults to "forbid", which is correct for the rare hand-flagged +// needauth converters but would block biber on *every* biblatex document and +// xindy/xindex on every indexed one. Gating here is consent, not a +// hard-deny master switch. +// +// Returns true if the processor may run. +bool checkProcessorAuth(string const & doc_fname, string const & command) +{ + if (!lyxrc.use_converter_needauth) + return true; + + docstring const title = + _("A LaTeX backend requires your authorization"); + docstring const warning = bformat( + _("<p>The following LaTeX backend has been requested " + "to allow execution of external programs:</p>" + "<center><p>%1$s</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("<tt>" + command + "</tt>")) + + _("<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>"); + + // No document identity (preview, clone, import): cannot persist a + // per-document decision, so prompt without the "Always" option. + if (doc_fname.empty()) + return frontend::Alert::prompt(title, warning, 0, 0, + _("Do ¬ allow"), _("A&llow")) != 0; + + if (theSession().authFiles().find(doc_fname)) + return true; + + int const choice = frontend::Alert::prompt(title, warning, 0, 0, + _("Do ¬ allow"), _("A&llow"), + _("&Always allow for this document")); + if (choice == 2) + theSession().authFiles().insert(doc_fname); + return choice != 0; +} + +} // namespace + + bool LaTeX::runBibTeX(vector<AuxInfo> const & bibtex_info, OutputParams const & runparams) { bool result = false; + + // Old biber is not safe. Plain bibtex is safe. + if (biber && isProcessorGated(runparams.bibtex_command) + && !checkProcessorAuth(doc_fname, runparams.bibtex_command)) + return false; + for (vector<AuxInfo>::const_iterator it = bibtex_info.begin(); it != bibtex_info.end(); ++it) { if (!biber && it->databases.empty()) diff --git a/src/LaTeX.h b/src/LaTeX.h index 0b46c607af..012c5a0759 100644 --- a/src/LaTeX.h +++ b/src/LaTeX.h @@ -162,6 +162,7 @@ public: support::FileName const & file, std::string const & path = empty_string(), std::string const & lpath = empty_string(), + std::string const & doc_fname = empty_string(), bool const clean_start = false); /// runs LaTeX several times @@ -231,6 +232,10 @@ private: /// Extra path, possibly relative to the document directory path. std::string lpath; + /// Absolute name for unique cache record in the trust gate. + /// Shared with Converters::checkAuth. + std::string doc_fname; + /// used by scanLogFile int num_errors; diff --git a/src/graphics/GraphicsConverter.cpp b/src/graphics/GraphicsConverter.cpp index 598e108b73..b7a59b97ac 100644 --- a/src/graphics/GraphicsConverter.cpp +++ b/src/graphics/GraphicsConverter.cpp @@ -30,6 +30,8 @@ #include "support/TempFile.h" #include <sstream> +#include <algorithm> +#include <cctype> #include <fstream> using namespace std; @@ -302,7 +304,12 @@ static void build_script(string const & doc_fname, theConverters().getPath(from_format, to_format); // Create a temporary base file-name for all intermediate steps. - string const from_ext = getExtension(from_file); + // The extension string is user-controlled. Avoid metacharacters + // to prevent havoc down the pipeline. + string from_ext = getExtension(from_file); + from_ext.erase(remove_if(from_ext.begin(), from_ext.end(), + [](unsigned char c){ return !(isalnum(c) || c == '_' || c == '-'); }), + from_ext.end()); TempFile tempfile(addExtension("gconvertXXXXXX", from_ext)); tempfile.setAutoRemove(false); string outfile = tempfile.name().toFilesystemEncoding(); diff --git a/src/graphics/PreviewLoader.cpp b/src/graphics/PreviewLoader.cpp index 0d65ccf507..2714ed9254 100644 --- a/src/graphics/PreviewLoader.cpp +++ b/src/graphics/PreviewLoader.cpp @@ -718,7 +718,12 @@ void PreviewLoader::Impl::startLoading(bool wait) } cs << latexparam; - cs << " --bibtex=" << quoteName(buffer_.params().bibtexCommand()); + + // --bibtex= allows document-controlled arbitrary code + // execution in lyxpreview_tools.py. Tradeoff when disabling + // it is unresolved citations inside math/ERT preview. + //cs << " --bibtex=" << quoteName(buffer_.params().bibtexCommand()); + if (buffer_.params().bufferFormat() == "lilypond-book") cs << " --lilypond"; diff --git a/src/support/FileName.cpp b/src/support/FileName.cpp index 482bb93e21..8c3b2e8dfc 100644 --- a/src/support/FileName.cpp +++ b/src/support/FileName.cpp @@ -972,14 +972,20 @@ string DocFileName::mangledFileName(string const & dir) const // are forbidden: '/', '.', ' ', and ':'. // On windows it is not possible to create files with '<', '>' or '?' // in the name. + // We forbid ';', '=' as they coudl become active in shell. static string const keep = "abcdefghijklmnopqrstuvwxyz" "ABCDEFGHIJKLMNOPQRSTUVWXYZ" - "+-0123456789;="; + "+-0123456789"; string::size_type pos = 0; while ((pos = mname.find_first_not_of(keep, pos)) != string::npos) mname[pos++] = '_'; - // Add the extension back on - mname = support::changeExtension(mname, getExtension(name)); + // Add the extension back on, but sanitize from metachars, + // it's user-controlled string. + string ext = getExtension(name); + pos = 0; + while ((pos = ext.find_first_not_of(keep, pos)) != string::npos) + ext[pos++] = '_'; + mname = support::changeExtension(mname, ext); // Prepend a counter to the filename. This is necessary to make // the mangled name unique. diff --git a/src/support/filetools.cpp b/src/support/filetools.cpp index f2697f784b..073d0511f8 100644 --- a/src/support/filetools.cpp +++ b/src/support/filetools.cpp @@ -206,7 +206,7 @@ FileName const makeLatexName(FileName const & file) // a non-latin world out there... string const keep = "abcdefghijklmnopqrstuvwxyz" "ABCDEFGHIJKLMNOPQRSTUVWXYZ" - "@!'()*+,-./0123456789:;<=>?[]`|"; + "0123456789+-._,@"; string::size_type pos = 0; while ((pos = name.find_first_not_of(keep, pos)) != string::npos) @@ -1164,7 +1164,22 @@ FileName const findtexfile(string const & fil, string const & /*format*/, // tfm - TFMFONTS, TEXFONTS // This means that to use kpsewhich in the best possible way we // should help it by setting additional path in the approp. envir.var. - string const kpsecmd = "kpsewhich " + fil; + + if (fil.empty()) + return FileName(); + + // Wrap fil in the shell's quoting form that disables the relevant + // metacharacter set. +#ifdef _WIN32 + // Reject '"' in filename, can't be backslashed & forbidden by NTFS anyway + if (fil.find('"') != string::npos) + return FileName(); + // disable metacharacters + string const kpsecmd = "kpsewhich -- \"" + fil + "\""; +#else + // disable metacharacters & escape existing ' + string const kpsecmd = "kpsewhich -- '" + subst(fil, "'", "'\\''") + "'"; +#endif cmd_ret const c = runCommand(kpsecmd); -- lyx-cvs mailing list [email protected] https://lists.lyx.org/mailman/listinfo/lyx-cvs