>From 7b2c712e942d418c06a75dc8a982975702aabce6 Mon Sep 17 00:00:00 2001 From: Pavel Sanda Date: Wed, 15 Jul 2026 13:52:29 +0200 Subject: [PATCH] Security hardening backport for LyX 2.5.x Accumulated backport of the coordinated LyX security release for distros pinned to an already-released 2.5.x that will not rebuild to 2.5.2. 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 00i document basename backtick in "..." export -> exec 00k \paperwidth/\paperheight -> parsecmd redirect export -> file write 00de processing consent gate (biber/xindy/xindex) authorization guard 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. Not included: the xindex version-check add-on (ships in the 2.5.2 release and master only). Assisted-by: Claude Opus 4.8 --- src/Buffer.cpp | 19 ++++- src/BufferParams.cpp | 26 ++++++- src/Converter.cpp | 31 +++++++- src/LaTeX.cpp | 152 ++++++++++++++++++++++++++++++++++++- src/LaTeX.h | 5 ++ src/graphics/GraphicsConverter.cpp | 9 ++- src/graphics/PreviewLoader.cpp | 7 +- src/support/FileName.cpp | 12 +-- src/support/filetools.cpp | 22 +++++- 9 files changed, 263 insertions(+), 20 deletions(-) diff --git a/src/Buffer.cpp b/src/Buffer.cpp index 2ea6125e06..ea1934903a 100644 --- a/src/Buffer.cpp +++ b/src/Buffer.cpp @@ -1402,12 +1402,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(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 << '\''); @@ -4641,6 +4651,9 @@ Buffer::ExportStatus Buffer::doExport(string const & target, bool put_in_tempdir filename = addName(temppath(), filename); filename = changeExtension(filename, theFormats().extension(backend_format)); + + // (00i-wide) makeLatexName keep-set is now shell-safe; no re-sanitize here. + LYXERR(Debug::FILES, "filename=" << filename); // Plain text backend diff --git a/src/BufferParams.cpp b/src/BufferParams.cpp index cb14f20aba..ffbb3fc203 100644 --- a/src/BufferParams.cpp +++ b/src/BufferParams.cpp @@ -1362,10 +1362,20 @@ string BufferParams::readToken(Lexer & lex, string const & token, } if (token == "\\paperwidth") { lex >> paperwidth; + if (!paperwidth.empty() && !isValidLength(paperwidth)) { + lyxerr << "Rejecting non-Length \\paperwidth value: " + << paperwidth << endl; + paperwidth.clear(); + } break; } if (token == "\\paperheight") { lex >> paperheight; + if (!paperheight.empty() && !isValidLength(paperheight)) { + lyxerr << "Rejecting non-Length \\paperheight value: " + << paperheight << endl; + paperheight.clear(); + } break; } if (token == "\\leftmargin") { @@ -4498,8 +4508,20 @@ string const BufferParams::getBibtexCommand(string const & cmd, bool const warn) 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 de2f955aa8..cdf52c027c 100644 --- a/src/Converter.cpp +++ b/src/Converter.cpp @@ -497,8 +497,34 @@ Converters::RetVal Converters::convert(Buffer const * buffer, && bp.encoding().package() == Encoding::japanese; runparams.use_indices = bp.use_indices; runparams.bibtex_command = bp.bibtexCommand(true); - runparams.index_command = (bp.index_command == "default") ? - string() : bp.index_command; + + // Accept only programs from fixed known list + string accepted_index_cmd; + if (bp.index_command != "default" && !bp.index_command.empty()) { + + // Do not allow redirection in index commands + bool const has_redirect = + bp.index_command.find_first_of("<>") != string::npos; + if (!has_redirect) { + string supplied_prog; + split(bp.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 = bp.index_command; + break; + } + } + } + if (accepted_index_cmd.empty()) + LYXERR0("Document-supplied index command '" + << bp.index_command << "' is not a recognised " + "index processor; falling back to default."); + } + + runparams.index_command = accepted_index_cmd; runparams.document_language = bp.language->lang(); // Some macros rely on font encoding runparams.main_fontenc = bp.main_font_encoding(); @@ -902,6 +928,7 @@ Converters::RetVal Converters::runLaTeX(Buffer const & buffer, string const & co string const name = buffer.latexName(); LaTeX latex(command, runparams, makeAbsPath(name), buffer.filePath(), buffer.layoutPos(), + buffer.absFileName(), buffer.isClone(), buffer.freshStartRequired()); 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 e22dd1eac8..1e42ca2d18 100644 --- a/src/LaTeX.cpp +++ b/src/LaTeX.cpp @@ -27,6 +27,9 @@ #include "Encoding.h" #include "Language.h" #include "LaTeXFeatures.h" +#include "Session.h" + +#include "frontends/alert.h" #include "support/debug.h" #include "support/docstring.h" @@ -39,6 +42,7 @@ #include "support/os.h" #include +#include #include #include @@ -65,6 +69,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 /* @@ -123,10 +131,11 @@ 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, + FileName const & f, string const & p, string const & lp, + string const & dfname, bool allow_cancellation, bool const clean_start) - : cmd(latex), file(f), path(p), lpath(lp), runparams(rp), biber(false), - allow_cancel(allow_cancellation) + : cmd(latex), file(f), path(p), lpath(lp), doc_fname(dfname), + runparams(rp), biber(false), allow_cancel(allow_cancellation) { num_errors = 0; // lualatex can still produce a DVI with --output-format=dvi. However, @@ -598,6 +607,12 @@ int LaTeX::runMakeIndex(string const & f, OutputParams const & rp, if (!rp.index_command.empty()) tmp = rp.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 Systemcall::KILLED; + Language const * doc_lang = languages.getLanguage(rp.document_language); if (contains(tmp, "$$x")) { @@ -803,11 +818,142 @@ 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 cache; + map::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.valid && regex_search(r.result, m, re)) { + int const maj = convert(m.str(1)); + int const min = convert(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( + _("

The following LaTeX backend has been requested " + "to allow execution of external programs:

" + "

%1$s

" + "

The external programs can execute arbitrary commands on " + "your system, including dangerous ones, if instructed to do " + "so by a maliciously crafted LyX document.

"), + from_utf8("" + command + "")) + + _("

Should LaTeX backends be allowed to run external " + "programs?

Allow them only if you trust the " + "origin/sender of the LyX document!

"); + + // 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 const & bibtex_info, OutputParams const & rp, int & exit_code) { bool result = false; exit_code = 0; + + // Old biber is not safe. Plain bibtex is safe. + if (biber && isProcessorGated(rp.bibtex_command) + && !checkProcessorAuth(doc_fname, rp.bibtex_command)) { + exit_code = Systemcall::KILLED; + return false; + } for (vector::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 185b9ebdd9..d9a2e00acd 100644 --- a/src/LaTeX.h +++ b/src/LaTeX.h @@ -177,6 +177,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 allow_cancellation = false, bool const clean_start = false); @@ -250,6 +251,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 0436f4c634..47c1445a0d 100644 --- a/src/graphics/GraphicsConverter.cpp +++ b/src/graphics/GraphicsConverter.cpp @@ -25,6 +25,8 @@ #include "support/TempFile.h" #include +#include +#include #include using namespace std; @@ -301,7 +303,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 25dcb2fd61..7824270cce 100644 --- a/src/graphics/PreviewLoader.cpp +++ b/src/graphics/PreviewLoader.cpp @@ -685,7 +685,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 e49f76e90c..2b8e0243f3 100644 --- a/src/support/FileName.cpp +++ b/src/support/FileName.cpp @@ -990,11 +990,11 @@ string DocFileName::mangledFileName(string const & dir, bool encrypt_path) const // xHTML route // we use hash instead of counter to get stable filenames in export directory if (encrypt_path) { - // sanitization probably not neccessary for xhtml, but won't harm string sanfn = support::changeExtension(onlyFileName(), string()); sanfn = sanitizeFileName(sanfn); - // Add the extension back on - sanfn = support::changeExtension(sanfn, getExtension(onlyFileName())); + // extension is user-controlled string, suppress metacharacters + sanfn = support::changeExtension(sanfn, + sanitizeFileName(getExtension(onlyFileName()))); //various filesystems have filename limit around 2^8 if (sanfn.length() > 230) @@ -1014,8 +1014,10 @@ string DocFileName::mangledFileName(string const & dir, bool encrypt_path) const mname = support::changeExtension(name, string()); // The mangled name must be a valid LaTeX name. mname = sanitizeFileName(mname); - // 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 + mname = support::changeExtension(mname, + sanitizeFileName(getExtension(name))); // Prepend a counter to the filename. This is necessary to make // the mangled name unique, see truncation below. diff --git a/src/support/filetools.cpp b/src/support/filetools.cpp index e0dc4e7654..e01d87cc00 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) @@ -1227,7 +1227,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); @@ -1363,9 +1378,10 @@ std::string sanitizeFileName(const std::string & str) // are forbidden: '/', '.', ' ', and ':'. // On windows it is not possible to create files with '<', '>' or '?' // in the name. + // We forbid ';', '=' as they could become active in shell. static std::string const keep = "abcdefghijklmnopqrstuvwxyz" "ABCDEFGHIJKLMNOPQRSTUVWXYZ" - "+-0123456789;="; + "+-0123456789"; std::string name = str; string::size_type pos = 0;