Skip to content

Commit

Permalink
[DS-4131] Fix zip import handling to avoid path traversal exploit
Browse files Browse the repository at this point in the history
  • Loading branch information
@kshepherd
kshepherd committed Jan 14, 2022
1 parent f775845 commit 7af52a0
Showing 1 changed file with 36 additions and 7 deletions.
43 changes: 36 additions & 7 deletions dspace-api/src/main/java/org/dspace/app/itemimport/ItemImportServiceImpl.java
View file
Expand Up @@ -55,6 +55,8 @@
import javax.xml.transform.TransformerException;
import java.io.*;
import java.net.URL;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.sql.SQLException;
import java.text.SimpleDateFormat;
import java.util.*;
Expand Down Expand Up @@ -1630,26 +1632,36 @@ public String unzip(File zipfile, String destDir) throws IOException {
{
log.error("Zip file '" + zipfile.getAbsolutePath() + "' does not exist, or is not readable.");
}
log.debug("Extracting zip at " + zipfile.getAbsolutePath());

String destinationDir = destDir;
if (destinationDir == null){
destinationDir = tempWorkDir;
}
log.debug("Using directory " + destinationDir + " for zip extraction. (destDir arg is " + destDir +
", tempWorkDir is " + tempWorkDir + ")");

File tempdir = new File(destinationDir);
if (!tempdir.isDirectory())
{
log.error("'" + ConfigurationManager.getProperty("org.dspace.app.itemexport.work.dir") +
"' as defined by the key 'org.dspace.app.itemexport.work.dir' in dspace.cfg " +
log.error("'" + ConfigurationManager.getProperty("org.dspace.app.batchitemexport.work.dir") +
"' as defined by the key 'org.dspace.app.batchitemexport.work.dir' in dspace.cfg " +
"is not a valid directory");
}

if (!tempdir.exists() && !tempdir.mkdirs())
{
log.error("Unable to create temporary directory: " + tempdir.getAbsolutePath());
}
String sourcedir = destinationDir + System.getProperty("file.separator") + zipfile.getName();
String zipDir = destinationDir + System.getProperty("file.separator") + zipfile.getName() + System.getProperty("file.separator");

if(!destinationDir.endsWith(System.getProperty("file.separator"))) {
destinationDir += System.getProperty("file.separator");
}

String sourcedir = destinationDir + zipfile.getName();
String zipDir = destinationDir + zipfile.getName() + System.getProperty("file.separator");

log.debug("zip directory to use is " + zipDir);


// 3
Expand All @@ -1660,11 +1672,27 @@ public String unzip(File zipfile, String destDir) throws IOException {
while (entries.hasMoreElements())
{
entry = entries.nextElement();
// Check that the true path to extract files is never outside allowed temp directories
// without creating any actual files on disk
log.debug("Inspecting entry name: " + entry.getName() + " for path traversal security");
File potentialExtract = new File(zipDir + entry.getName());
String canonicalPath = potentialExtract.getCanonicalPath();
log.debug("Canonical path to potential File is " + canonicalPath);
if(!canonicalPath.startsWith(zipDir)) {
log.error("Rejecting zip file: " + zipfile.getName() + " as it contains an entry that would be extracted " +
"outside the temporary unzip directory: " + canonicalPath);
throw new IOException("Error extracting " + zipfile + ": Canonical path of zip entry: " +
entry.getName() + " (" + canonicalPath + ") does not start with permissible temp unzip directory (" + destinationDir +
")");
}

if (entry.isDirectory())
{
if (!new File(zipDir + entry.getName()).mkdir())
{
// Log error and throw IOException if a directory entry could not be created
File newDir = new File(zipDir + entry.getName());
if (!newDir.mkdirs()) {
log.error("Unable to create contents directory: " + zipDir + entry.getName());
throw new IOException("Unable to create contents directory: " + zipDir + entry.getName());
}
}
else
Expand All @@ -1673,6 +1701,7 @@ public String unzip(File zipfile, String destDir) throws IOException {
log.info("Extracting file: " + entry.getName());

int index = entry.getName().lastIndexOf('/');
log.debug("Index of " + entry.getName() + " is " + index);
if (index == -1)
{
// Was it created on Windows instead?
Expand Down Expand Up @@ -1701,11 +1730,11 @@ public String unzip(File zipfile, String destDir) throws IOException {
}
}


}
byte[] buffer = new byte[1024];
int len;
InputStream in = zf.getInputStream(entry);
log.debug("Reading " + zipDir + entry.getName() + " into InputStream");
BufferedOutputStream out = new BufferedOutputStream(
new FileOutputStream(zipDir + entry.getName()));
while((len = in.read(buffer)) >= 0)
Expand Down

2 comments on commit 7af52a0

@eulereadgbe
Copy link

Choose a reason for hiding this comment

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

It seems that this commit doesn't work in a Windows environment. I just tested this in a fresh install of Windows and the import failed. The log shows this:
ERROR org.dspace.app.itemimport.ItemImportServiceImpl @ Rejecting zip file: SimpleArchiveFormat.zip as it contains an entry that would be extracted outside the temporary unzip directory: C:\DSpace\imports\SimpleArchiveFormat.zip\SimpleArchiveFormat

From the command line, it returns this error:

java.io.IOException: Error extracting C:\Users\enemiz\Documents\SimpleArchiveFormat.zip: Canonical path of zip entry: SimpleArchiveFormat/ (C:\DSpace\imports\SimpleArchiveFormat.zip\SimpleArchiveFormat) does not start with permissible temp unzip directory (C:\DSpace/imports\)
        at org.dspace.app.itemimport.ItemImportServiceImpl.unzip(ItemImportServiceImpl.java:1685)
        at org.dspace.app.itemimport.ItemImportServiceImpl.unzip(ItemImportServiceImpl.java:1624)
        at org.dspace.app.itemimport.ItemImportServiceImpl.unzip(ItemImportServiceImpl.java:1764)
        at org.dspace.app.itemimport.ItemImportCLITool.main(ItemImportCLITool.java:368)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
        at java.lang.reflect.Method.invoke(Unknown Source)
        at org.dspace.app.launcher.ScriptLauncher.runOneCommand(ScriptLauncher.java:229)
        at org.dspace.app.launcher.ScriptLauncher.main(ScriptLauncher.java:81) java.io.IOException: Error extracting C:\Users\enemiz\Documents\SimpleArchiveFormat.zip: Canonical path of zip entry: SimpleArchiveFormat/ (C:\DSpace\imports\SimpleArchiveFormat.zip\SimpleArchiveFormat) does not start with permissible temp unzip directory (C:\DSpace/imports\)

The Batch Import (ZIP) in the web UI doesn't also work.

Using the same zip file, the import was successful when I tested this on a fresh install of Ubuntu.

@kshepherd
Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @eulereadgbe , I suspect either System.getProperty("file.separator") didn't do what we expect for windows, or we're not using it consistently to construct the path

Please sign in to comment.