diff options
-rw-r--r-- | .github/workflows/build.yml | 25 | ||||
-rw-r--r-- | launcher/minecraft/OneSixVersionFormat.cpp | 11 | ||||
-rw-r--r-- | launcher/modplatform/modrinth/ModrinthInstanceCreationTask.cpp | 19 | ||||
-rw-r--r-- | tests/Task_test.cpp | 30 |
4 files changed, 63 insertions, 22 deletions
diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 6ec4f6a4..86e88fa1 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -149,6 +149,15 @@ jobs: with: key: ${{ matrix.os }}-qt${{ matrix.qt_ver }}-${{ matrix.architecture }} + - name: Retrieve ccache cache (Windows MinGW-w64) + if: runner.os == 'Windows' && matrix.msystem != '' && inputs.build_type == 'Debug' + uses: actions/cache@v3.2.4 + with: + path: '${{ github.workspace }}\.ccache' + key: ${{ matrix.os }}-mingw-w64-ccache-${{ github.run_id }} + restore-keys: | + ${{ matrix.os }}-mingw-w64-ccache + - name: Setup ccache (Windows MinGW-w64) if: runner.os == 'Windows' && matrix.msystem != '' && inputs.build_type == 'Debug' shell: msys2 {0} @@ -165,15 +174,6 @@ jobs: run: | echo "CCACHE_VAR=ccache" >> $GITHUB_ENV - - name: Retrieve ccache cache (Windows MinGW-w64) - if: runner.os == 'Windows' && matrix.msystem != '' && inputs.build_type == 'Debug' - uses: actions/cache@v3.2.4 - with: - path: '${{ github.workspace }}\.ccache' - key: ${{ matrix.os }}-mingw-w64 - restore-keys: | - ${{ matrix.os }}-mingw-w64 - - name: Set short version shell: bash run: | @@ -510,6 +510,13 @@ jobs: with: name: PrismLauncher-${{ runner.os }}-${{ env.VERSION }}-${{ inputs.build_type }}-x86_64.AppImage path: PrismLauncher-${{ runner.os }}-${{ env.VERSION }}-${{ inputs.build_type }}-x86_64.AppImage + + - name: ccache stats (Windows MinGW-w64) + if: runner.os == 'Windows' && matrix.msystem != '' + shell: msys2 {0} + run: | + ccache -s + snap: runs-on: ubuntu-20.04 steps: diff --git a/launcher/minecraft/OneSixVersionFormat.cpp b/launcher/minecraft/OneSixVersionFormat.cpp index 280f6b26..888b6860 100644 --- a/launcher/minecraft/OneSixVersionFormat.cpp +++ b/launcher/minecraft/OneSixVersionFormat.cpp @@ -39,6 +39,8 @@ #include "minecraft/ParseUtils.h" #include <minecraft/MojangVersionFormat.h> +#include <QRegularExpression> + using namespace Json; static void readString(const QJsonObject &root, const QString &key, QString &variable) @@ -121,6 +123,15 @@ VersionFilePtr OneSixVersionFormat::versionFileFromJson(const QJsonDocument &doc out->uid = root.value("fileId").toString(); } + const QRegularExpression valid_uid_regex{ QRegularExpression::anchoredPattern(QStringLiteral(R"([a-zA-Z0-9-_]+(?:\.[a-zA-Z0-9-_]+)*)")) }; + if (!valid_uid_regex.match(out->uid).hasMatch()) { + qCritical() << "The component's 'uid' contains illegal characters! UID:" << out->uid; + out->addProblem( + ProblemSeverity::Error, + QObject::tr("The component's 'uid' contains illegal characters! This can cause security issues.") + ); + } + out->version = root.value("version").toString(); MojangVersionFormat::readVersionProperties(root, out.get()); diff --git a/launcher/modplatform/modrinth/ModrinthInstanceCreationTask.cpp b/launcher/modplatform/modrinth/ModrinthInstanceCreationTask.cpp index 94c0bf77..6814e645 100644 --- a/launcher/modplatform/modrinth/ModrinthInstanceCreationTask.cpp +++ b/launcher/modplatform/modrinth/ModrinthInstanceCreationTask.cpp @@ -225,10 +225,19 @@ bool ModrinthCreationTask::createInstance() m_files_job.reset(new NetJob(tr("Mod download"), APPLICATION->network())); + auto root_modpack_path = FS::PathCombine(m_stagingPath, ".minecraft"); + auto root_modpack_url = QUrl::fromLocalFile(root_modpack_path); + for (auto file : m_files) { - auto path = FS::PathCombine(m_stagingPath, ".minecraft", file.path); - qDebug() << "Will try to download" << file.downloads.front() << "to" << path; - auto dl = Net::Download::makeFile(file.downloads.dequeue(), path); + auto file_path = FS::PathCombine(root_modpack_path, file.path); + if (!root_modpack_url.isParentOf(QUrl::fromLocalFile(file_path))) { + // This means we somehow got out of the root folder, so abort here to prevent exploits + setError(tr("One of the files has a path that leads to an arbitrary location (%1). This is a security risk and isn't allowed.").arg(file.path)); + return false; + } + + qDebug() << "Will try to download" << file.downloads.front() << "to" << file_path; + auto dl = Net::Download::makeFile(file.downloads.dequeue(), file_path); dl->addValidator(new Net::ChecksumValidator(file.hashAlgorithm, file.hash)); m_files_job->addNetAction(dl); @@ -236,8 +245,8 @@ bool ModrinthCreationTask::createInstance() // FIXME: This really needs to be put into a ConcurrentTask of // MultipleOptionsTask's , once those exist :) auto param = dl.toWeakRef(); - connect(dl.get(), &NetAction::failed, [this, &file, path, param] { - auto ndl = Net::Download::makeFile(file.downloads.dequeue(), path); + connect(dl.get(), &NetAction::failed, [this, &file, file_path, param] { + auto ndl = Net::Download::makeFile(file.downloads.dequeue(), file_path); ndl->addValidator(new Net::ChecksumValidator(file.hashAlgorithm, file.hash)); m_files_job->addNetAction(ndl); if (auto shared = param.lock()) shared->succeeded(); diff --git a/tests/Task_test.cpp b/tests/Task_test.cpp index 558cd2c0..95eb4a30 100644 --- a/tests/Task_test.cpp +++ b/tests/Task_test.cpp @@ -7,6 +7,8 @@ #include <tasks/SequentialTask.h> #include <tasks/Task.h> +#include <array> + /* Does nothing. Only used for testing. */ class BasicTask : public Task { Q_OBJECT @@ -35,10 +37,23 @@ class BasicTask_MultiStep : public Task { void executeTask() override {}; }; -class BigConcurrentTask : public QThread { +class BigConcurrentTask : public ConcurrentTask { + Q_OBJECT + + void startNext() override + { + // This is here only to help fill the stack a bit more quickly (if there's an issue, of course :^)) + // Each tasks thus adds 1024 * 4 bytes to the stack, at the very least. + [[maybe_unused]] volatile std::array<uint32_t, 1024> some_data_on_the_stack {}; + + ConcurrentTask::startNext(); + } +}; + +class BigConcurrentTaskThread : public QThread { Q_OBJECT - ConcurrentTask big_task; + BigConcurrentTask big_task; void run() override { @@ -48,7 +63,9 @@ class BigConcurrentTask : public QThread { deadline.start(); // NOTE: Arbitrary value that manages to trigger a problem when there is one. - static const unsigned s_num_tasks = 1 << 14; + // Considering each tasks, in a problematic state, adds 1024 * 4 bytes to the stack, + // this number is enough to fill up 16 MiB of stack, more than enough to cause a problem. + static const unsigned s_num_tasks = 1 << 12; auto sub_tasks = new BasicTask::Ptr[s_num_tasks]; for (unsigned i = 0; i < s_num_tasks; i++) { @@ -237,12 +254,9 @@ class TaskTest : public QObject { { QEventLoop loop; - auto thread = new BigConcurrentTask; - // NOTE: This is an arbitrary value, big enough to not cause problems on normal execution, but low enough - // so that the number of tasks that needs to get ran to potentially cause a problem isn't too big. - thread->setStackSize(32 * 1024); + auto thread = new BigConcurrentTaskThread; - connect(thread, &BigConcurrentTask::finished, &loop, &QEventLoop::quit); + connect(thread, &BigConcurrentTaskThread::finished, &loop, &QEventLoop::quit); thread->start(); |