Skip to content

Commit 8a37b7e

Browse files
committed
Fix path traversal vulnerability
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
1 parent 68d6825 commit 8a37b7e

File tree

2 files changed

+12
-4
lines changed

2 files changed

+12
-4
lines changed

src/main/java/org/codehaus/plexus/archiver/AbstractUnArchiver.java

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import java.io.InputStream;
2323
import java.io.OutputStream;
2424
import java.nio.file.Files;
25+
import java.nio.file.Path;
2526
import java.util.ArrayList;
2627
import java.util.Date;
2728
import java.util.List;
@@ -341,8 +342,10 @@ protected void extractFile( final File srcF, final File dir, final InputStream c
341342
final File targetFileName = FileUtils.resolveFile( dir, entryName );
342343

343344
// Make sure that the resolved path of the extracted file doesn't escape the destination directory
344-
String canonicalDirPath = dir.getCanonicalPath();
345-
String canonicalDestPath = targetFileName.getCanonicalPath();
345+
// getCanonicalFile().toPath() is used instead of getCanonicalPath() (returns String),
346+
// because "/opt/directory".startsWith("/opt/dir") would return false negative.
347+
Path canonicalDirPath = dir.getCanonicalFile().toPath();
348+
Path canonicalDestPath = targetFileName.getCanonicalFile().toPath();
346349

347350
if ( !canonicalDestPath.startsWith( canonicalDirPath ) )
348351
{

src/test/java/org/codehaus/plexus/archiver/AbstractUnArchiverTest.java

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -72,14 +72,19 @@ public void shouldThrowExceptionBecauseRewrittenPathIsOutOfDirectory( @TempDir F
7272
throws ArchiverException
7373
{
7474
// given
75-
final FileMapper[] fileMappers = new FileMapper[] { pName -> "../PREFIX/" + pName, pName -> pName + ".SUFFIX"};
75+
76+
// The prefix includes the target directory name to make sure we catch cases when the paths
77+
// are compared as strings. For example /opt/directory starts with /opt/dir but it is
78+
// sibling and not inside /opt/dir.
79+
String prefix = "../" + targetFolder.getName() + "PREFIX/";
80+
final FileMapper[] fileMappers = new FileMapper[] { pName -> prefix + pName, pName -> pName + ".SUFFIX"};
7681

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

8590
@Test

0 commit comments

Comments
 (0)