From d4f6b5005101ca508ecbb01694b925a098eb06c6 Mon Sep 17 00:00:00 2001
From: Plamen Totev <plamen.iv.totev@gmail.com>
Date: Sun, 19 Mar 2023 18:39:11 +0000
Subject: [PATCH] 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
---
 .../org/codehaus/plexus/archiver/AbstractUnArchiver.java | 7 +++++--
 .../codehaus/plexus/archiver/AbstractUnArchiverTest.java | 9 +++++++--
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/src/main/java/org/codehaus/plexus/archiver/AbstractUnArchiver.java b/src/main/java/org/codehaus/plexus/archiver/AbstractUnArchiver.java
index af58daff2..52454f1d4 100644
--- a/src/main/java/org/codehaus/plexus/archiver/AbstractUnArchiver.java
+++ b/src/main/java/org/codehaus/plexus/archiver/AbstractUnArchiver.java
@@ -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;
@@ -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 ) )
         {
diff --git a/src/test/java/org/codehaus/plexus/archiver/AbstractUnArchiverTest.java b/src/test/java/org/codehaus/plexus/archiver/AbstractUnArchiverTest.java
index 3e440887e..b7698aa1f 100644
--- a/src/test/java/org/codehaus/plexus/archiver/AbstractUnArchiverTest.java
+++ b/src/test/java/org/codehaus/plexus/archiver/AbstractUnArchiverTest.java
@@ -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