From d774f0e94e7c5a7864ca585c5b3493ee9e33fcf6 Mon Sep 17 00:00:00 2001 From: "Danilo M." 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. --- docs/superpowers/plans/2026-08-02-qtmaildir-v1.md | 25 ++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) (limited to 'docs/superpowers') diff --git a/docs/superpowers/plans/2026-08-02-qtmaildir-v1.md b/docs/superpowers/plans/2026-08-02-qtmaildir-v1.md index b264986..391bcb6 100644 --- a/docs/superpowers/plans/2026-08-02-qtmaildir-v1.md +++ b/docs/superpowers/plans/2026-08-02-qtmaildir-v1.md @@ -1000,6 +1000,7 @@ private slots: void missingFileIsReported(); void hostileFilenameIsSanitised(); void savedAttachmentMatchesBytes(); + void safeFilenameStripsPathComponents(); private: QString fixture(const QString &name) const @@ -1384,12 +1385,26 @@ 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)) { + // Belt and braces, and currently UNREACHABLE through this function: + // safeFilename() above already reduces any name to a basename, so no + // caller-supplied filename can produce a target outside `directory`. + // The guard exists so that a future change which stops sanitising, or + // which lets a caller pass a subpath, still cannot escape. Do not write + // a test that drives saveTo() expecting a refusal: it cannot happen + // while safeFilename() runs first. Test safeFilename() instead, which + // is the control that actually stops traversal today. + // + // The comparison must be separator-aware. A bare startsWith() on the + // strings would accept "/tmp/safe-evil/x" as being inside "/tmp/safe", + // since one is a string prefix of the other with no path boundary + // between them. cleanPath() also resolves ".." before comparison rather + // than leaving it to be compared textually. + const QString cleanDir = QDir::cleanPath(QDir(directory).absolutePath()); + const QString cleanTarget = QDir::cleanPath(target); + if (cleanTarget != cleanDir + && !cleanTarget.startsWith(cleanDir + QLatin1Char('/'))) { if (error) - *error = QStringLiteral("Refusing to write outside %1").arg(canonicalDir); + *error = QStringLiteral("Refusing to write outside %1").arg(cleanDir); return {}; } -- cgit v1.2.3