Fixed the Zip Slip vulnerability in JlCompress

When extracting a file with a dangerous path like "../evil.exe" from a ZIP archive with JlCompress::extractDir(), the target file would be created outside of the target directory, potentially even overwriting an existing file there.
stachenov · Jun 12, 2018 · 5d2fc16 · 5d2fc16
1 parent ad509e8
commit 5d2fc16
Show file tree

Hide file tree

Showing 3 changed files with 30 additions and 14 deletions.
diff --git a/NEWS.txt b/NEWS.txt
@@ -1,5 +1,8 @@
 QuaZIP changes
 
+* Current
+        * Fixed the Zip Slip vulnerability in JlCompress
+
 * 0.7.5
         * Fixed target_link_libraries call in CMakeLists
         * Worked around a Qt 4.6 bug (QTBUG-15421) screwing up hidden

diff --git a/quazip/JlCompress.cpp b/quazip/JlCompress.cpp
@@ -364,15 +364,19 @@ QStringList JlCompress::extractDir(QuaZip &zip, const QString &dir)
     if(!zip.open(QuaZip::mdUnzip)) {
         return QStringList();
     }
-
-    QDir directory(dir);
+    QString cleanDir = QDir::cleanPath(dir);
+    QDir directory(cleanDir);
+    QString absCleanDir = directory.absolutePath();
     QStringList extracted;
     if (!zip.goToFirstFile()) {
         return QStringList();
     }
     do {
         QString name = zip.getCurrentFileName();
         QString absFilePath = directory.absoluteFilePath(name);
+        QString absCleanPath = QDir::cleanPath(absFilePath);
+        if (!absCleanPath.startsWith(absCleanDir + "/"))
+            continue;
         if (!extractFile(&zip, "", absFilePath)) {
             removeFile(extracted);
             return QStringList();

diff --git a/qztest/testjlcompress.cpp b/qztest/testjlcompress.cpp
@@ -302,17 +302,25 @@ void TestJlCompress::extractDir_data()
 {
     QTest::addColumn<QString>("zipName");
     QTest::addColumn<QStringList>("fileNames");
-    QTest::newRow("simple") << "jlextdir.zip" << (
-            QStringList() << "test0.txt" << "testdir1/test1.txt"
+    QTest::addColumn<QStringList>("expectedExtracted");
+    QTest::newRow("simple") << "jlextdir.zip"
+        << (QStringList() << "test0.txt" << "testdir1/test1.txt"
+            << "testdir2/test2.txt" << "testdir2/subdir/test2sub.txt")
+        << (QStringList() << "test0.txt" << "testdir1/test1.txt"
             << "testdir2/test2.txt" << "testdir2/subdir/test2sub.txt");
-    QTest::newRow("separate dir") << "sepdir.zip" << (
-            QStringList() << "laj/" << "laj/lajfile.txt");
+    QTest::newRow("separate dir") << "sepdir.zip"
+        << (QStringList() << "laj/" << "laj/lajfile.txt")
+        << (QStringList() << "laj/" << "laj/lajfile.txt");
+    QTest::newRow("Zip Slip") << "zipslip.zip"
+        << (QStringList() << "test0.txt" << "../zipslip.txt")
+        << (QStringList() << "test0.txt");
 }
 
 void TestJlCompress::extractDir()
 {
     QFETCH(QString, zipName);
     QFETCH(QStringList, fileNames);
+    QFETCH(QStringList, expectedExtracted);
     QDir curDir;
     if (!curDir.mkpath("jlext/jldir")) {
         QFAIL("Couldn't mkpath jlext/jldir");
@@ -325,17 +333,18 @@ void TestJlCompress::extractDir()
     }
     QStringList extracted;
     QCOMPARE((extracted = JlCompress::extractDir(zipName, "jlext/jldir"))
-        .count(), fileNames.count());
-    foreach (QString fileName, fileNames) {
-        QString fullName = "jlext/jldir/" + fileName;
+        .count(), expectedExtracted.count());
+    const QString dir = "jlext/jldir/";
+    foreach (QString fileName, expectedExtracted) {
+        QString fullName = dir + fileName;
         QFileInfo fileInfo(fullName);
         QFileInfo extInfo("tmp/" + fileName);
         if (!fileInfo.isDir())
             QCOMPARE(fileInfo.size(), extInfo.size());
         QCOMPARE(fileInfo.permissions(), extInfo.permissions());
         curDir.remove(fullName);
         curDir.rmpath(fileInfo.dir().path());
-        QString absolutePath = fileInfo.absoluteFilePath();
+        QString absolutePath = QDir(dir).absoluteFilePath(fileName);
         if (fileInfo.isDir() && !absolutePath.endsWith('/'))
 	    absolutePath += '/';
         QVERIFY(extracted.contains(absolutePath));
@@ -344,17 +353,17 @@ void TestJlCompress::extractDir()
     QFile zipFile(zipName);
     QVERIFY(zipFile.open(QIODevice::ReadOnly));
     QCOMPARE((extracted = JlCompress::extractDir(&zipFile, "jlext/jldir"))
-        .count(), fileNames.count());
-    foreach (QString fileName, fileNames) {
-        QString fullName = "jlext/jldir/" + fileName;
+        .count(), expectedExtracted.count());
+    foreach (QString fileName, expectedExtracted) {
+        QString fullName = dir + fileName;
         QFileInfo fileInfo(fullName);
         QFileInfo extInfo("tmp/" + fileName);
         if (!fileInfo.isDir())
             QCOMPARE(fileInfo.size(), extInfo.size());
         QCOMPARE(fileInfo.permissions(), extInfo.permissions());
         curDir.remove(fullName);
         curDir.rmpath(fileInfo.dir().path());
-        QString absolutePath = fileInfo.absoluteFilePath();
+        QString absolutePath = QDir(dir).absoluteFilePath(fileName);
         if (fileInfo.isDir() && !absolutePath.endsWith('/'))
         absolutePath += '/';
         QVERIFY(extracted.contains(absolutePath));