From 0802b6d506ed687249f77e39df5f57aab67fb215 Mon Sep 17 00:00:00 2001 From: Chocobo1 Date: Fri, 29 Jul 2022 11:16:40 +0800 Subject: [PATCH] Fix empty string parameter was omitted `QProcess::splitCommand()` will omit empty strings like `""` so provide our own replacement. Closes #13124. --- src/app/application.cpp | 128 ++++++++++++++++++++++---------------- src/base/utils/string.cpp | 37 +++++++++++ src/base/utils/string.h | 2 + test/CMakeLists.txt | 1 + test/testutilsstring.cpp | 59 ++++++++++++++++++ 5 files changed, 172 insertions(+), 55 deletions(-) create mode 100644 test/testutilsstring.cpp diff --git a/src/app/application.cpp b/src/app/application.cpp index 6962544de..f777f3f27 100644 --- a/src/app/application.cpp +++ b/src/app/application.cpp @@ -83,6 +83,7 @@ #include "base/utils/compare.h" #include "base/utils/fs.h" #include "base/utils/misc.h" +#include "base/utils/string.h" #include "base/version.h" #include "applicationinstancemanager.h" #include "filelogger.h" @@ -353,64 +354,74 @@ void Application::processMessage(const QString &message) void Application::runExternalProgram(const BitTorrent::Torrent *torrent) const { - QString program = Preferences::instance()->getAutoRunProgram().trimmed(); + // Cannot give users shell environment by default, as doing so could + // enable command injection via torrent name and other arguments + // (especially when some automated download mechanism has been setup). + // See: https://github.com/qbittorrent/qBittorrent/issues/10925 - for (int i = (program.length() - 2); i >= 0; --i) + const auto replaceVariables = [torrent](QString str) -> QString { - if (program[i] != u'%') - continue; - - const ushort specifier = program[i + 1].unicode(); - switch (specifier) + for (int i = (str.length() - 2); i >= 0; --i) { - case u'C': - program.replace(i, 2, QString::number(torrent->filesCount())); - break; - case u'D': - program.replace(i, 2, torrent->savePath().toString()); - break; - case u'F': - program.replace(i, 2, torrent->contentPath().toString()); - break; - case u'G': - program.replace(i, 2, torrent->tags().join(u","_qs)); - break; - case u'I': - program.replace(i, 2, (torrent->infoHash().v1().isValid() ? torrent->infoHash().v1().toString() : u"-"_qs)); - break; - case u'J': - program.replace(i, 2, (torrent->infoHash().v2().isValid() ? torrent->infoHash().v2().toString() : u"-"_qs)); - break; - case u'K': - program.replace(i, 2, torrent->id().toString()); - break; - case u'L': - program.replace(i, 2, torrent->category()); - break; - case u'N': - program.replace(i, 2, torrent->name()); - break; - case u'R': - program.replace(i, 2, torrent->rootPath().toString()); - break; - case u'T': - program.replace(i, 2, torrent->currentTracker()); - break; - case u'Z': - program.replace(i, 2, QString::number(torrent->totalSize())); - break; - default: - // do nothing - break; + if (str[i] != u'%') + continue; + + const ushort specifier = str[i + 1].unicode(); + switch (specifier) + { + case u'C': + str.replace(i, 2, QString::number(torrent->filesCount())); + break; + case u'D': + str.replace(i, 2, torrent->savePath().toString()); + break; + case u'F': + str.replace(i, 2, torrent->contentPath().toString()); + break; + case u'G': + str.replace(i, 2, torrent->tags().join(u","_qs)); + break; + case u'I': + str.replace(i, 2, (torrent->infoHash().v1().isValid() ? torrent->infoHash().v1().toString() : u"-"_qs)); + break; + case u'J': + str.replace(i, 2, (torrent->infoHash().v2().isValid() ? torrent->infoHash().v2().toString() : u"-"_qs)); + break; + case u'K': + str.replace(i, 2, torrent->id().toString()); + break; + case u'L': + str.replace(i, 2, torrent->category()); + break; + case u'N': + str.replace(i, 2, torrent->name()); + break; + case u'R': + str.replace(i, 2, torrent->rootPath().toString()); + break; + case u'T': + str.replace(i, 2, torrent->currentTracker()); + break; + case u'Z': + str.replace(i, 2, QString::number(torrent->totalSize())); + break; + default: + // do nothing + break; + } + + // decrement `i` to avoid unwanted replacement, example pattern: "%%N" + --i; } - // decrement `i` to avoid unwanted replacement, example pattern: "%%N" - --i; - } + return str; + }; - LogMsg(tr("Torrent: %1, running external program, command: %2").arg(torrent->name(), program)); + const QString logMsg = tr("Running external program. Torrent: \"%1\". Command: `%2`"); + // The processing sequenece is different for Windows and other OS, this is intentional #if defined(Q_OS_WIN) + const QString program = replaceVariables(Preferences::instance()->getAutoRunProgram().trimmed()); const std::wstring programWStr = program.toStdWString(); // Need to split arguments manually because QProcess::startDetached(QString) @@ -419,10 +430,15 @@ void Application::runExternalProgram(const BitTorrent::Torrent *torrent) const int argCount = 0; std::unique_ptr args {::CommandLineToArgvW(programWStr.c_str(), &argCount), ::LocalFree}; + if (argCount <= 0) + return; + QStringList argList; for (int i = 1; i < argCount; ++i) argList += QString::fromWCharArray(args[i]); + LogMsg(logMsg.arg(torrent->name(), program)); + QProcess proc; proc.setProgram(QString::fromWCharArray(args[0])); proc.setArguments(argList); @@ -449,14 +465,16 @@ void Application::runExternalProgram(const BitTorrent::Torrent *torrent) const }); proc.startDetached(); #else // Q_OS_WIN - // Cannot give users shell environment by default, as doing so could - // enable command injection via torrent name and other arguments - // (especially when some automated download mechanism has been setup). - // See: https://github.com/qbittorrent/qBittorrent/issues/10925 - QStringList args = QProcess::splitCommand(program); + QStringList args = Utils::String::splitCommand(Preferences::instance()->getAutoRunProgram().trimmed()); + if (args.isEmpty()) return; + for (QString &arg : args) + arg = replaceVariables(arg); + + LogMsg(logMsg.arg(torrent->name(), args.join(u' '))); + const QString command = args.takeFirst(); QProcess::startDetached(command, args); #endif diff --git a/src/base/utils/string.cpp b/src/base/utils/string.cpp index f7c0049ca..449cb62b8 100644 --- a/src/base/utils/string.cpp +++ b/src/base/utils/string.cpp @@ -33,6 +33,7 @@ #include #include +#include #include #if (QT_VERSION < QT_VERSION_CHECK(6, 0, 0)) @@ -76,6 +77,42 @@ QString Utils::String::wildcardToRegexPattern(const QString &pattern) } #endif +QStringList Utils::String::splitCommand(const QString &command) +{ + QStringList ret; + ret.reserve(32); + + bool inQuotes = false; + QString tmp; + for (const QChar c : command) + { + if (c == u' ') + { + if (!inQuotes) + { + if (!tmp.isEmpty()) + { + ret.append(tmp); + tmp.clear(); + } + + continue; + } + } + else if (c == u'"') + { + inQuotes = !inQuotes; + } + + tmp.append(c); + } + + if (!tmp.isEmpty()) + ret.append(tmp); + + return ret; +} + std::optional Utils::String::parseBool(const QString &string) { if (string.compare(u"true", Qt::CaseInsensitive) == 0) diff --git a/src/base/utils/string.h b/src/base/utils/string.h index f95d6d295..19f6ddee3 100644 --- a/src/base/utils/string.h +++ b/src/base/utils/string.h @@ -61,6 +61,8 @@ namespace Utils::String std::optional parseInt(const QString &string); std::optional parseDouble(const QString &string); + QStringList splitCommand(const QString &command); + QString join(const QList &strings, QStringView separator); QString fromDouble(double n, int precision); diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 5b1e87827..3b8babbc8 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -14,6 +14,7 @@ set(testFiles testorderedset.cpp testutilscompare.cpp testutilsgzip.cpp + testutilsstring.cpp ) foreach(testFile ${testFiles}) get_filename_component(testFilename "${testFile}" NAME_WLE) diff --git a/test/testutilsstring.cpp b/test/testutilsstring.cpp new file mode 100644 index 000000000..319acd046 --- /dev/null +++ b/test/testutilsstring.cpp @@ -0,0 +1,59 @@ +/* + * Bittorrent Client using Qt and libtorrent. + * Copyright (C) 2022 Mike Tzou (Chocobo1) + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + * + * In addition, as a special exception, the copyright holders give permission to + * link this program with the OpenSSL project's "OpenSSL" library (or with + * modified versions of it that use the same license as the "OpenSSL" library), + * and distribute the linked executables. You must obey the GNU General Public + * License in all respects for all of the code used other than "OpenSSL". If you + * modify file(s), you may extend this exception to your version of the file(s), + * but you are not obligated to do so. If you do not wish to do so, delete this + * exception statement from your version. + */ + +#include + +#include "base/global.h" +#include "base/utils/string.h" + +class TestUtilsString final : public QObject +{ + Q_OBJECT + Q_DISABLE_COPY_MOVE(TestUtilsString) + +public: + TestUtilsString() = default; + +private slots: + void testSplitCommand() const + { + QCOMPARE(Utils::String::splitCommand({}), {}); + QCOMPARE(Utils::String::splitCommand(u""_qs), {}); + QCOMPARE(Utils::String::splitCommand(u" "_qs), {}); + QCOMPARE(Utils::String::splitCommand(uR"("")"_qs), {uR"("")"_qs}); + QCOMPARE(Utils::String::splitCommand(uR"(" ")"_qs), {uR"(" ")"_qs}); + QCOMPARE(Utils::String::splitCommand(u"\"\"\""_qs), {u"\"\"\""_qs}); + QCOMPARE(Utils::String::splitCommand(uR"(" """)"_qs), {uR"(" """)"_qs}); + QCOMPARE(Utils::String::splitCommand(u" app a b c "_qs), QStringList({u"app"_qs, u"a"_qs, u"b"_qs, u"c"_qs})); + QCOMPARE(Utils::String::splitCommand(u" cmd.exe /d --arg2 \"arg3\" \"\" arg5 \"\"arg6 \"arg7 "_qs) + , QStringList({u"cmd.exe"_qs, u"/d"_qs, u"--arg2"_qs, u"\"arg3\""_qs, u"\"\""_qs, u"arg5"_qs, u"\"\"arg6"_qs, u"\"arg7 "_qs})); + } +}; + +QTEST_APPLESS_MAIN(TestUtilsString) +#include "testutilsstring.moc"