On Tue, Oct 06, 2026 at 11:30:38AM -0400, Richard Kimberly Heck wrote: > On 10/6/26 3:15 AM, Pavel Sanda wrote: > > On Mon, Sep 28, 2026 at 10:14:16PM +0200, Pavel Sanda wrote: > > > I'll keep posting next pacthes for various issues, but should not > > The next case handles git/svn invocation argument smuggling. > > Looks good. Nothing we can do about cvs and rcs?
Good that you pushed. rcs can't, but cvs can actually handle the same. After some play, there is little bit more dangerous workaround for rcs as well - adding ./ for the processed file will clearly separate the filename from -args. Attaching as a separate patch, as the chance that I screewed something with this change is higher (though it was tested). We'll see, updated patches below ;) Pavel
>From 95f295d226ff874f346e441231869f2a75090548 Mon Sep 17 00:00:00 2001 From: Pavel Sanda <[email protected]> Date: Sun, 11 Oct 2026 21:55:36 +0200 Subject: [PATCH] VCS: block argument smuggling Insert " -- " between every git, svn and cvs subcommand and the filename arguments that may originate from attacker-controlled paths. Excluded: * git describe - no filename argument * git show <rev>:./<file> - the argv element starts with the revision, not '-'; '--' is not supported in this syntax * RCS - has no functional --, handled separately Tier 01 DiD / vcs-args: -- separator for VCS filename arguments Assisted-by: Claude Opus 5 --- src/VCBackend.cpp | 86 +++++++++++++++++++++++------------------------ 1 file changed, 43 insertions(+), 43 deletions(-) diff --git a/src/VCBackend.cpp b/src/VCBackend.cpp index 751529c9be..897c35489f 100644 --- a/src/VCBackend.cpp +++ b/src/VCBackend.cpp @@ -612,7 +612,7 @@ bool CVS::retrieve(FileName const & file) { LYXERR(Debug::LYXVC, "LyXVC::CVS: retrieve.\n\t" << file); // The caller ensures that file does not exist, so no need to check that. - return doVCCommandCall("cvs -q update " + quoteName(file.toFilesystemEncoding()), + return doVCCommandCall("cvs -q update -- " + quoteName(file.toFilesystemEncoding()), file.onlyPath()) == 0; } @@ -679,7 +679,7 @@ CVS::CvsStatus CVS::getStatus() return StatusError; } - if (doVCCommandCallWithOutput("cvs status " + getTarget(File), + if (doVCCommandCallWithOutput("cvs status -- " + getTarget(File), FileName(owner_->filePath()), tmpf)) { return StatusError; } @@ -720,7 +720,7 @@ void CVS::getRevisionInfo() } int rc = doVCCommandCallWithOutput("cvs log -r" + version_ - + ' ' + getTarget(File), + + " -- " + getTarget(File), FileName(owner_->filePath()), tmpf); if (rc) { LYXERR(Debug::LYXVC, "cvs log failed with exit code " << rc); @@ -756,7 +756,7 @@ void CVS::getRevisionInfo() void CVS::registrer(string const & msg) { - doVCCommand("cvs -q add -m \"" + msg + "\" " + doVCCommand("cvs -q add -m \"" + msg + "\" -- " + getTarget(File), FileName(owner_->filePath())); } @@ -775,7 +775,7 @@ string CVS::rename(support::FileName const & newFile, string const & msg) string ret = copy(newFile, msg); if (ret.empty()) return ret; - string cmd = "cvs -q remove -m \"" + msg + "\" " + + string cmd = "cvs -q remove -m \"" + msg + "\" -- " + quoteName(oldFile.onlyFileName()); FileName path(oldFile.onlyPath()); return doVCCommand(cmd, path) ? string() : ret; @@ -796,14 +796,14 @@ string CVS::copy(support::FileName const & newFile, string const & msg) return string(); FileName path(oldFile.onlyPath()); string relFile(to_utf8(newFile.relPath(path.absFileName()))); - string cmd("cvs -q add -m \"" + msg + "\" " + quoteName(relFile)); + string cmd("cvs -q add -m \"" + msg + "\" -- " + quoteName(relFile)); return doVCCommand(cmd, path) ? string() : "CVS: Proceeded"; } void CVS::getDiff(OperationMode opmode, FileName const & tmpf) { - doVCCommandWithOutput("cvs diff " + getTarget(opmode), + doVCCommandWithOutput("cvs diff -- " + getTarget(opmode), FileName(owner_->filePath()), tmpf, false); } @@ -811,7 +811,7 @@ void CVS::getDiff(OperationMode opmode, FileName const & tmpf) int CVS::edit() { vcstatus_ = LOCKED; - return doVCCommand("cvs -q edit " + getTarget(File), + return doVCCommand("cvs -q edit -- " + getTarget(File), FileName(owner_->filePath())); } @@ -819,14 +819,14 @@ int CVS::edit() int CVS::unedit() { vcstatus_ = UNLOCKED; - return doVCCommand("cvs -q unedit " + getTarget(File), + return doVCCommand("cvs -q unedit -- " + getTarget(File), FileName(owner_->filePath())); } int CVS::update(OperationMode opmode, FileName const & tmpf) { - return doVCCommandWithOutput("cvs -q update " + return doVCCommandWithOutput("cvs -q update -- " + getTarget(opmode), FileName(owner_->filePath()), tmpf, false); } @@ -864,7 +864,7 @@ LyXVC::CommandResult CVS::checkIn(string const & msg, string & log) return LyXVC::VCSuccess; case LocallyModified: case LocallyAdded: { - int rc = doVCCommand("cvs -q commit -m \"" + msg + "\" " + int rc = doVCCommand("cvs -q commit -m \"" + msg + "\" -- " + getTarget(File), FileName(owner_->filePath())); if (rc) @@ -1082,7 +1082,7 @@ bool CVS::undoLastEnabled() void CVS::getLog(FileName const & tmpf) { - doVCCommandWithOutput("cvs log " + getTarget(File), + doVCCommandWithOutput("cvs log -- " + getTarget(File), FileName(owner_->filePath()), tmpf); } @@ -1129,7 +1129,7 @@ bool CVS::prepareFileRevision(string const & revis, string & f) return false; } - doVCCommandWithOutput("cvs update -p -r" + rev + ' ' + doVCCommandWithOutput("cvs update -p -r" + rev + " -- " + getTarget(File), FileName(owner_->filePath()), tmpf); tmpf.refresh(); @@ -1173,7 +1173,7 @@ bool SVN::findFile(FileName const & file) // Now we check the status of the file. string const fname = onlyFileName(file.absFileName()); LYXERR(Debug::LYXVC, "LyXVC: Checking if file is under svn control for `" << fname << '\''); - bool found = 0 == doVCCommandCall("svn info " + quoteName(fname), + bool found = 0 == doVCCommandCall("svn info -- " + quoteName(fname), file.onlyPath()); LYXERR(Debug::LYXVC, "SVN control: " << (found ? "enabled" : "disabled")); return found; @@ -1200,7 +1200,7 @@ bool SVN::checkLockMode() } LYXERR(Debug::LYXVC, "Detecting locking mode..."); - if (doVCCommandCall("svn proplist " + quoteName(onlyFileName(owner_->absFileName())) + if (doVCCommandCall("svn proplist -- " + quoteName(onlyFileName(owner_->absFileName())) + " > " + quoteName(tmpf.toFilesystemEncoding()), FileName(owner_->filePath()))) return false; @@ -1235,14 +1235,14 @@ bool SVN::retrieve(FileName const & file) { LYXERR(Debug::LYXVC, "LyXVC::SVN: retrieve.\n\t" << file); // The caller ensures that file does not exist, so no need to check that. - return doVCCommandCall("svn update -q --non-interactive " + quoteName(file.onlyFileName()), + return doVCCommandCall("svn update -q --non-interactive -- " + quoteName(file.onlyFileName()), file.onlyPath()) == 0; } void SVN::registrer(string const & /*msg*/) { - doVCCommand("svn add -q --parents " + quoteName(onlyFileName(owner_->absFileName())), + doVCCommand("svn add -q --parents -- " + quoteName(onlyFileName(owner_->absFileName())), FileName(owner_->filePath())); } @@ -1260,10 +1260,10 @@ string SVN::rename(support::FileName const & newFile, string const & msg) // confusing to the user to have two uncommitted files. FileName path(owner_->filePath()); string relFile(to_utf8(newFile.relPath(path.absFileName()))); - string cmd("svn move -q " + quoteName(onlyFileName(owner_->absFileName())) + + string cmd("svn move -q -- " + quoteName(onlyFileName(owner_->absFileName())) + ' ' + quoteName(relFile)); if (doVCCommand(cmd, path)) { - cmd = "svn revert -q " + + cmd = "svn revert -q -- " + quoteName(onlyFileName(owner_->absFileName())) + ' ' + quoteName(relFile); doVCCommand(cmd, path); @@ -1276,7 +1276,7 @@ string SVN::rename(support::FileName const & newFile, string const & msg) f.push_back(newFile); string log; if (checkIn(f, msg, log) != LyXVC::VCSuccess) { - cmd = "svn revert -q " + + cmd = "svn revert -q -- " + quoteName(onlyFileName(owner_->absFileName())) + ' ' + quoteName(relFile); doVCCommand(cmd, path); @@ -1301,7 +1301,7 @@ string SVN::copy(support::FileName const & newFile, string const & msg) // confusing to the user to have an uncommitted file. FileName path(owner_->filePath()); string relFile(to_utf8(newFile.relPath(path.absFileName()))); - string cmd("svn copy -q " + quoteName(onlyFileName(owner_->absFileName())) + + string cmd("svn copy -q -- " + quoteName(onlyFileName(owner_->absFileName())) + ' ' + quoteName(relFile)); if (doVCCommand(cmd, path)) return string(); @@ -1380,7 +1380,7 @@ bool SVN::isCheckInWithConfirmation() return true; } - doVCCommandCall("svn diff " + quoteName(owner_->absFileName()) + doVCCommandCall("svn diff -- " + quoteName(owner_->absFileName()) + " > " + quoteName(tmpf.toFilesystemEncoding()), FileName(owner_->filePath())); @@ -1426,7 +1426,7 @@ bool SVN::fileLock(bool lock, FileName const & tmpf, string &status) if (!locked_mode_ || (isLocked() == lock)) return true; - string const arg = lock ? "lock " : "unlock "; + string const arg = lock ? "lock -- " : "unlock -- "; doVCCommand("svn "+ arg + quoteName(onlyFileName(owner_->absFileName())) + " > " + quoteName(tmpf.toFilesystemEncoding()), FileName(owner_->filePath())); @@ -1466,7 +1466,7 @@ string SVN::checkOut() return N_("Error: Could not generate logfile."); } - doVCCommand("svn update --non-interactive " + quoteName(onlyFileName(owner_->absFileName())) + doVCCommand("svn update --non-interactive -- " + quoteName(onlyFileName(owner_->absFileName())) + " > " + quoteName(tmpf.toFilesystemEncoding()), FileName(owner_->filePath())); @@ -1503,7 +1503,7 @@ string SVN::repoUpdate() return N_("Error: Could not generate logfile."); } - doVCCommand("svn diff " + quoteName(owner_->filePath()) + doVCCommand("svn diff -- " + quoteName(owner_->filePath()) + " > " + quoteName(tmpf.toFilesystemEncoding()), FileName(owner_->filePath())); docstring res = tmpf.fileContents("UTF-8"); @@ -1532,7 +1532,7 @@ string SVN::repoUpdate() // + " > " + quoteName(tmpf.toFilesystemEncoding()), // FileName(owner_->filePath())); // res = "Revert log:\n" + tmpf.fileContents("UTF-8"); - doVCCommand("svn update --accept mine-full " + quoteName(owner_->filePath()) + doVCCommand("svn update --accept mine-full -- " + quoteName(owner_->filePath()) + " > " + quoteName(tmpf.toFilesystemEncoding()), FileName(owner_->filePath())); res += "Update log:\n" + tmpf.fileContents("UTF-8"); @@ -1557,7 +1557,7 @@ string SVN::lockingToggle() return N_("Error: Could not generate logfile."); } - int ret = doVCCommand("svn proplist " + quoteName(onlyFileName(owner_->absFileName())) + int ret = doVCCommand("svn proplist -- " + quoteName(onlyFileName(owner_->absFileName())) + " > " + quoteName(tmpf.toFilesystemEncoding()), FileName(owner_->filePath())); if (ret) @@ -1567,12 +1567,12 @@ string SVN::lockingToggle() string res = scanLogFile(tmpf, log); bool locking = contains(res, "svn:needs-lock"); if (!locking) - ret = doVCCommand("svn propset svn:needs-lock ON " + ret = doVCCommand("svn propset svn:needs-lock ON -- " + quoteName(onlyFileName(owner_->absFileName())) + " > " + quoteName(tmpf.toFilesystemEncoding()), FileName(owner_->filePath())); else - ret = doVCCommand("svn propdel svn:needs-lock " + ret = doVCCommand("svn propdel svn:needs-lock -- " + quoteName(onlyFileName(owner_->absFileName())) + " > " + quoteName(tmpf.toFilesystemEncoding()), FileName(owner_->filePath())); @@ -1601,7 +1601,7 @@ bool SVN::revert() // gets the updated version from the repository. string const fil = quoteName(onlyFileName(owner_->absFileName())); - if (doVCCommand("svn revert -q " + fil, + if (doVCCommand("svn revert -q -- " + fil, FileName(owner_->filePath()))) return false; owner_->markClean(); @@ -1676,7 +1676,7 @@ bool SVN::getFileRevisionInfo() return false; } - doVCCommand("svn info --xml " + quoteName(onlyFileName(owner_->absFileName())) + doVCCommand("svn info --xml -- " + quoteName(onlyFileName(owner_->absFileName())) + " > " + quoteName(tmpf.toFilesystemEncoding()), FileName(owner_->filePath())); @@ -1748,7 +1748,7 @@ bool SVN::getTreeRevisionInfo() void SVN::getLog(FileName const & tmpf) { - doVCCommand("svn log " + quoteName(onlyFileName(owner_->absFileName())) + doVCCommand("svn log -- " + quoteName(onlyFileName(owner_->absFileName())) + " > " + quoteName(tmpf.toFilesystemEncoding()), FileName(owner_->filePath())); } @@ -1781,7 +1781,7 @@ bool SVN::prepareFileRevision(string const & revis, string & f) return false; } - doVCCommand("svn cat -r " + revname + ' ' + doVCCommand("svn cat -r " + revname + " -- " + quoteName(onlyFileName(owner_->absFileName())) + " > " + quoteName(tmpf.toFilesystemEncoding()), FileName(owner_->filePath())); @@ -1840,7 +1840,7 @@ bool GIT::findFile(FileName const & file) string const fname = onlyFileName(file.absFileName()); LYXERR(Debug::LYXVC, "LyXVC: Checking if file is under git control for `" << fname << '\''); - doVCCommandCall("git ls-files " + + doVCCommandCall("git ls-files -- " + quoteName(fname) + " > " + quoteName(tmpf.toFilesystemEncoding()), file.onlyPath()); @@ -1863,14 +1863,14 @@ bool GIT::retrieve(FileName const & file) { LYXERR(Debug::LYXVC, "LyXVC::GIT: retrieve.\n\t" << file); // The caller ensures that file does not exist, so no need to check that. - return doVCCommandCall("git checkout -q " + quoteName(file.onlyFileName()), + return doVCCommandCall("git checkout -q -- " + quoteName(file.onlyFileName()), file.onlyPath()) == 0; } void GIT::registrer(string const & /*msg*/) { - doVCCommand("git add " + quoteName(onlyFileName(owner_->absFileName())), + doVCCommand("git add -- " + quoteName(onlyFileName(owner_->absFileName())), FileName(owner_->filePath())); } @@ -1888,10 +1888,10 @@ string GIT::rename(support::FileName const & newFile, string const & msg) // confusing to the user to have two uncommitted files. FileName path(owner_->filePath()); string relFile(to_utf8(newFile.relPath(path.absFileName()))); - string cmd("git mv " + quoteName(onlyFileName(owner_->absFileName())) + + string cmd("git mv -- " + quoteName(onlyFileName(owner_->absFileName())) + ' ' + quoteName(relFile)); if (doVCCommand(cmd, path)) { - cmd = "git checkout -q " + + cmd = "git checkout -q -- " + quoteName(onlyFileName(owner_->absFileName())) + ' ' + quoteName(relFile); doVCCommand(cmd, path); @@ -1904,7 +1904,7 @@ string GIT::rename(support::FileName const & newFile, string const & msg) f.push_back(newFile); string log; if (checkIn(f, msg, log) != LyXVC::VCSuccess) { - cmd = "git checkout -q " + + cmd = "git checkout -q -- " + quoteName(onlyFileName(owner_->absFileName())) + ' ' + quoteName(relFile); doVCCommand(cmd, path); @@ -1990,7 +1990,7 @@ bool GIT::isCheckInWithConfirmation() return true; } - doVCCommandCall("git diff " + quoteName(owner_->absFileName()) + doVCCommandCall("git diff -- " + quoteName(owner_->absFileName()) + " > " + quoteName(tmpf.toFilesystemEncoding()), FileName(owner_->filePath())); @@ -2069,7 +2069,7 @@ bool GIT::revert() // gets the updated version from the repository. string const fil = quoteName(onlyFileName(owner_->absFileName())); - if (doVCCommand("git checkout -q " + fil, + if (doVCCommand("git checkout -q -- " + fil, FileName(owner_->filePath()))) return false; owner_->markClean(); @@ -2150,7 +2150,7 @@ bool GIT::getFileRevisionInfo() return false; } - doVCCommand("git log -n 1 --pretty=format:%H%n%h%n%an%n%ai " + quoteName(onlyFileName(owner_->absFileName())) + doVCCommand("git log -n 1 --pretty=format:%H%n%h%n%an%n%ai -- " + quoteName(onlyFileName(owner_->absFileName())) + " > " + quoteName(tmpf.toFilesystemEncoding()), FileName(owner_->filePath())); @@ -2203,7 +2203,7 @@ bool GIT::getTreeRevisionInfo() void GIT::getLog(FileName const & tmpf) { - doVCCommand("git log " + quoteName(onlyFileName(owner_->absFileName())) + doVCCommand("git log -- " + quoteName(onlyFileName(owner_->absFileName())) + " > " + quoteName(tmpf.toFilesystemEncoding()), FileName(owner_->filePath())); } -- 2.39.5
>From 67b05b35670cd6319bae454a37d62e78ceb3102b Mon Sep 17 00:00:00 2001 From: Pavel Sanda <[email protected]> Date: Sun, 11 Oct 2026 22:14:45 +0200 Subject: [PATCH] RCS: prefix filename arguments with ./ RCS has no --, so using prefixing with "./" because an argument starting with '.' can never be read as a flag while naming the same file. Tier 01 DiD / vcs-args: ./ prefix for RCS filename arguments Assisted-by: Claude Opus 5 --- src/VCBackend.cpp | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/VCBackend.cpp b/src/VCBackend.cpp index 751529c9be..ad3f0386f3 100644 --- a/src/VCBackend.cpp +++ b/src/VCBackend.cpp @@ -126,6 +126,8 @@ FileName VCS::checkParentDirs(FileName const & start, std::string const & file) // ///////////////////////////////////////////////////////////////////// +// Don't use absolute paths here, "./" is added to guard against "-files". + RCS::RCS(FileName const & m, Buffer * b) : VCS(b), master_(m) { // Here we know that the buffer file is either already in RCS or @@ -235,7 +237,7 @@ void RCS::registrer(string const & msg) string cmd = "ci -q -u -i -t-\""; cmd += msg; cmd += "\" "; - cmd += quoteName(onlyFileName(owner_->absFileName())); + cmd += quoteName("./" + onlyFileName(owner_->absFileName())); doVCCommand(cmd, FileName(owner_->filePath())); } @@ -278,7 +280,7 @@ string RCS::copy(support::FileName const & newFile, string const & msg) LyXVC::CommandResult RCS::checkIn(string const & msg, string & log) { int ret = doVCCommand("ci -q -u -m\"" + msg + "\" " - + quoteName(onlyFileName(owner_->absFileName())), + + quoteName("./" + onlyFileName(owner_->absFileName())), FileName(owner_->filePath())); if (ret) return LyXVC::ErrorCommand; @@ -323,7 +325,7 @@ bool RCS::isCheckInWithConfirmation() string RCS::checkOut() { owner_->markClean(); - int ret = doVCCommand("co -q -l " + quoteName(onlyFileName(owner_->absFileName())), + int ret = doVCCommand("co -q -l " + quoteName("./" + onlyFileName(owner_->absFileName())), FileName(owner_->filePath())); return ret ? string() : "RCS: Proceeded"; } @@ -367,7 +369,7 @@ bool RCS::lockingToggleEnabled() bool RCS::revert() { if (doVCCommand("co -f -u" + version_ + ' ' - + quoteName(onlyFileName(owner_->absFileName())), + + quoteName("./" + onlyFileName(owner_->absFileName())), FileName(owner_->filePath()))) return false; // We ignore changes and just reload! @@ -387,7 +389,7 @@ void RCS::undoLast() { LYXERR(Debug::LYXVC, "LyXVC: undoLast"); doVCCommand("rcs -o" + version_ + ' ' - + quoteName(onlyFileName(owner_->absFileName())), + + quoteName("./" + onlyFileName(owner_->absFileName())), FileName(owner_->filePath())); } @@ -400,7 +402,7 @@ bool RCS::undoLastEnabled() void RCS::getLog(FileName const & tmpf) { - doVCCommand("rlog " + quoteName(onlyFileName(owner_->absFileName())) + doVCCommand("rlog " + quoteName("./" + onlyFileName(owner_->absFileName())) + " > " + quoteName(tmpf.toFilesystemEncoding()), FileName(owner_->filePath())); } @@ -448,7 +450,7 @@ bool RCS::getRevisionInfo() LYXERR(Debug::LYXVC, "Could not generate logfile " << tmpf); return false; } - doVCCommand("rlog -r " + quoteName(onlyFileName(owner_->absFileName())) + doVCCommand("rlog -r " + quoteName("./" + onlyFileName(owner_->absFileName())) + " > " + quoteName(tmpf.toFilesystemEncoding()), FileName(owner_->filePath())); @@ -499,7 +501,7 @@ bool RCS::prepareFileRevision(string const &revis, string & f) } doVCCommand("co -p" + rev + ' ' - + quoteName(onlyFileName(owner_->absFileName())) + + quoteName("./" + onlyFileName(owner_->absFileName())) + " > " + quoteName(tmpf.toFilesystemEncoding()), FileName(owner_->filePath())); tmpf.refresh(); -- 2.39.5
-- lyx-devel mailing list [email protected] https://lists.lyx.org/mailman/listinfo/lyx-devel
