Skip to content

Add ability to limit output size for zip unarchiver #115

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Add ability to limit output size for zip unarchiver #115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d145401
Add ability to limit output size for zip unarchiver as a way of prote…
satamas Jul 15, 2019
54704a6
Add zip output size limit test
satamas Jul 15, 2019
74b2b3a
Refactor imports after review
satamas Jul 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 18 additions & 16 deletions src/main/java/org/codehaus/plexus/archiver/AbstractUnArchiver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.util.ArrayList;
import java.util.Date;
import java.util.List;
import org.apache.commons.compress.utils.BoundedInputStream;
import org.codehaus.plexus.archiver.util.ArchiveEntryUtils;
import org.codehaus.plexus.components.io.attributes.SymlinkUtils;
import org.codehaus.plexus.components.io.filemappers.FileMapper;
Expand Down Expand Up @@ -316,9 +317,17 @@ public void setIgnorePermissions( final boolean ignorePermissions )
this.ignorePermissions = ignorePermissions;
}

protected void extractFile( final File srcF, final File dir, final InputStream compressedInputStream,
String entryName, final Date entryDate, final boolean isDirectory,
final Integer mode, String symlinkDestination, final FileMapper[] fileMappers )
protected File extractFile(final File srcF, final File dir, final InputStream compressedInputStream,
Copy link
Member

Choose a reason for hiding this comment

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

Please follow the coding style. There are spaces around the braces.

Please correct me if I'm wrong, but I think that would break existing code (if somebody have overridden the protected method). Would it make sense to keep this method void. If you don't have limit set then you don't care about the file anyway.

String entryName, final Date entryDate, final boolean isDirectory,
final Integer mode, String symlinkDestination, final FileMapper[] fileMappers)
throws IOException, ArchiverException {
return extractFile(srcF, dir, compressedInputStream, Long.MAX_VALUE, entryName, entryDate, isDirectory,
mode, symlinkDestination, fileMappers);
}

protected File extractFile(final File srcF, final File dir, final InputStream compressedInputStream,
Long remainingSpace, String entryName, final Date entryDate, final boolean isDirectory,
Copy link
Member

@plamentotev plamentotev Jul 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think sizeLimit or something among those lines makes more sense as this is what this variable is used for. It is the remaining size for the client of the method. For the method it is the size limit it should impose to the output stream.

Copy link
Member

Choose a reason for hiding this comment

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

Also would it make sense to have different name for this method (as the other extractFile does not return File) to avoid confusion. I'm not saying it is. I'm just wondering.

final Integer mode, String symlinkDestination, final FileMapper[] fileMappers )
throws IOException, ArchiverException
{
if ( fileMappers != null )
Expand All @@ -345,7 +354,7 @@ protected void extractFile( final File srcF, final File dir, final InputStream c
{
if ( !isOverwrite() && f.exists() && ( f.lastModified() >= entryDate.getTime() ) )
{
return;
return f;
Copy link
Member

Choose a reason for hiding this comment

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

The file is not really extracted. Then should the file be returned?

}

// create intermediary directories - sometimes zip don't add them
Expand All @@ -365,18 +374,8 @@ else if ( isDirectory )
}
else
{
OutputStream out = null;
try
{
out = new FileOutputStream( f );

IOUtil.copy( compressedInputStream, out );
out.close();
out = null;
}
finally
{
IOUtil.close( out );
try (OutputStream out = new FileOutputStream(f)) {
IOUtil.copy(new BoundedInputStream(compressedInputStream, remainingSpace), out);
}
}

Expand All @@ -386,10 +385,13 @@ else if ( isDirectory )
{
ArchiveEntryUtils.chmod( f, mode );
}

return f;
}
catch ( final FileNotFoundException ex )
{
getLogger().warn( "Unable to expand to file " + f.getPath() );
return null;
}
}

Expand Down
147 changes: 78 additions & 69 deletions src/main/java/org/codehaus/plexus/archiver/zip/AbstractZipUnArchiver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,12 @@
import java.util.Date;
import java.util.Enumeration;
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import org.apache.commons.compress.archivers.zip.UnicodePathExtraField;
import org.apache.commons.compress.archivers.zip.ZipArchiveEntry;
import org.apache.commons.compress.archivers.zip.ZipFile;
import org.apache.commons.compress.utils.IOUtils;
import org.codehaus.plexus.archiver.AbstractUnArchiver;
import org.codehaus.plexus.archiver.ArchiverException;
import org.codehaus.plexus.components.io.filemappers.FileMapper;
import org.codehaus.plexus.components.io.resources.PlexusIoResource;

/**
Expand All @@ -44,6 +43,9 @@ public abstract class AbstractZipUnArchiver

private String encoding = "UTF8";

@Nullable
private Long maxOutputSize;
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to use long with -1 meaning unlimited in order to avoid NPEs?

Copy link
Member

Choose a reason for hiding this comment

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

Would it makes sense to have it for other archives (such as tar)?


public AbstractZipUnArchiver()
{
}
Expand All @@ -68,6 +70,14 @@ public void setEncoding( String encoding )
this.encoding = encoding;
}

/**
* Set produced output size limit as a way of protection against zip bombs
* @param maxOutputSize - max size of produced output in bytes
*/
public void setMaxOutputSize(Long maxOutputSize) {
this.maxOutputSize = maxOutputSize;
}

private static class ZipEntryFileInfo
implements PlexusIoResource
{
Expand Down Expand Up @@ -160,78 +170,58 @@ public boolean isExisting()
protected void execute()
throws ArchiverException
{
getLogger().debug( "Expanding: " + getSourceFile() + " into " + getDestDirectory() );
org.apache.commons.compress.archivers.zip.ZipFile zf = null;
InputStream in = null;
try
try(ZipFile zipFile = new ZipFile( getSourceFile(), encoding, true ))
Copy link
Member

Choose a reason for hiding this comment

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

Would you please keep the diff minimal. I think this change makes the diff difficult to read. Using try-with-resource is better, but it would be better if this refactoring is made in separate merge request in order to make the commit easier to understand.

{
zf = new org.apache.commons.compress.archivers.zip.ZipFile( getSourceFile(), encoding, true );
final Enumeration e = zf.getEntriesInPhysicalOrder();
getLogger().debug( "Expanding: " + getSourceFile() + " into " + getDestDirectory() );
Long remainingSpace = maxOutputSize;
final Enumeration e = zipFile.getEntriesInPhysicalOrder();
Copy link
Member

Choose a reason for hiding this comment

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

The same here. Initially I thought the order in which the entries are iterated is changed :) Refactoring unrelated to the change(like renaming variables, changing the lines order, etc) is best left in separate merge request.

while ( e.hasMoreElements() )
{
final ZipArchiveEntry ze = (ZipArchiveEntry) e.nextElement();
final ZipEntryFileInfo fileInfo = new ZipEntryFileInfo( zf, ze );
final ZipEntryFileInfo fileInfo = new ZipEntryFileInfo( zipFile, ze );
if ( isSelected( fileInfo.getName(), fileInfo ) )
{
in = zf.getInputStream( ze );

extractFileIfIncluded( getSourceFile(), getDestDirectory(), in, fileInfo.getName(),
new Date( ze.getTime() ), ze.isDirectory(),
ze.getUnixMode() != 0 ? ze.getUnixMode() : null,
resolveSymlink( zf, ze ), getFileMappers() );

in.close();
in = null;
try (InputStream in = zipFile.getInputStream(ze)) {
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

if (remainingSpace != null)
{
File file = extractFile(getSourceFile(), getDestDirectory(), in, remainingSpace,
fileInfo.getName(), new Date(ze.getTime()), ze.isDirectory(),
ze.getUnixMode() != 0 ? ze.getUnixMode() : null,
resolveSymlink(zipFile, ze), getFileMappers());
if (file != null)
{
remainingSpace -= file.length();
if (remainingSpace <= 0)
{
throw new ArchiverException("Maximum output size limit reached");
}
}
}
else
{
extractFile(getSourceFile(), getDestDirectory(), in, fileInfo.getName(),
new Date(ze.getTime()), ze.isDirectory(),
ze.getUnixMode() != 0 ? ze.getUnixMode() : null,
resolveSymlink(zipFile, ze), getFileMappers());
}
}
}
}

zf.close();
zf = null;

getLogger().debug( "expand complete" );
}
catch ( final IOException ioe )
{
throw new ArchiverException( "Error while expanding " + getSourceFile().getAbsolutePath(), ioe );
}
finally
{
IOUtils.closeQuietly( in );
IOUtils.closeQuietly( zf );
}
}

private String resolveSymlink( ZipFile zf, ZipArchiveEntry ze )
throws IOException
{
if ( ze.isUnixSymlink() )
{
return zf.getUnixSymlink( ze );
}
else
{
return null;
}
}

private void extractFileIfIncluded( final File sourceFile, final File destDirectory, final InputStream inputStream,
final String name, final Date time, final boolean isDirectory,
final Integer mode, String symlinkDestination, final FileMapper[] fileMappers )
throws IOException, ArchiverException
{
extractFile( sourceFile, destDirectory, inputStream, name, time, isDirectory, mode, symlinkDestination, fileMappers );
}

@Override
protected void execute( final String path, final File outputDirectory )
throws ArchiverException
{
org.apache.commons.compress.archivers.zip.ZipFile zipFile = null;
InputStream in = null;
try
try(ZipFile zipFile = new ZipFile( getSourceFile(), encoding, true ))
{
zipFile = new org.apache.commons.compress.archivers.zip.ZipFile( getSourceFile(), encoding, true );

Long remainingSpace = maxOutputSize;
final Enumeration e = zipFile.getEntriesInPhysicalOrder();

while ( e.hasMoreElements() )
Expand All @@ -245,30 +235,49 @@ protected void execute( final String path, final File outputDirectory )

if ( ze.getName().startsWith( path ) )
{
in = zipFile.getInputStream( ze );

extractFileIfIncluded( getSourceFile(), outputDirectory, in,
ze.getName(), new Date( ze.getTime() ), ze.isDirectory(),
ze.getUnixMode() != 0 ? ze.getUnixMode() : null,
resolveSymlink( zipFile, ze ), getFileMappers() );

in.close();
in = null;
try(InputStream in = zipFile.getInputStream( ze )) {
if (remainingSpace != null)
{
File file = extractFile(getSourceFile(), outputDirectory, in, remainingSpace,
ze.getName(), new Date(ze.getTime()), ze.isDirectory(),
ze.getUnixMode() != 0 ? ze.getUnixMode() : null,
resolveSymlink(zipFile, ze), getFileMappers());
if (file != null)
{
remainingSpace -= file.length();
Copy link
Member

Choose a reason for hiding this comment

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

Is file.length() reliable way to get the file size. What I mean is (quoting the File class API docs):

Where it is required to distinguish an I/O exception from the case that 0L is returned, or where several attributes of the same file are required at the same time, then the java.nio.file.Files#readAttributes method may be used.

if (remainingSpace <= 0)
{
throw new ArchiverException("Maximum output size limit reached");
}
}
}
else
{
extractFile(getSourceFile(), outputDirectory, in,
ze.getName(), new Date(ze.getTime()), ze.isDirectory(),
ze.getUnixMode() != 0 ? ze.getUnixMode() : null,
resolveSymlink(zipFile, ze), getFileMappers());
}
}
}
}

zipFile.close();
zipFile = null;
}
catch ( final IOException ioe )
{
throw new ArchiverException( "Error while expanding " + getSourceFile().getAbsolutePath(), ioe );
}
finally
}

private String resolveSymlink( ZipFile zf, ZipArchiveEntry ze )
Copy link
Member

Choose a reason for hiding this comment

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

Is resolve link changed?

throws IOException
{
if ( ze.isUnixSymlink() )
{
return zf.getUnixSymlink( ze );
}
else
{
IOUtils.closeQuietly( in );
IOUtils.closeQuietly( zipFile );
return null;
}
}

}
25 changes: 25 additions & 0 deletions src/test/java/org/codehaus/plexus/archiver/zip/ZipUnArchiverTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,31 @@ public void testExtractingZipWithEntryOutsideDestDirThrowsException()
assertTrue( ex.getMessage().startsWith( "Entry is outside of the target directory" ) );
}

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

FileUtils.deleteDirectory( outputDirectory );

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

assertNotNull( ex );
assertTrue( ex.getMessage().startsWith( "Maximum output size limit reached" ) );
}

private ZipArchiver getZipArchiver()
{
try
Expand Down