diff --git a/modules/juce_core/files/juce_TemporaryFile.cpp b/modules/juce_core/files/juce_TemporaryFile.cpp index 645ba055ca..4cf2f9f332 100644 --- a/modules/juce_core/files/juce_TemporaryFile.cpp +++ b/modules/juce_core/files/juce_TemporaryFile.cpp @@ -105,7 +105,7 @@ 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 1bc025f5b3..084e6d3789 100644 --- a/modules/juce_core/zip/juce_ZipFile.cpp +++ b/modules/juce_core/zip/juce_ZipFile.cpp @@ -122,6 +122,19 @@ 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 { @@ -400,6 +413,14 @@ Result ZipFile::uncompressTo (const File& targetDirectory, } 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); @@ -414,6 +435,9 @@ Result ZipFile::uncompressEntry (int index, const File& targetDirectory, bool sh 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) @@ -424,13 +448,16 @@ Result ZipFile::uncompressEntry (int index, const File& targetDirectory, bool sh 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()); @@ -649,12 +676,9 @@ struct ZIPTests : public UnitTest : UnitTest ("ZIP", UnitTestCategories::compression) {} - void runTest() override + static MemoryBlock createZipMemoryBlock (const StringArray& entryNames) { - beginTest ("ZIP"); - ZipFile::Builder builder; - StringArray entryNames { "first", "second", "third" }; HashMap blocks; for (auto& entryName : entryNames) @@ -669,8 +693,61 @@ struct ZIPTests : public UnitTest MemoryBlock data; MemoryOutputStream mo (data, false); builder.writeToStream (mo, nullptr); + + return data; + } + + void runZipSlipTest() + { + const std::map 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()); @@ -681,6 +758,9 @@ struct ZIPTests : public UnitTest std::unique_ptr input (zip.createStreamForEntry (*entry)); expectEquals (input->readEntireStreamAsString(), entryName); } + + beginTest ("ZipSlip"); + runZipSlipTest(); } }; diff --git a/modules/juce_core/zip/juce_ZipFile.h b/modules/juce_core/zip/juce_ZipFile.h index d0a4baff82..212e547354 100644 --- a/modules/juce_core/zip/juce_ZipFile.h +++ b/modules/juce_core/zip/juce_ZipFile.h @@ -179,6 +179,25 @@ public: 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.