3 files changed, 47 insertions, 13 deletions

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();