diff --git a/core_lib/src/util/util.cpp b/core_lib/src/util/util.cpp index 3388a9b247..27f1352eef 100644 --- a/core_lib/src/util/util.cpp +++ b/core_lib/src/util/util.cpp @@ -129,58 +129,50 @@ QString uniqueString(int len) return QString::fromUtf8(s); } +QString closestCanonicalPath(QString path) +{ + QString origPath = QDir(path).absolutePath(); + + // Iterate up the path until an existing file/directory is found + QFileInfo existingSubpath(origPath); + // Symlinks must be checked for separately because exists checks if the target of the symlink exists, not the symlink itself + while (!existingSubpath.isRoot() && !existingSubpath.exists() && !existingSubpath.isSymbolicLink()) + { + // Move up one directory logically + existingSubpath.setFile(existingSubpath.dir().absolutePath()); + } + + // Resolve symlinks for all existing parts of the path + QString canonicalPath = existingSubpath.canonicalFilePath(); + if (canonicalPath.isEmpty()) + { + // This can happen if there is a dangling symlink in the path + return QString(); + } + + // Combine existing canonical path with non-existing path segment + QString finalPath = QDir(canonicalPath).filePath(QDir(existingSubpath.absoluteFilePath()).relativeFilePath(origPath)); + + return QDir(finalPath).absolutePath(); +} + QString validateDataPath(QString filePath, QString dataDirPath) { // Make sure src path is relative if (!QFileInfo(filePath).isRelative()) return QString(); - QFileInfo fi(dataDirPath, filePath); - // Recursively resolve symlinks - QString canonicalPath = fi.canonicalFilePath(); + // Get canonical path of data dir and file for comparison + QString canonicalDataDirPath = closestCanonicalPath(dataDirPath); + QString canonicalFilePath = closestCanonicalPath(QDir(dataDirPath).filePath(filePath)); - QDir dataDir(dataDirPath); - // Resolve symlinks in data dir path so it can be compared against file paths with resolved symlinks - if (dataDir.exists()) - { - dataDir.setPath(dataDir.canonicalPath()); - } - // Iterate over parent directories of the file path to see if one of them equals the data directory - if (canonicalPath.isEmpty()) + if (canonicalFilePath.startsWith(canonicalDataDirPath)) { - // File does not exist, use absolute path and attempt to resolve symlinks again for each parent directory - fi.setFile(fi.absoluteFilePath()); - QDir ancestor(fi.absoluteFilePath()); - while (ancestor != dataDir) { - if (ancestor.isRoot()) - { - // Reached root directory without finding data dir - return QString(); - } - QDir newAncestor = QFileInfo(ancestor.absolutePath()).dir(); - if (newAncestor.exists()) - { - // Resolve directory symlinks - newAncestor.setPath(newAncestor.canonicalPath()); - } - ancestor = newAncestor; - } - // One of the parent directories of filePath matches dataDir - return fi.absoluteFilePath(); + return canonicalFilePath; } else { - // File exists and all symlinks have been resolved in canonicalPath so no further attempts to resolve symlinks are necessary - fi.setFile(canonicalPath); - QDir ancestor = fi.dir(); - while (ancestor != dataDir) - { - if (ancestor.isRoot()) { - // Data dir was not found in ancestors of the src path - return QString(); - } - ancestor = QFileInfo(ancestor.absolutePath()).dir(); - } - // One of the parent directories of filePath matches dataDir - return fi.absoluteFilePath(); + // If canonicalFilePath does not start with the canonicalDataDirPath, then symlinks or '..' have made + // the file resolve outside of the data directory and the file should not be loaded. + return QString(); } } diff --git a/core_lib/src/util/util.h b/core_lib/src/util/util.h index eccc44e0d4..15bdb90cb0 100644 --- a/core_lib/src/util/util.h +++ b/core_lib/src/util/util.h @@ -68,6 +68,18 @@ QString ffmpegLocation(); quint64 imageSize(const QImage&); QString uniqueString(int len); +/** + * Converts a filesystem path to its canonical form, ie. an absolute path with any existing symlinks resolved. + * + * This function does the same thing as QDir::canonicalPath if a file or directory exists at the path. + * If the path does not point to an existing file or directory, then this function will resolve the symlinks only + * for the existing parent directories, rather than returning an empty string as QDir::canonicalPath does. + * This function may still return a blank string if the path contains dangling symbolic links. + * + * @param path The path to convert to canonical form. + * @return A canonical path, or as close to one as possible. + */ +QString closestCanonicalPath(QString path); /** * Performs safety checks for paths to data directory assets. * @@ -83,10 +95,11 @@ QString uniqueString(int len); * function if: * - An existing file is being modified/appended in-place (not overwritten) in the data directory. * - The data directory is not guaranteed to be the immediate parent directory of the file being written. + * - The path comes from an untrusted source or could contain sections to navigate up the directory heirarchy (ie. '../') * * @param filePath A path to a data file. * @param dataDir The path to the data directory. - * @return The valid resolved path, or empty if the path is not valid. + * @return The closest canonical resolved path, or empty if the path did not pass validation or contains dangling symbolic links. */ QString validateDataPath(QString filePath, QString dataDirPath); diff --git a/tests/src/test_layerbitmap.cpp b/tests/src/test_layerbitmap.cpp index c216c17a33..cac8f3fbf5 100644 --- a/tests/src/test_layerbitmap.cpp +++ b/tests/src/test_layerbitmap.cpp @@ -17,6 +17,7 @@ GNU General Public License for more details. #include "layerbitmap.h" #include "bitmapimage.h" +#include "util.h" #include #include @@ -61,7 +62,7 @@ TEST_CASE("Load bitmap layer from XML") REQUIRE(frame != nullptr); REQUIRE(frame->top() == 0); REQUIRE(frame->left() == 0); - REQUIRE(QDir(frame->fileName()) == QDir(dataDir.filePath("001.001.png"))); + REQUIRE(closestCanonicalPath(frame->fileName()) == closestCanonicalPath(dataDir.filePath("001.001.png"))); } SECTION("Multiple frames") @@ -78,7 +79,7 @@ TEST_CASE("Load bitmap layer from XML") REQUIRE(frame != nullptr); REQUIRE(frame->top() == 0); REQUIRE(frame->left() == 0); - REQUIRE(QDir(frame->fileName()) == QDir(dataDir.filePath(QString("001.%1.png").arg(QString::number(i), 3, QChar('0'))))); + REQUIRE(closestCanonicalPath(frame->fileName()) == closestCanonicalPath(dataDir.filePath(QString("001.%1.png").arg(QString::number(i), 3, QChar('0'))))); } } @@ -112,6 +113,6 @@ TEST_CASE("Load bitmap layer from XML") REQUIRE(frame != nullptr); REQUIRE(frame->top() == 0); REQUIRE(frame->left() == 0); - REQUIRE(QDir(frame->fileName()) == QDir(dataDir.filePath("subdir/001.001.png"))); + REQUIRE(closestCanonicalPath(frame->fileName()) == closestCanonicalPath(dataDir.filePath("subdir/001.001.png"))); } } diff --git a/tests/src/test_layersound.cpp b/tests/src/test_layersound.cpp index de5fbc5209..1806032322 100644 --- a/tests/src/test_layersound.cpp +++ b/tests/src/test_layersound.cpp @@ -17,6 +17,7 @@ GNU General Public License for more details. #include "layersound.h" #include "soundclip.h" +#include "util.h" #include #include @@ -63,7 +64,7 @@ TEST_CASE("Load sound layer from XML") REQUIRE(soundLayer->keyFrameCount() == 1); SoundClip* frame = static_cast(soundLayer->getKeyFrameAt(1)); REQUIRE(frame != nullptr); - REQUIRE(QDir(frame->fileName()) == QDir(dataDir.filePath("sound_001.wav"))); + REQUIRE(closestCanonicalPath(frame->fileName()) == closestCanonicalPath(dataDir.filePath("sound_001.wav"))); } SECTION("Multiple clips") @@ -78,7 +79,7 @@ TEST_CASE("Load sound layer from XML") { SoundClip* frame = static_cast(soundLayer->getKeyFrameAt(i)); REQUIRE(frame != nullptr); - REQUIRE(QDir(frame->fileName()) == QDir(dataDir.filePath(QString("sound_%1.wav").arg(QString::number(i), 3, QChar('0'))))); + REQUIRE(closestCanonicalPath(frame->fileName()) == closestCanonicalPath(dataDir.filePath(QString("sound_%1.wav").arg(QString::number(i), 3, QChar('0'))))); } } @@ -111,6 +112,6 @@ TEST_CASE("Load sound layer from XML") REQUIRE(soundLayer->keyFrameCount() == 1); SoundClip* frame = static_cast(soundLayer->getKeyFrameAt(1)); REQUIRE(frame != nullptr); - REQUIRE(QDir(frame->fileName()) == QDir(dataDir.filePath("subdir/sound_001.wav"))); + REQUIRE(closestCanonicalPath(frame->fileName()) == closestCanonicalPath(dataDir.filePath("subdir/sound_001.wav"))); } } diff --git a/tests/src/test_layervector.cpp b/tests/src/test_layervector.cpp index ba950db5ce..1d54115d45 100644 --- a/tests/src/test_layervector.cpp +++ b/tests/src/test_layervector.cpp @@ -16,6 +16,7 @@ GNU General Public License for more details. #include "catch.hpp" #include "layervector.h" +#include "util.h" #include "vectorimage.h" @@ -71,7 +72,7 @@ TEST_CASE("Load vector layer from XML") REQUIRE(vectorLayer->keyFrameCount() == 1); VectorImage* frame = static_cast(vectorLayer->getKeyFrameAt(1)); REQUIRE(frame != nullptr); - REQUIRE(QDir(frame->fileName()) == QDir(dataDir.filePath("001.001.vec"))); + REQUIRE(closestCanonicalPath(frame->fileName()) == closestCanonicalPath(dataDir.filePath("001.001.vec"))); } SECTION("Multiple frames") @@ -86,7 +87,7 @@ TEST_CASE("Load vector layer from XML") { VectorImage* frame = static_cast(vectorLayer->getKeyFrameAt(i)); REQUIRE(frame != nullptr); - REQUIRE(QDir(frame->fileName()) == QDir(dataDir.filePath(QString("001.%1.vec").arg(QString::number(i), 3, QChar('0'))))); + REQUIRE(closestCanonicalPath(frame->fileName()) == closestCanonicalPath(dataDir.filePath(QString("001.%1.vec").arg(QString::number(i), 3, QChar('0'))))); } } @@ -119,6 +120,6 @@ TEST_CASE("Load vector layer from XML") REQUIRE(vectorLayer->keyFrameCount() == 1); VectorImage* frame = static_cast(vectorLayer->getKeyFrameAt(1)); REQUIRE(frame != nullptr); - REQUIRE(QDir(frame->fileName()) == QDir(dataDir.filePath("subdir/001.001.vec"))); + REQUIRE(closestCanonicalPath(frame->fileName()) == closestCanonicalPath(dataDir.filePath("subdir/001.001.vec"))); } }