Skip to content

Commit

Permalink
fix: fail when trying to extract outside of dest dir
Browse files Browse the repository at this point in the history
A well crafted zip file may cause the code to extract outside of the destination dir.
This PR fails when that happens so that no unexpected behaviour happens.
  • Loading branch information
Odinn authored and Odinn committed May 5, 2018
1 parent 97c0d97 commit 58bc24e
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,15 @@ protected void extractFile( final File srcF, final File dir, final InputStream c
// Hmm. Symlinks re-evaluate back to the original file here. Unsure if this is a good thing...
final File f = FileUtils.resolveFile( dir, entryName );

// Make sure that the resolved path of the extracted file doesn't escape the destination directory
String canonicalDirPath = dir.getCanonicalPath();
String canonicalDestPath = f.getCanonicalPath();

if ( !canonicalDestPath.startsWith( canonicalDirPath ) )
{
throw new ArchiverException( "Entry is outside of the target directory (" + entryName + ")" );
}

try
{
if ( !isOverwrite() && f.exists() && ( f.lastModified() >= entryDate.getTime() ) )
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,30 @@ public void testSelectors()
} );
}

public void testExtractingZipWithEntryOutsideDestDirThrowsException()
throws Exception
{
Exception ex = null;
String s = "target/zip-unarchiver-slip-tests";
File testZip = new File( getBasedir(), "src/test/zips/zip-slip.zip" );
File outputDirectory = new File( getBasedir(), s );

FileUtils.deleteDirectory( outputDirectory );

try
{
ZipUnArchiver zu = getZipUnArchiver( testZip );
zu.extract( "", outputDirectory );
}
catch ( Exception e )
{
ex = e;
}

assertNotNull( ex );
assertTrue( ex.getMessage().startsWith( "Entry is outside of the target directory" ) );
}

private ZipArchiver getZipArchiver()
{
try
Expand Down
Binary file added src/test/zips/zip-slip.zip
Binary file not shown.

3 comments on commit 58bc24e

@jpederzolli
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change breaks cases where the maven-dependency-plugin is leveraged to intentionally unpack an archive to an arbitrary directory -- was this intended?

@michael-o
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I consider this not being our task to be checked. Plexus Archiver does sole unpacking. No semantics.

@plamentotev
Copy link
Member

@plamentotev plamentotev commented on 58bc24e Jun 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the purpose of the fix is to not allow files from the archive to be written outside the destination directory. Otherwise a maliciously crafted archive may cause files to be extracted in arbitrary (potentially dangerous) location. @jpederzolli I'm not sure I understand what you mean. Is there a use case where you want to unpack files outside the destination directory? Could you please give an example? Or even better - open an issue so we can better track it.

Please sign in to comment.