[LyX/2.4.x] Security hardening backport for LyX 2.4.x
Pavel Sanda <[email protected]>
| Newsgroups | gmane.editors.lyx.cvs |
|---|---|
| Message-ID | <[email protected]> |
commit 10e012639c9e53a8fdf7740e083aed99c0c6fadd Author: Pavel Sanda <[email protected]> Date: Wed Jul 15 13:51:00 2026 +0200 Security hardening backport for LyX 2.4.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 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 biber reporting version >= 2.22. 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 73de572eb0..bca4a9beef 100644 --- a/src/Buffer.cpp +++ b/src/Buffer.cpp @@ -1398,12 +1398,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 << '\''); @@ -4595,6 +4605,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 dcd539dfd1..04ae90c97e 100644 --- a/src/BufferParams.cpp +++ b/src/BufferParams.cpp @@ -1114,8 +1114,18 @@ string BufferParams::readToken(Lexer & lex, string const & token, isboxbgcolor = true; } 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") { @@ -3828,8 +3838,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 41bd8ce1a0..3c4130ebf1 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(); @@ -901,6 +927,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 3225867ac2..9ff5bf8a02 100644 --- a/src/LaTeX.cpp +++ b/src/LaTeX.cpp @@ -26,6 +26,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" @@ -38,6 +41,7 @@ #include "support/os.h" #include <fstream> +#include <map> #include <regex> #include <stack> @@ -64,6 +68,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 /* @@ -108,10 +116,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, @@ -548,6 +557,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")) { @@ -753,11 +768,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<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.valid && regex_search(r.result, 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 & 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<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 569114d369..e973300d12 100644 --- a/src/LaTeX.h +++ b/src/LaTeX.h @@ -178,6 +178,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); @@ -251,6 +252,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 34d37a4066..31e62e89c7 100644 --- a/src/graphics/GraphicsConverter.cpp +++ b/src/graphics/GraphicsConverter.cpp @@ -26,6 +26,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 be76444df5..a8b9d229cf 100644 --- a/src/graphics/PreviewLoader.cpp +++ b/src/graphics/PreviewLoader.cpp @@ -686,7 +686,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 864f38770c..7549a92283 100644 --- a/src/support/FileName.cpp +++ b/src/support/FileName.cpp @@ -996,11 +996,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) @@ -1020,8 +1020,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 954e68d04b..7b6d189218 100644 --- a/src/support/filetools.cpp +++ b/src/support/filetools.cpp @@ -209,7 +209,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) @@ -1230,7 +1230,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); @@ -1366,9 +1381,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; -- lyx-cvs mailing list [email protected] https://lists.lyx.org/mailman/listinfo/lyx-cvs