Skip to content
Permalink
Browse files Browse the repository at this point in the history
[DS-4131] Better path handling in ItemImport zips
  • Loading branch information
kshepherd committed Jul 26, 2022
1 parent 73cdff2 commit 56e7604
Showing 1 changed file with 30 additions and 6 deletions.
36 changes: 30 additions & 6 deletions dspace-api/src/main/java/org/dspace/app/itemimport/ItemImport.java
Expand Up @@ -2003,22 +2003,30 @@ public static String unzip(File zipfile, String destDir) throws IOException {
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.batchitemimport.work.dir") +
"' as defined by the key 'org.dspace.app.batchitemimport.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
String sourceDirForZip = sourcedir;
Expand All @@ -2028,11 +2036,26 @@ public static 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 Down Expand Up @@ -2074,6 +2097,7 @@ public static 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

0 comments on commit 56e7604

Please sign in to comment.