diff options
| author | Danilo M. <danix@danix.xyz> | 2026-08-02 17:32:59 +0200 |
|---|---|---|
| committer | Danilo M. <danix@danix.xyz> | 2026-08-02 17:32:59 +0200 |
| commit | d774f0e94e7c5a7864ca585c5b3493ee9e33fcf6 (patch) | |
| tree | aa438d80f2421419f5595a93a423371753df3061 /tests/test_mimeparser.cpp | |
| parent | 35d3e4ff127b0213fa4d34f630c4a019e533a4df (diff) | |
| download | qtmaildir-d774f0e94e7c5a7864ca585c5b3493ee9e33fcf6.tar.gz qtmaildir-d774f0e94e7c5a7864ca585c5b3493ee9e33fcf6.zip | |
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.
Diffstat (limited to 'tests/test_mimeparser.cpp')
| -rw-r--r-- | tests/test_mimeparser.cpp | 58 |
1 files changed, 58 insertions, 0 deletions
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" |
