diff options
Diffstat (limited to 'src')
| -rw-r--r-- | src/mimeparser.cpp | 27 | ||||
| -rw-r--r-- | src/mimeparser.h | 18 |
2 files changed, 41 insertions, 4 deletions
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 {}; diff --git a/src/mimeparser.h b/src/mimeparser.h index 27a239f..891478a 100644 --- a/src/mimeparser.h +++ b/src/mimeparser.h @@ -27,6 +27,24 @@ struct Attachment /// Writes the attachment into directory. Returns the full path written, or /// an empty string on failure with *error set. QString saveTo(const QString &directory, QString *error) const; + + /// True if candidatePath (need not exist) is directory itself or strictly + /// beneath it, by path-boundary comparison after QDir::cleanPath on both + /// sides (so ".." segments are resolved rather than compared textually). + /// A bare QString::startsWith() is NOT sufficient here: it would let + /// "/tmp/safe-evil" pass against "/tmp/safe" since one string is a + /// textual prefix of the other despite being sibling directories. + /// + /// This is defence-in-depth, not currently load-bearing: saveTo() always + /// sanitises the name with safeFilename() first, which reduces it to a + /// plain basename, so no path reaching this check via saveTo()'s public + /// interface can actually fail it today. It exists for a future change + /// that stops sanitising, or that accepts a caller-supplied subpath. + /// Exposed as its own function so that guarantee can be tested directly, + /// independent of safeFilename() — a test driven purely through saveTo() + /// cannot exercise this comparison at all, since safeFilename() always + /// runs first and never produces a path that could fail it. + static bool isPathInsideDirectory(const QString &directory, const QString &candidatePath); }; struct ParsedMessage |