From 18dffc348f722fc79ed336523982541283bc84e1 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.
---
 tests/test_mimeparser.cpp | 58 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

(limited to 'tests/test_mimeparser.cpp')

diff --git a/tests/test_mimeparser.cpp b/tests/test_mimeparser.cpp
index 72bdff7..030f23e 100644
--- a/tests/test_mimeparser.cpp
+++ b/tests/test_mimeparser.cpp
@@ -19,6 +19,8 @@ private slots:
     void missingFileIsReported();
     void hostileFilenameIsSanitised();
     void savedAttachmentMatchesBytes();
+    void safeFilenameStripsPathComponents();
+    void pathInsideDirectoryRejectsSiblingPrefix();
 
 private:
     QString fixture(const QString &name) const
@@ -167,5 +169,61 @@ void TestMimeParser::savedAttachmentMatchesBytes()
     QCOMPARE(f.readAll(), msg.attachments.first().data);
 }
 
+void TestMimeParser::safeFilenameStripsPathComponents()
+{
+    // This is the control that genuinely stops traversal: saveTo() always
+    // routes through safeFilename() first, so whatever this function
+    // guarantees is what actually protects a write to disk. Constructed by
+    // hand since these are adversarial names not tied to any fixture.
+    Attachment a;
+    a.mimeType = QStringLiteral("text/plain");
+    a.data = QByteArrayLiteral("x");
+
+    a.filename = QStringLiteral("../../../../tmp/pwned.txt");
+    QCOMPARE(a.safeFilename(), QStringLiteral("pwned.txt"));
+
+    a.filename = QStringLiteral("../xyz-evil/x.txt");
+    QCOMPARE(a.safeFilename(), QStringLiteral("x.txt"));
+
+    a.filename = QStringLiteral("..\\..\\windows\\evil.txt");
+    QCOMPARE(a.safeFilename(), QStringLiteral("evil.txt"));
+
+    a.filename = QStringLiteral("plain.txt");
+    QCOMPARE(a.safeFilename(), QStringLiteral("plain.txt"));
+
+    // Nothing usable remains: a generated name is produced instead. Assert
+    // its shape rather than an exact value, since it embeds a fresh UUID.
+    a.filename = QStringLiteral("..");
+    QString generated = a.safeFilename();
+    QVERIFY(!generated.isEmpty());
+    QVERIFY(generated != QStringLiteral(".."));
+    QVERIFY(!generated.contains(QLatin1Char('/')));
+
+    a.filename = QString();
+    generated = a.safeFilename();
+    QVERIFY(!generated.isEmpty());
+    QVERIFY(!generated.contains(QLatin1Char('/')));
+}
+
+void TestMimeParser::pathInsideDirectoryRejectsSiblingPrefix()
+{
+    // Direct test of the containment guard's own comparison, independent of
+    // safeFilename() (which always runs first inside saveTo() and would
+    // mask a broken guard, since it never produces an escaping path). This
+    // targets exactly the defect that was found: a plain string
+    // startsWith() incorrectly treats a sibling directory whose name merely
+    // extends the target's name (e.g. "/tmp/safe-evil") as contained within
+    // it (e.g. "/tmp/safe").
+    const QString base = QStringLiteral("/tmp/safe");
+
+    QVERIFY(Attachment::isPathInsideDirectory(base, base + QStringLiteral("/notes.txt")));
+    QVERIFY(Attachment::isPathInsideDirectory(base, base + QStringLiteral("/sub/notes.txt")));
+    QVERIFY(Attachment::isPathInsideDirectory(base, base));
+
+    QVERIFY(!Attachment::isPathInsideDirectory(base, QStringLiteral("/tmp/safe-evil/x")));
+    QVERIFY(!Attachment::isPathInsideDirectory(base, QStringLiteral("/tmp/safe/../etc/passwd")));
+    QVERIFY(!Attachment::isPathInsideDirectory(base, QStringLiteral("/etc/passwd")));
+}
+
 QTEST_MAIN(TestMimeParser)
 #include "test_mimeparser.moc"
-- 
cgit v1.2.3