From d774f0e94e7c5a7864ca585c5b3493ee9e33fcf6 Mon Sep 17 00:00:00 2001
From: "Danilo M." <danix@danix.xyz>
Date: Sun, 2 Aug 2026 17:32:59 +0200
Subject: fix: make attachment path-containment guard separator-aware

Attachment::saveTo()'s escape guard compared paths with a bare
QString::startsWith(), which is not a path-boundary test: "/tmp/safe-evil"
textually starts with "/tmp/safe", so a sibling directory whose name merely
extends the target's name would incorrectly pass as contained within it.

Extract the check into Attachment::isPathInsideDirectory(), comparing
QDir::cleanPath()'d absolute paths and requiring an exact match or a prefix
ending at a '/' boundary. Not exploitable today since safeFilename() always
reduces the name to a bare basename before saveTo() builds the target, so
the guard is unreachable via saveTo()'s public interface; comments on both
now say so plainly instead of implying it is currently load-bearing.

Add pathInsideDirectoryRejectsSiblingPrefix, testing the guard directly
(independent of safeFilename(), which would mask a broken guard by never
producing an escaping path), and safeFilenameStripsPathComponents, testing
the sanitiser that actually stops traversal today.
---
 src/mimeparser.cpp | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

(limited to 'src/mimeparser.cpp')

diff --git a/src/mimeparser.cpp b/src/mimeparser.cpp
index 7393a96..0c457c2 100644
--- a/src/mimeparser.cpp
+++ b/src/mimeparser.cpp
@@ -146,15 +146,34 @@ QString Attachment::safeFilename() const
     return name;
 }
 
+bool Attachment::isPathInsideDirectory(const QString &directory, const QString &candidatePath)
+{
+    // Compare candidatePath itself, not QFileInfo(candidatePath).absolutePath()
+    // (which would be its *parent* directory) -- candidatePath may itself be
+    // the directory being tested, as in the "is directory itself" case this
+    // function documents.
+    const QString canonicalDir = QDir::cleanPath(QDir(directory).absolutePath());
+    const QString canonicalTarget =
+        QDir::cleanPath(QFileInfo(candidatePath).absoluteFilePath());
+    return canonicalTarget == canonicalDir
+        || canonicalTarget.startsWith(canonicalDir + QLatin1Char('/'));
+}
+
 QString Attachment::saveTo(const QString &directory, QString *error) const
 {
     const QDir dir(directory);
     const QString target = dir.absoluteFilePath(safeFilename());
 
-    // Belt and braces: confirm the resolved path really is inside directory,
-    // so a future change to safeFilename() cannot silently reintroduce escape.
-    const QString canonicalDir = QDir(directory).absolutePath();
-    if (!QFileInfo(target).absolutePath().startsWith(canonicalDir)) {
+    // Defence-in-depth, not currently load-bearing: safeFilename() always
+    // reduces the name to a plain basename before target is built above, so
+    // this check cannot actually be failed via saveTo()'s public interface
+    // today (dir.absoluteFilePath(basename) can't escape dir). It exists so
+    // that a future change which stops sanitising the name, or which starts
+    // accepting a caller-supplied subpath instead of a bare filename, still
+    // cannot write outside directory. See Attachment::isPathInsideDirectory
+    // for the containment logic and its own direct tests.
+    if (!isPathInsideDirectory(directory, target)) {
+        const QString canonicalDir = QDir::cleanPath(QDir(directory).absolutePath());
         if (error)
             *error = QStringLiteral("Refusing to write outside %1").arg(canonicalDir);
         return {};
-- 
cgit v1.2.3