CVE-2021-23521: Improper Link Resolution Before File Access ('Link Following') (CWE-59)
value: This affects the package juce-framework/JUCE before 6.1.5. This vulnerability is triggered when a malicious archive is crafted with an entry containing a symbolic link. When extracted, the symbolic link is followed outside of the target dir allowing writing arbitrary files on the target host. In some cases, this can allow an attacker to execute arbitrary code. The vulnerable code is in the ZipFile::uncompressEntry function in juce_ZipFile.cpp and is executed when the archive is extracted upon calling uncompressTo() on a ZipFile object.
CWE description: The product attempts to access a file based on the filename, but it does not properly prevent that filename from identifying a link or shortcut that resolves to an unintended resource.
CVSS v3 base score: 7.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);