Skip to content
Permalink
Browse files Browse the repository at this point in the history
ZipFile: Add path checks to uncompressEntry()
  • Loading branch information
szarvas committed Jan 12, 2022
1 parent a2cc9a8 commit 2e874e8
Show file tree
Hide file tree
Showing 3 changed files with 105 additions and 6 deletions.
2 changes: 1 addition & 1 deletion modules/juce_core/files/juce_TemporaryFile.cpp
Expand Up @@ -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);
Expand Down
90 changes: 85 additions & 5 deletions modules/juce_core/zip/juce_ZipFile.cpp
Expand Up @@ -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
{
Expand Down Expand Up @@ -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);

Expand All @@ -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)

Expand All @@ -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());

Expand Down Expand Up @@ -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<String, MemoryBlock> blocks;

for (auto& entryName : entryNames)
Expand All @@ -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<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());
Expand All @@ -681,6 +758,9 @@ struct ZIPTests : public UnitTest
std::unique_ptr<InputStream> input (zip.createStreamForEntry (*entry));
expectEquals (input->readEntireStreamAsString(), entryName);
}

beginTest ("ZipSlip");
runZipSlipTest();
}
};

Expand Down
19 changes: 19 additions & 0 deletions modules/juce_core/zip/juce_ZipFile.h
Expand Up @@ -179,6 +179,25 @@ class JUCE_API ZipFile
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.
Expand Down

0 comments on commit 2e874e8

Please sign in to comment.