diff --git a/audio/src/backend/openal.cpp b/audio/src/backend/openal.cpp index 788167ad10..dcffaccdd9 100644 --- a/audio/src/backend/openal.cpp +++ b/audio/src/backend/openal.cpp @@ -587,6 +587,7 @@ void OpenAL::cleanupInput() } delete[] inputBuffer; + inputBuffer = nullptr; } /** diff --git a/src/chatlog/content/text.cpp b/src/chatlog/content/text.cpp index 14e15dd7c7..a21e1d0186 100644 --- a/src/chatlog/content/text.cpp +++ b/src/chatlog/content/text.cpp @@ -10,6 +10,9 @@ #include #include #include +#include +#include +#include #include #include #include @@ -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 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) diff --git a/src/chatlog/customtextdocument.cpp b/src/chatlog/customtextdocument.cpp index bc3a31c0a5..eb7ff4d1eb 100644 --- a/src/chatlog/customtextdocument.cpp +++ b/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 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(); } diff --git a/src/chatlog/textformatter.cpp b/src/chatlog/textformatter.cpp index 0352b3145e..ffb42f253e 100644 --- a/src/chatlog/textformatter.cpp +++ b/src/chatlog/textformatter.cpp @@ -74,13 +74,17 @@ const QVector 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. }; diff --git a/src/core/core.cpp b/src/core/core.cpp index 7edff5ebd9..ad0ba72cdf 100644 --- a/src/core/core.cpp +++ b/src/core/core.cpp @@ -29,6 +29,8 @@ #include #include #include +#include + #include #include @@ -40,7 +42,7 @@ namespace { QList shuffleBootstrapNodes(QList 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(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 nameBuf(titleSize); tox_conference_get_title(tox.get(), conferenceNumber, nameBuf.data(), &error); if (PARSE_ERR(error)) { diff --git a/src/core/coreav.cpp b/src/core/coreav.cpp index a25fb7547b..c2f3d852ce 100644 --- a/src/core/coreav.cpp +++ b/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 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(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(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; diff --git a/src/ipc.cpp b/src/ipc.cpp index b4e7c2dbbb..0bd78bfb60 100644 --- a/src/ipc.cpp +++ b/src/ipc.cpp @@ -16,6 +16,9 @@ #ifndef _MSC_VER #include #endif +#if !defined(Q_OS_WIN) && !defined(_MSC_VER) +#include +#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; } diff --git a/src/persistence/db/rawdatabaseimpl.cpp b/src/persistence/db/rawdatabaseimpl.cpp index 67701428af..8a7a196d45 100644 --- a/src/persistence/db/rawdatabaseimpl.cpp +++ b/src/persistence/db/rawdatabaseimpl.cpp @@ -12,6 +12,8 @@ #include #include +#include +#include #include #include @@ -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(passData.data()), static_cast(passData.size()), expandConstant, nullptr)); - return QString::fromUtf8(QByteArray(reinterpret_cast(key.get()) + 32, 32).toHex()); + QByteArray rawKey(reinterpret_cast(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(passData.data()), static_cast(passData.size()), reinterpret_cast(salt.constData()), nullptr)); - return QString::fromUtf8(QByteArray(reinterpret_cast(key.get()) + 32, 32).toHex()); + QByteArray rawKey(reinterpret_cast(key.get()) + 32, 32); + QString hexKey = QString::fromUtf8(rawKey.toHex()); + rawKey.fill('\0'); // Wipe raw key bytes + return hexKey; } void RawDatabaseImpl::compileAndExecute(Transaction& trans) diff --git a/src/persistence/paths.h b/src/persistence/paths.h index 9e1f122ee1..417b50645a 100644 --- a/src/persistence/paths.h +++ b/src/persistence/paths.h @@ -5,6 +5,7 @@ #pragma once +#include #include #include @@ -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; diff --git a/src/persistence/profile.cpp b/src/persistence/profile.cpp index 357ebcb19c..ef68d570d7 100644 --- a/src/persistence/profile.cpp +++ b/src/persistence/profile.cpp @@ -24,6 +24,7 @@ #include #include #include +#include #include #include @@ -347,6 +348,11 @@ std::unique_ptr Profile::createProfile(const QString& name, const QStri CameraSource& cameraSource, IMessageBoxManager& messageBoxManager) { + if (!isValidProfileName(name)) { + qWarning() << "Invalid profile name rejected:" << name; + return nullptr; + } + CreateToxDataError error; Paths& paths = settings.getPaths(); const QString path = paths.getSettingsDirPath() + name + ".tox"; @@ -510,6 +516,7 @@ bool Profile::saveToxSave(QByteArray data) // check if everything got written if (saveFile.flush()) { saveFile.commit(); + Paths::setSecureFilePermissions(path); } else { saveFile.cancelWriting(); qCritical() << "Failed to write, can't save"; @@ -802,6 +809,34 @@ void Profile::removeAvatar(const ToxPk& owner) } } +/** + * @brief Validates that a profile name is safe for use in file paths. + * Rejects names containing path separators, traversal sequences, control + * characters, and Windows reserved device names. + */ +bool Profile::isValidProfileName(const QString& name) +{ + if (name.isEmpty() || name.trimmed().isEmpty()) { + return false; + } + // Reject path separators, traversal, and dangerous characters + if (name.contains('/') || name.contains('\\') || name.contains("..") || name.contains('\0')) { + return false; + } + // Reject Windows reserved device names + static const QSet reserved = { + QStringLiteral("CON"), QStringLiteral("PRN"), QStringLiteral("AUX"), + QStringLiteral("NUL"), QStringLiteral("COM1"), QStringLiteral("COM2"), + QStringLiteral("COM3"), QStringLiteral("COM4"), QStringLiteral("COM5"), + QStringLiteral("COM6"), QStringLiteral("COM7"), QStringLiteral("COM8"), + QStringLiteral("COM9"), QStringLiteral("LPT1"), QStringLiteral("LPT2"), + QStringLiteral("LPT3"), QStringLiteral("LPT4"), QStringLiteral("LPT5"), + QStringLiteral("LPT6"), QStringLiteral("LPT7"), QStringLiteral("LPT8"), + QStringLiteral("LPT9"), + }; + return !reserved.contains(name.toUpper()); +} + bool Profile::exists(QString name, Paths& paths) { const QString path = paths.getSettingsDirPath() + name; diff --git a/src/persistence/profile.h b/src/persistence/profile.h index b565978ca8..686de72880 100644 --- a/src/persistence/profile.h +++ b/src/persistence/profile.h @@ -66,6 +66,7 @@ class Profile : public QObject bool rename(QString newName); + static bool isValidProfileName(const QString& name); static QStringList getAllProfileNames(Paths& paths); static QString getProfilePath(const QString& name, const Paths& paths); diff --git a/src/platform/posixsignalnotifier.cpp b/src/platform/posixsignalnotifier.cpp index 1850bb269a..91818348ff 100644 --- a/src/platform/posixsignalnotifier.cpp +++ b/src/platform/posixsignalnotifier.cpp @@ -87,12 +87,12 @@ void installCrashHandler() constexpr struct sigaction default_action = {}; ::sigaction(sig, &default_action, nullptr); - // Print to qCritical, which will write to the log file, if possible. - // This might crash more or fail allocations. It's best effort. - qCritical("Crash signal %d received", sig); - Stacktrace::process([](const Stacktrace::Frame& frame) { qCritical() << frame; }); + // Only use async-signal-safe functions here. + // write() and _exit() are safe; qCritical/Stacktrace are NOT. + const char msg[] = "Fatal: crash signal received\n"; + (void)::write(STDERR_FILENO, msg, sizeof(msg) - 1); - // We let the handler return to trigger the default action. + // Re-raise to trigger default handler (core dump). }; for (auto s : crashSignals) { @@ -152,9 +152,11 @@ void PosixSignalNotifier::watchUsrSignals() void PosixSignalNotifier::unwatchSignal(int signum) { struct sigaction action = {}; // all zeroes by default - action.sa_handler = [](int sig) { - qWarning("Signal %d received twice; terminating ungracefully", sig); - ::exit(EXIT_FAILURE); + action.sa_handler = [](int) { + // Only async-signal-safe functions: write() and _exit(). + const char msg[] = "Signal received twice; terminating ungracefully\n"; + (void)::write(STDERR_FILENO, msg, sizeof(msg) - 1); + _exit(EXIT_FAILURE); }; if (::sigaction(signum, &action, nullptr) != 0) { diff --git a/src/video/cameradevice.cpp b/src/video/cameradevice.cpp index 0ea40bb589..f715c2fdc8 100644 --- a/src/video/cameradevice.cpp +++ b/src/video/cameradevice.cpp @@ -288,10 +288,13 @@ void CameraDevice::open() */ bool CameraDevice::close() { - if (--refcount > 0) - return false; - + // Acquire lock before decrementing to prevent another thread from finding + // this device in openDevices while we're about to delete it. openDeviceLock.lock(); + if (--refcount > 0) { + openDeviceLock.unlock(); + return false; + } openDevices.remove(devName); openDeviceLock.unlock(); avformat_close_input(&context); diff --git a/src/video/corevideosource.cpp b/src/video/corevideosource.cpp index a581f49a5a..bcb0214847 100644 --- a/src/video/corevideosource.cpp +++ b/src/video/corevideosource.cpp @@ -55,6 +55,12 @@ void CoreVideoSource::pushFrame(const vpx_image_t* vpxFrame) const int width = vpxFrame->d_w; const int height = vpxFrame->d_h; + // Validate dimensions: must be positive, even (YUV420), and within reasonable bounds + if (width <= 0 || height <= 0 || (width % 2) != 0 || (height % 2) != 0 + || width > 4096 || height > 4096) { + return; + } + if (subscribers <= 0) return; @@ -99,16 +105,15 @@ void CoreVideoSource::subscribe() void CoreVideoSource::unsubscribe() { - biglock.lock(); + const QMutexLocker locker(&biglock); if (--subscribers == 0) { if (deleteOnClose) { - biglock.unlock(); - // DANGEROUS: No member access after this point, that's why we manually unlock - delete this; + // Defer deletion to event loop to avoid destroying the mutex + // while other threads may be waiting on it. + deleteLater(); return; } } - biglock.unlock(); } /** diff --git a/src/video/videoframe.cpp b/src/video/videoframe.cpp index feb77f9d68..9888cabf36 100644 --- a/src/video/videoframe.cpp +++ b/src/video/videoframe.cpp @@ -7,6 +7,8 @@ #include +#include + extern "C" { #pragma GCC diagnostic push @@ -198,6 +200,15 @@ std::shared_ptr VideoFrame::fromAVFrame(IDType sourceID, AVFrame* so // We need to add a new source to our reference map, obtain write lock refsLock.unlock(); refsLock.lockForWrite(); + + // Re-check after lock upgrade to handle race where another thread + // inserted between our unlock and lock-for-write. + if (!refsMap.contains(sourceID)) { + // operator[] default-constructs the entries in-place. + // QMutex is non-copyable so we cannot use assignment. + refsMap[sourceID]; + mutexMap[sourceID]; + } } const auto frame = [sourceID, sourceFrame] { @@ -320,6 +331,12 @@ std::pair VideoFrame::toToxYUVFrame() return {ToxYUVFrame{}, ReadWriteLocker()}; } + // Validate dimensions fit in uint16_t to prevent silent wraparound + if (frameSize.width() > std::numeric_limits::max() + || frameSize.height() > std::numeric_limits::max()) { + return {ToxYUVFrame{}, ReadWriteLocker()}; + } + return toGenericObject(frameSize, AV_PIX_FMT_YUV420P, true, [frameSize](AVFrame* const frame) { // Converter function (constructs ToxAVFrame out of AVFrame*) return ToxYUVFrame{ diff --git a/src/video/videoframe.h b/src/video/videoframe.h index 09340f2369..1119586735 100644 --- a/src/video/videoframe.h +++ b/src/video/videoframe.h @@ -78,7 +78,7 @@ class ReadWriteLocker } private: - QReadWriteLock* lock_; + QReadWriteLock* lock_ = nullptr; }; struct ToxYUVFrame