Skip to content

Commit

Permalink
Fix path traversal vulnerability
Browse files Browse the repository at this point in the history
AbstractUnArchiver#extractFile uses String#startsWith to verify
whether the target file is located inside the destination directory.
This check gives false negative for cases such as /opt/directory and
/opt/dir. /opt/directory starts with /opt/dir although it is not inside it.
This is a limited  path traversal vulnerability.

Fixes: #260
  • Loading branch information
plamentotev committed Mar 20, 2023
1 parent 68d6825 commit 8a37b7e
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.io.InputStream;
import java.io.OutputStream;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.Date;
import java.util.List;
Expand Down Expand Up @@ -341,8 +342,10 @@ protected void extractFile( final File srcF, final File dir, final InputStream c
final File targetFileName = 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 = targetFileName.getCanonicalPath();
// getCanonicalFile().toPath() is used instead of getCanonicalPath() (returns String),
// because "/opt/directory".startsWith("/opt/dir") would return false negative.
Path canonicalDirPath = dir.getCanonicalFile().toPath();
Path canonicalDestPath = targetFileName.getCanonicalFile().toPath();

if ( !canonicalDestPath.startsWith( canonicalDirPath ) )
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,14 +72,19 @@ public void shouldThrowExceptionBecauseRewrittenPathIsOutOfDirectory( @TempDir F
throws ArchiverException
{
// given
final FileMapper[] fileMappers = new FileMapper[] { pName -> "../PREFIX/" + pName, pName -> pName + ".SUFFIX"};

// The prefix includes the target directory name to make sure we catch cases when the paths
// are compared as strings. For example /opt/directory starts with /opt/dir but it is
// sibling and not inside /opt/dir.
String prefix = "../" + targetFolder.getName() + "PREFIX/";
final FileMapper[] fileMappers = new FileMapper[] { pName -> prefix + pName, pName -> pName + ".SUFFIX"};

// when
Exception exception = assertThrows( ArchiverException.class, () ->
abstractUnArchiver.extractFile( null, targetFolder, null, "ENTRYNAME", null, false, null, null, fileMappers ) );
// then
// ArchiverException is thrown providing the rewritten path
assertEquals( "Entry is outside of the target directory (../PREFIX/ENTRYNAME.SUFFIX)", exception.getMessage() );
assertEquals( "Entry is outside of the target directory (" + prefix + "ENTRYNAME.SUFFIX)", exception.getMessage() );
}

@Test
Expand Down

0 comments on commit 8a37b7e

Please sign in to comment.