CVE-2021-23520: Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal') (CWE-22)
value: The package juce-framework/juce before 6.1.5 are vulnerable to Arbitrary File Write via Archive Extraction (Zip Slip) via the ZipFile::uncompressEntry function in juce_ZipFile.cpp. This vulnerability is triggered when the archive is extracted upon calling uncompressTo() on a ZipFile object.
CWE description: The product uses external input to construct a pathname that is intended to identify a file or directory that is located underneath a restricted parent directory, but the product does not properly neutralize special elements within the pathname that can cause the pathname to resolve to a location that is outside of the restricted directory.
CVSS v3 base score: 9.8
Published: 2022-01-31T11:15Z
Repository: https://github.com/juce-framework/JUCE
Language: C++
Files changed: modules/juce_core/files/juce_TemporaryFile.cpp, modules/juce_core/zip/juce_ZipFile.cpp, modules/juce_core/zip/juce_ZipFile.h
Commit date: 2022-01-12 13:16:57 +0100
Commit message: ZipFile:  Add path checks to uncompressEntry()

Diff: diff --git a/modules/juce_core/files/juce_TemporaryFile.cpp b/modules/juce_core/files/juce_TemporaryFile.cpp
index 645ba055c..4cf2f9f33 100644
--- a/modules/juce_core/files/juce_TemporaryFile.cpp
+++ b/modules/juce_core/files/juce_TemporaryFile.cpp
@@ -103,11 +103,11 @@ bool TemporaryFile::overwriteTargetFileWithTemporary() const
 bool TemporaryFile::deleteTemporaryFile() const
 {
     // Have a few attempts at deleting the file before giving up..
     for (int i = 5; --i >= 0;)
     {
-        if (temporaryFile.deleteFile())
+        if (temporaryFile.isDirectory() ? temporaryFile.deleteRecursively() : temporaryFile.deleteFile())
             return true;
 
         Thread::sleep (50);
     }
 
diff --git a/modules/juce_core/zip/juce_ZipFile.cpp b/modules/juce_core/zip/juce_ZipFile.cpp
index 1bc025f5b..084e6d378 100644
--- a/modules/juce_core/zip/juce_ZipFile.cpp
+++ b/modules/juce_core/zip/juce_ZipFile.cpp
@@ -120,10 +120,23 @@ static int64 findCentralDirectoryFileHeader (InputStream& input, int& numEntries
     }
 
     return 0;
 }
 
+static bool hasSymbolicPart (const File& root, const File& f)
+{
+    jassert (root == f || f.isAChildOf (root));
+
+    for (auto p = f; p != root; p = p.getParentDirectory())
+    {
+        if (p.isSymbolicLink())
+            return true;
+    }
+
+    return false;
+}
+
 //==============================================================================
 struct ZipFile::ZipInputStream  : public InputStream
 {
     ZipInputStream (ZipFile& zf, const ZipFile::ZipEntryHolder& zei)
         : file (zf),
@@ -398,10 +411,18 @@ Result ZipFile::uncompressTo (const File& targetDirectory,
 
     return Result::ok();
 }
 
 Result ZipFile::uncompressEntry (int index, const File& targetDirectory, bool shouldOverwriteFiles)
+{
+    return uncompressEntry (index,
+                            targetDirectory,
+                            shouldOverwriteFiles ? OverwriteFiles::yes : OverwriteFiles::no,
+                            FollowSymlinks::no);
+}
+
+Result ZipFile::uncompressEntry (int index, const File& targetDirectory, OverwriteFiles overwriteFiles, FollowSymlinks followSymlinks)
 {
     auto* zei = entries.getUnchecked (index);
 
    #if JUCE_WINDOWS
     auto entryPath = zei->entry.filename;
@@ -412,27 +433,33 @@ Result ZipFile::uncompressEntry (int index, const File& targetDirectory, bool sh
     if (entryPath.isEmpty())
         return Result::ok();
 
     auto targetFile = targetDirectory.getChildFile (entryPath);
 
+    if (! targetFile.isAChildOf (targetDirectory))
+        return Result::fail ("Entry " + entryPath + " is outside the target directory");
+
     if (entryPath.endsWithChar ('/') || entryPath.endsWithChar ('\\'))
         return targetFile.createDirectory(); // (entry is a directory, not a file)
 
     std::unique_ptr<InputStream> in (createStreamForEntry (index));
 
     if (in == nullptr)
         return Result::fail ("Failed to open the zip file for reading");
 
     if (targetFile.exists())
     {
-        if (! shouldOverwriteFiles)
+        if (overwriteFiles == OverwriteFiles::no)
             return Result::ok();
 
         if (! targetFile.deleteFile())
             return Result::fail ("Failed to write to target file: " + targetFile.getFullPathName());
     }
 
+    if (followSymlinks == FollowSymlinks::no && hasSymbolicPart (targetDirectory, targetFile.getParentDirectory()))
+        return Result::fail ("Parent directory leads through symlink for target file: " + targetFile.getFullPathName());
+
     if (! targetFile.getParentDirectory().createDirectory())
         return Result::fail ("Failed to create target folder: " + targetFile.getParentDirectory().getFullPathName());
 
     if (zei->entry.isSymbolicLink)
     {
@@ -647,16 +674,13 @@ struct ZIPTests   : public UnitTest
 {
     ZIPTests()
         : UnitTest ("ZIP", UnitTestCategories::compression)
     {}
 
-    void runTest() override
+    static MemoryBlock createZipMemoryBlock (const StringArray& entryNames)
     {
-        beginTest ("ZIP");
-
         ZipFile::Builder builder;
-        StringArray entryNames { "first", "second", "third" };
         HashMap<String, MemoryBlock> blocks;
 
         for (auto& entryName : entryNames)
         {
             auto& block = blocks.getReference (entryName);
@@ -667,22 +691,78 @@ struct ZIPTests   : public UnitTest
         }
 
         MemoryBlock data;
         MemoryOutputStream mo (data, false);
         builder.writeToStream (mo, nullptr);
+
+        return data;
+    }
+
+    void runZipSlipTest()
+    {
+        const std::map<String, bool> testCases = { { "a",                    true  },
+#if JUCE_WINDOWS
+                                                   { "C:/b",                 false },
+#else
+                                                   { "/b",                   false },
+#endif
+                                                   { "c/d",                  true  },
+                                                   { "../e/f",               false },
+                                                   { "../../g/h",            false },
+                                                   { "i/../j",               true  },
+                                                   { "k/l/../",              true  },
+                                                   { "m/n/../../",           false },
+                                                   { "o/p/../../../",        false } };
+
+        StringArray entryNames;
+
+        for (const auto& testCase : testCases)
+            entryNames.add (testCase.first);
+
+        TemporaryFile tmpDir;
+        tmpDir.getFile().createDirectory();
+        auto data = createZipMemoryBlock (entryNames);
         MemoryInputStream mi (data, false);
+        ZipFile zip (mi);
+
+        for (int i = 0; i < zip.getNumEntries(); ++i)
+        {
+            const auto result = zip.uncompressEntry (i, tmpDir.getFile());
+            const auto caseIt = testCases.find (zip.getEntry (i)->filename);
 
+            if (caseIt != testCases.end())
+            {
+                expect (result.wasOk() == caseIt->second,
+                        zip.getEntry (i)->filename + " was unexpectedly " + (result.wasOk() ? "OK" : "not OK"));
+            }
+            else
+            {
+                expect (false);
+            }
+        }
+    }
+
+    void runTest() override
+    {
+        beginTest ("ZIP");
+
+        StringArray entryNames { "first", "second", "third" };
+        auto data = createZipMemoryBlock (entryNames);
+        MemoryInputStream mi (data, false);
         ZipFile zip (mi);
 
         expectEquals (zip.getNumEntries(), entryNames.size());
 
         for (auto& entryName : entryNames)
         {
             auto* entry = zip.getEntry (entryName);
             std::unique_ptr<InputStream> input (zip.createStreamForEntry (*entry));
             expectEquals (input->readEntireStreamAsString(), entryName);
         }
+
+        beginTest ("ZipSlip");
+        runZipSlipTest();
     }
 };
 
 static ZIPTests zipTests;
 
diff --git a/modules/juce_core/zip/juce_ZipFile.h b/modules/juce_core/zip/juce_ZipFile.h
index d0a4baff8..212e54735 100644
--- a/modules/juce_core/zip/juce_ZipFile.h
+++ b/modules/juce_core/zip/juce_ZipFile.h
@@ -177,10 +177,29 @@ public:
     */
     Result uncompressEntry (int index,
                             const File& targetDirectory,
                             bool shouldOverwriteFiles = true);
 
+    enum class OverwriteFiles { no, yes };
+    enum class FollowSymlinks { no, yes };
+
+    /** Uncompresses one of the entries from the zip file.
+
+        This will expand the entry and write it in a target directory. The entry's path is used to
+        determine which subfolder of the target should contain the new file.
+
+        @param index                the index of the entry to uncompress - this must be a valid index
+                                    between 0 and (getNumEntries() - 1).
+        @param targetDirectory      the root folder to uncompress into
+        @param overwriteFiles       whether to overwrite existing files with similarly-named ones
+        @param followSymlinks       whether to follow symlinks inside the target directory
+        @returns success if all the files are successfully unzipped
+    */
+    Result uncompressEntry (int index,
+                            const File& targetDirectory,
+                            OverwriteFiles overwriteFiles,
+                            FollowSymlinks followSymlinks);
 
     //==============================================================================
     /** Used to create a new zip file.
 
         Create a ZipFile::Builder object, and call its addFile() method to add some files,

Vulnerable code:         if (temporaryFile.deleteFile())
        if (! shouldOverwriteFiles)
    void runTest() override
        beginTest ("ZIP");

        StringArray entryNames { "first", "second", "third" };
Fixed code:         if (temporaryFile.isDirectory() ? temporaryFile.deleteRecursively() : temporaryFile.deleteFile())
static bool hasSymbolicPart (const File& root, const File& f)
{
    jassert (root == f || f.isAChildOf (root));

    for (auto p = f; p != root; p = p.getParentDirectory())
    {
        if (p.isSymbolicLink())
            return true;
    }

    return false;
}

{
    return uncompressEntry (index,
                            targetDirectory,
                            shouldOverwriteFiles ? OverwriteFiles::yes : OverwriteFiles::no,
                            FollowSymlinks::no);
}

Result ZipFile::uncompressEntry (int index, const File& targetDirectory, OverwriteFiles overwriteFiles, FollowSymlinks followSymlinks)
    if (! targetFile.isAChildOf (targetDirectory))
        return Result::fail ("Entry " + entryPath + " is outside the target directory");

        if (overwriteFiles == OverwriteFiles::no)
    if (followSymlinks == FollowSymlinks::no && hasSymbolicPart (targetDirectory, targetFile.getParentDirectory()))
        return Result::fail ("Parent directory leads through symlink for target file: " + targetFile.getFullPathName());

    static MemoryBlock createZipMemoryBlock (const StringArray& entryNames)

        return data;
    }

    void runZipSlipTest()
    {
        const std::map<String, bool> testCases = { { "a",                    true  },
#if JUCE_WINDOWS
                                                   { "C:/b",                 false },
#else
                                                   { "/b",                   false },
#endif
                                                   { "c/d",                  true  },
                                                   { "../e/f",               false },
                                                   { "../../g/h",            false },
                                                   { "i/../j",               true  },
                                                   { "k/l/../",              true  },
                                                   { "m/n/../../",           false },
                                                   { "o/p/../../../",        false } };

        StringArray entryNames;

        for (const auto& testCase : testCases)
            entryNames.add (testCase.first);

        TemporaryFile tmpDir;
        tmpDir.getFile().createDirectory();
        auto data = createZipMemoryBlock (entryNames);
        ZipFile zip (mi);

        for (int i = 0; i < zip.getNumEntries(); ++i)
        {
            const auto result = zip.uncompressEntry (i, tmpDir.getFile());
            const auto caseIt = testCases.find (zip.getEntry (i)->filename);
            if (caseIt != testCases.end())
            {
                expect (result.wasOk() == caseIt->second,
                        zip.getEntry (i)->filename + " was unexpectedly " + (result.wasOk() ? "OK" : "not OK"));
            }
            else
            {
                expect (false);
            }
        }
    }

    void runTest() override
    {
        beginTest ("ZIP");

        StringArray entryNames { "first", "second", "third" };
        auto data = createZipMemoryBlock (entryNames);
        MemoryInputStream mi (data, false);

        beginTest ("ZipSlip");
        runZipSlipTest();
    enum class OverwriteFiles { no, yes };
    enum class FollowSymlinks { no, yes };

    /** Uncompresses one of the entries from the zip file.

        This will expand the entry and write it in a target directory. The entry's path is used to
        determine which subfolder of the target should contain the new file.

        @param index                the index of the entry to uncompress - this must be a valid index
                                    between 0 and (getNumEntries() - 1).
        @param targetDirectory      the root folder to uncompress into
        @param overwriteFiles       whether to overwrite existing files with similarly-named ones
        @param followSymlinks       whether to follow symlinks inside the target directory
        @returns success if all the files are successfully unzipped
    */
    Result uncompressEntry (int index,
                            const File& targetDirectory,
                            OverwriteFiles overwriteFiles,
                            FollowSymlinks followSymlinks);