fix(security): harden URL handling, fix memory safety and race conditions

audio/src/backend/openal.cpp

@@ -587,6 +587,7 @@ void OpenAL::cleanupInput()
     }
 
     delete[] inputBuffer;
+    inputBuffer = nullptr;
 }
 
 /**

src/chatlog/content/text.cpp

@@ -10,6 +10,9 @@
 #include <QDebug>
 #include <QDesktopServices>
 #include <QFontMetrics>
+#include <QMessageBox>
+#include <QSet>
+#include <QUrl>
 #include <QGraphicsSceneMouseEvent>
 #include <QPainter>
 #include <QPalette>
@@ -269,10 +272,47 @@ void Text::mouseReleaseEvent(QGraphicsSceneMouseEvent* event)
         return;
 
     const QString anchor = doc->documentLayout()->anchorAt(event->pos());
+    if (anchor.isEmpty())
+        return;
+
+    const QUrl url(anchor);
+    const QString scheme = url.scheme().toLower();
 
-    // open anchor in browser
-    if (!anchor.isEmpty())
-        QDesktopServices::openUrl(QUrl(anchor));
+    // Block dangerous schemes that trigger OS-level protocol handlers:
+    // - smb:// leaks NTLM hashes on Windows
+    // - file:// probes local filesystem
+    // - ed2k://, magnet: etc. invoke arbitrary external applications
+    static const QSet<QString> blockedSchemes = {
+        QStringLiteral("file"),   QStringLiteral("smb"),    QStringLiteral("ed2k"),
+        QStringLiteral("ms-msdt"), QStringLiteral("search-ms"),
+    };
+
+    if (blockedSchemes.contains(scheme)) {
+        qWarning() << "Blocked URL with dangerous scheme:" << scheme;
+        return;
+    }
+
+    // For tox: URIs, open directly (handled internally)
+    if (scheme == QStringLiteral("tox")) {
+        QDesktopServices::openUrl(url);
+        return;
+    }
+
+    // For all external URLs (http, https, ftp, mailto, gemini, etc.),
+    // show a confirmation dialog. This is critical for Tor/proxy users:
+    // opening a URL in the system browser bypasses the Tox proxy and
+    // reveals the user's real IP address to the destination server.
+    const auto answer = QMessageBox::question(
+        nullptr, tr("Open URL"),
+        tr("Opening this link in your browser may reveal your IP address.\n\n"
+           "URL: %1\n\n"
+           "Do you want to open it?")
+            .arg(url.toDisplayString()),
+        QMessageBox::Yes | QMessageBox::No, QMessageBox::No);
+
+    if (answer == QMessageBox::Yes) {
+        QDesktopServices::openUrl(url);
+    }
 }
 
 void Text::hoverMoveEvent(QGraphicsSceneHoverEvent* event)

src/chatlog/customtextdocument.cpp

@@ -33,5 +33,11 @@ QVariant CustomTextDocument::loadResource(int type, const QUrl& name)
         return icon->pixmap(size);
     }
 
-    return QTextDocument::loadResource(type, name);
+    // Block remote resource loading to prevent IP tracking via injected <img> tags.
+    // Only allow Qt resource files and local resources.
+    if (name.scheme() == QStringLiteral("qrc") || name.scheme().isEmpty()) {
+        return QTextDocument::loadResource(type, name);
+    }
+    qWarning() << "Blocked resource load for scheme:" << name.scheme();
+    return QVariant();
 }

src/chatlog/textformatter.cpp

@@ -74,13 +74,17 @@ const QVector<QRegularExpression> URI_WORD_PATTERNS = {
     // Note: This does not match only strictly valid URLs, but we broaden search to any string following scheme to
     // allow UTF-8 "IRI"s instead of ASCII-only URLs
     QRegularExpression(QStringLiteral(R"((?<=^|\s)\S*((((http[s]?)|ftp)://)\S+))")),
-    QRegularExpression(QStringLiteral(R"((?<=^|\s)\S*((file|smb)://([\S| ]*)))")),
+    // file:// and smb:// removed: dangerous schemes that enable NTLM hash theft
+    // and local file probing from remote messages. See also URL scheme allowlist
+    // in text.cpp mouseReleaseEvent().
     QRegularExpression(QStringLiteral(R"((?<=^|\s)\S*(tox:[a-zA-Z\d]{76}))")),
     QRegularExpression(QStringLiteral(R"((?<=^|\s)\S*(mailto:\S+@\S+\.\S+))")),
+    // Fixed: [\S| ] was matching any character (pipe literal inside char class).
+    // Changed to \S+ with a length cap via {1,2048} to prevent ReDoS.
     QRegularExpression(QStringLiteral(
-        R"((?<=^|\s)\S*(magnet:[?]((xt(.\d)?=urn:)|(mt=)|(kt=)|(tr=)|(dn=)|(xl=)|(xs=)|(as=)|(x.))[\S| ]+))")),
+        R"((?<=^|\s)\S*(magnet:[?]((xt(.\d)?=urn:)|(mt=)|(kt=)|(tr=)|(dn=)|(xl=)|(xs=)|(as=)|(x.))\S{1,2048}))")),
     QRegularExpression(QStringLiteral(R"((?<=^|\s)\S*(gemini://\S+))")),
-    QRegularExpression(QStringLiteral(R"((?<=^|\s)\S*(ed2k://\|file\|\S+))")),
+    // ed2k:// removed: dangerous external protocol handler invocation.
 };
 
 

src/core/core.cpp

@@ -29,6 +29,8 @@
 #include <cassert>
 #include <chrono>
 #include <memory>
+#include <QRandomGenerator>
+
 #include <random>
 #include <tox/tox.h>
 
@@ -40,7 +42,7 @@ namespace {
 
 QList<DhtServer> shuffleBootstrapNodes(QList<DhtServer> bootstrapNodes)
 {
-    std::mt19937 rng(std::chrono::high_resolution_clock::now().time_since_epoch().count());
+    std::mt19937 rng(std::random_device{}());
     std::shuffle(bootstrapNodes.begin(), bootstrapNodes.end(), rng);
     return bootstrapNodes;
 }
@@ -360,7 +362,7 @@ void Core::bootstrapDht()
         }
         if (dhtServer.statusTcp) {
             const auto ports = dhtServer.tcpPorts.size();
-            const auto tcpPort = dhtServer.tcpPorts[rand() % ports];
+            const auto tcpPort = dhtServer.tcpPorts[QRandomGenerator::global()->bounded(static_cast<quint32>(ports))];
             tox_add_tcp_relay(tox.get(), address.constData(), tcpPort, pkPtr, &error);
             PARSE_ERR(error);
         }
@@ -958,7 +960,7 @@ void Core::loadConferences()
         const size_t titleSize = tox_conference_get_title_size(tox.get(), conferenceNumber, &error);
         const ConferenceId persistentId = getConferencePersistentId(conferenceNumber);
         const QString defaultName = tr("Conference %1").arg(persistentId.toString().left(8));
-        if (PARSE_ERR(error) || (titleSize == 0u)) {
+        if (PARSE_ERR(error) && (titleSize != 0u)) {
             std::vector<uint8_t> nameBuf(titleSize);
             tox_conference_get_title(tox.get(), conferenceNumber, nameBuf.data(), &error);
             if (PARSE_ERR(error)) {

src/core/coreav.cpp

@@ -296,22 +296,23 @@ bool CoreAV::startCall(uint32_t friendNum, bool video)
 
 bool CoreAV::cancelCall(uint32_t friendNum)
 {
-    isCancelling = true;
     QWriteLocker locker{&callsLock};
+    isCancelling = true;
     const QMutexLocker<QRecursiveMutex> coreLocker{&coreLock};
 
     qDebug() << "Cancelling call with" << friendNum;
     Toxav_Err_Call_Control err;
     toxav_call_control(toxav.get(), friendNum, TOXAV_CALL_CONTROL_CANCEL, &err);
     if (!PARSE_ERR(err)) {
+        isCancelling = false;
         return false;
     }
 
     calls.erase(friendNum);
+    isCancelling = false;
     locker.unlock();
 
     emit avEnd(friendNum);
-    isCancelling = false;
     return true;
 }
 
@@ -867,13 +868,14 @@ void CoreAV::audioFrameCallback(ToxAV* toxAV, uint32_t friendNum, const int16_t*
 {
     std::ignore = toxAV;
     auto* self = static_cast<CoreAV*>(vSelf);
-    // If call is cancelling just return
-    if (self->isCancelling)
-        return;
     // This callback should come from the CoreAV thread
     assert(QThread::currentThread() == self->coreAvThread.get());
     const QReadLocker locker{&self->callsLock};
 
+    // Check cancellation under the lock to avoid TOCTOU race with cancelCall()
+    if (self->isCancelling)
+        return;
+
     auto it = self->calls.find(friendNum);
     if (it == self->calls.end()) {
         return;
@@ -894,13 +896,14 @@ void CoreAV::videoFrameCallback(ToxAV* toxAV, uint32_t friendNum, uint16_t w, ui
 {
     std::ignore = toxAV;
     auto* self = static_cast<CoreAV*>(vSelf);
-    // If call is cancelling just return
-    if (self->isCancelling)
-        return;
     // This callback should come from the CoreAV thread
     assert(QThread::currentThread() == self->coreAvThread.get());
     const QReadLocker locker{&self->callsLock};
 
+    // Check cancellation under the lock to avoid TOCTOU race with cancelCall()
+    if (self->isCancelling)
+        return;
+
     auto it = self->calls.find(friendNum);
     if (it == self->calls.end()) {
         return;

src/ipc.cpp

@@ -16,6 +16,9 @@
 #ifndef _MSC_VER
 #include <unistd.h>
 #endif
+#if !defined(Q_OS_WIN) && !defined(_MSC_VER)
+#include <pwd.h>
+#endif
 
 namespace {
 #if QT_CONFIG(sharedmemory)
@@ -27,6 +30,12 @@ const char* getCurUsername()
 #else
 const char* getCurUsername()
 {
+    // Prefer getpwuid over getenv("USER") since environment variables
+    // can be unset or spoofed.
+    const struct passwd* pw = getpwuid(getuid());
+    if (pw != nullptr) {
+        return pw->pw_name;
+    }
     return getenv("USER");
 }
 #endif
@@ -235,15 +244,17 @@ bool IPC::waitUntilAccepted(time_t postTime, int32_t timeout /*=-1*/)
 {
     bool result = false;
     const time_t start = time(nullptr);
+    // Cap infinite wait to 30 seconds to prevent unbounded blocking
+    const int32_t effectiveTimeout = (timeout <= 0) ? 30 : timeout;
     forever
     {
         result = isEventAccepted(postTime);
-        if (result || (timeout > 0 && difftime(time(nullptr), start) >= timeout)) {
+        if (result || (difftime(time(nullptr), start) >= effectiveTimeout)) {
             break;
         }
 
         qApp->processEvents();
-        QThread::msleep(0);
+        QThread::msleep(50); // Avoid busy-wait: sleep 50ms between polls
     }
     return result;
 }

src/persistence/db/rawdatabaseimpl.cpp

@@ -12,6 +12,8 @@
 #include <QMutexLocker>
 
 #include <cassert>
+#include <cerrno>
+#include <cstring>
 #include <tox/toxencryptsave.h>
 #include <utility>
 
@@ -535,12 +537,26 @@ bool RawDatabaseImpl::decryptDatabase()
 
 bool RawDatabaseImpl::commitDbSwap(const QString& hexKey)
 {
-    // This is racy as hell, but nobody will race with us since we hold the profile lock
-    // If we crash or die here, the rename should be atomic, so we can recover no matter
-    // what
+    // We hold the profile lock so no other qTox instance will race with us.
+    // On POSIX, rename() is atomic when src and dst are on the same filesystem,
+    // so we skip the remove() step -- rename replaces the target atomically.
     close();
-    QFile::remove(path);
-    QFile::rename(path + ".tmp", path);
+#if defined(Q_OS_UNIX)
+    const QByteArray srcPath = (path + ".tmp").toUtf8();
+    const QByteArray dstPath = path.toUtf8();
+    if (::rename(srcPath.constData(), dstPath.constData()) != 0) {
+        qCritical() << "Atomic rename failed:" << strerror(errno);
+        return false;
+    }
+#else
+    if (!QFile::remove(path)) {
+        qCritical() << "Failed to remove old database:" << path;
+    }
+    if (!QFile::rename(path + ".tmp", path)) {
+        qCritical() << "Failed to rename temp database:" << path;
+        return false;
+    }
+#endif
     currentHexKey = hexKey;
     if (!open(path, currentHexKey)) {
         qCritical() << "Failed to swap db";
@@ -619,6 +635,9 @@ struct PassKeyDeleter
 {
     void operator()(Tox_Pass_Key* pass_key)
     {
+        // Note: Tox_Pass_Key is an opaque type, so we cannot wipe its
+        // contents directly. Key material wiping for intermediate buffers
+        // is done in deriveKey() instead.
         tox_pass_key_free(pass_key);
     }
 };
@@ -644,7 +663,10 @@ QString RawDatabaseImpl::deriveKey(const QString& password)
         tox_pass_key_derive_with_salt(reinterpret_cast<const uint8_t*>(passData.data()),
                                       static_cast<std::size_t>(passData.size()), expandConstant,
                                       nullptr));
-    return QString::fromUtf8(QByteArray(reinterpret_cast<char*>(key.get()) + 32, 32).toHex());
+    QByteArray rawKey(reinterpret_cast<char*>(key.get()) + 32, 32);
+    QString hexKey = QString::fromUtf8(rawKey.toHex());
+    rawKey.fill('\0'); // Wipe raw key bytes
+    return hexKey;
 }
 
 /**
@@ -672,7 +694,10 @@ QString RawDatabaseImpl::deriveKey(const QString& password, const QByteArray& sa
         tox_pass_key_derive_with_salt(reinterpret_cast<const uint8_t*>(passData.data()),
                                       static_cast<std::size_t>(passData.size()),
                                       reinterpret_cast<const uint8_t*>(salt.constData()), nullptr));
-    return QString::fromUtf8(QByteArray(reinterpret_cast<char*>(key.get()) + 32, 32).toHex());
+    QByteArray rawKey(reinterpret_cast<char*>(key.get()) + 32, 32);
+    QString hexKey = QString::fromUtf8(rawKey.toHex());
+    rawKey.fill('\0'); // Wipe raw key bytes
+    return hexKey;
 }
 
 void RawDatabaseImpl::compileAndExecute(Transaction& trans)

src/persistence/paths.h

@@ -5,6 +5,7 @@
 
 #pragma once
 
+#include <QFile>
 #include <QString>
 #include <QStringList>
 
@@ -47,6 +48,19 @@ class Paths
     QString getBackupUserNodesFilePath() const;
 #endif
 
+    /**
+     * @brief Set restrictive permissions (owner-only read/write) on a sensitive file.
+     * No-op on Windows (relies on NTFS ACLs).
+     */
+    static void setSecureFilePermissions(const QString& filePath)
+    {
+#ifndef Q_OS_WIN
+        QFile::setPermissions(filePath, QFile::ReadOwner | QFile::WriteOwner);
+#else
+        Q_UNUSED(filePath);
+#endif
+    }
+
 private:
     QString basePath;
     QString overridePath;