From 09a1f7530d944638d286d4e0c3c34b00e5ea5411 Mon Sep 17 00:00:00 2001 From: XingY Date: Thu, 18 Sep 2025 14:47:10 -0700 Subject: [PATCH 1/3] Arbitrary file access during archive extraction ("Zip Slip") --- flow/enginesrc/org/labkey/flow/Main.java | 15 +++++++++------ .../labkey/flow/persist/AnalysisSerializer.java | 16 +++++++++------- .../labkey/flow/controllers/WorkspaceData.java | 5 +++-- 3 files changed, 21 insertions(+), 15 deletions(-) diff --git a/flow/enginesrc/org/labkey/flow/Main.java b/flow/enginesrc/org/labkey/flow/Main.java index 74cbb0175..c5ec419e7 100644 --- a/flow/enginesrc/org/labkey/flow/Main.java +++ b/flow/enginesrc/org/labkey/flow/Main.java @@ -37,6 +37,7 @@ import org.labkey.flow.persist.AnalysisSerializer; import org.labkey.flow.persist.AttributeSet; import org.labkey.flow.persist.ObjectType; +import org.labkey.vfs.FileLike; import org.w3c.dom.Document; import javax.xml.transform.OutputKeys; @@ -51,6 +52,7 @@ import java.io.IOException; import java.io.InputStream; import java.io.PrintStream; +import java.nio.file.Path; import java.util.ArrayList; import java.util.Collection; import java.util.EnumSet; @@ -383,7 +385,7 @@ private static ExternalAnalysis readTsvAnalysisResults(File analysisResultsFile, if (analysisResultsFile.getName().endsWith(".zip")) { // NOTE: Duplicated code in AnalysisScriptController - File statisticsFile; + FileLike statisticsFile; java.util.zip.ZipFile zipFile; try { @@ -409,12 +411,13 @@ private static ExternalAnalysis readTsvAnalysisResults(File analysisResultsFile, // UNDONE: instead of unzipping into temp dir, make zip VirtualFile impl readable. try { - File tmpDir = FileUtil.createTempDirectory("flow").toFile(); - tmpDir.deleteOnExit(); + FileLike tmpDirFileLike = FileUtil.createTempDirectoryFileLike("flow"); + Path tmpDirFilePath = tmpDirFileLike.toNioPathForWrite(); + tmpDirFilePath.toFile().deleteOnExit(); - ZipUtil.unzipToDirectory(analysisResultsFile, tmpDir); - statisticsFile = new File(tmpDir, zipEntry.getName()); - rootDir = new FileSystemFile(statisticsFile.getParentFile()); + ZipUtil.unzipToDirectory(analysisResultsFile.toPath(), tmpDirFilePath); + statisticsFile = tmpDirFileLike.resolveChild(zipEntry.getName()); + rootDir = new FileSystemFile(statisticsFile.getParent().toNioPathForRead()); } catch (IOException ioe) { diff --git a/flow/enginesrc/org/labkey/flow/persist/AnalysisSerializer.java b/flow/enginesrc/org/labkey/flow/persist/AnalysisSerializer.java index 18b7e5f87..9dee7589b 100644 --- a/flow/enginesrc/org/labkey/flow/persist/AnalysisSerializer.java +++ b/flow/enginesrc/org/labkey/flow/persist/AnalysisSerializer.java @@ -42,6 +42,7 @@ import org.labkey.flow.analysis.web.GraphSpec; import org.labkey.flow.analysis.web.StatisticSpec; import org.labkey.flow.analysis.web.SubsetSpec; +import org.labkey.vfs.FileLike; import java.io.File; import java.io.IOException; @@ -253,7 +254,7 @@ public void error(String msg, Throwable t) } } - public static File extractArchive(File file, File tempDir) + public static File extractArchive(File file, FileLike tempDir) throws IOException { File statisticsFile = null; @@ -271,13 +272,13 @@ else if (file.getName().endsWith(".zip")) ZipFile zipFile = new ZipFile(file); - File importDir; + FileLike importDir; String zipBaseName = FileUtil.getBaseName(file); ZipEntry zipEntry = zipFile.getEntry(AnalysisSerializer.STATISTICS_FILENAME); if (zipEntry != null) { // Create extra directory under tempDir to extract into. - importDir = new File(tempDir, zipBaseName); + importDir = tempDir.resolveChild(zipBaseName); } else { @@ -288,12 +289,13 @@ else if (file.getName().endsWith(".zip")) if (zipEntry == null) throw new IOException("Couldn't find '" + AnalysisSerializer.STATISTICS_FILENAME + "' or '" + zipBaseName + "/" + AnalysisSerializer.STATISTICS_FILENAME + "' in the zip archive."); - //File importDir = File.createTempFile(zipBaseName, null, tempDir); - if (importDir.exists() && !FileUtil.deleteDir(importDir)) + if (importDir.exists() && !FileUtil.deleteDir(importDir.toNioPathForWrite())) throw new IOException("Could not delete the directory \"" + importDir + "\""); - ZipUtil.unzipToDirectory(file, importDir); - statisticsFile = new File(importDir, zipEntry.getName()); + ZipUtil.unzipToDirectory(file.toPath(), importDir.toNioPathForWrite()); + FileLike statisticsFileLike = importDir.resolveChild(zipEntry.getName()); + if (statisticsFileLike.exists()) + statisticsFile = statisticsFileLike.toNioPathForRead().toFile(); } if (statisticsFile == null) diff --git a/flow/src/org/labkey/flow/controllers/WorkspaceData.java b/flow/src/org/labkey/flow/controllers/WorkspaceData.java index 84815ec1c..ea7addd89 100644 --- a/flow/src/org/labkey/flow/controllers/WorkspaceData.java +++ b/flow/src/org/labkey/flow/controllers/WorkspaceData.java @@ -42,6 +42,7 @@ import org.labkey.flow.data.FlowProtocol; import org.labkey.flow.persist.AnalysisSerializer; import org.labkey.flow.persist.AttributeCache; +import org.labkey.vfs.FileLike; import org.springframework.validation.Errors; import java.io.File; @@ -206,8 +207,8 @@ public void validate(User user, Container container, Errors errors) throws Excep else if (path.endsWith(".zip")) { // Extract external analysis zip into pipeline - File tempDir = pipeRoot.resolvePath(PipelineService.UNZIP_DIR); - if (tempDir.exists() && !FileUtil.deleteDir(tempDir)) + FileLike tempDir = pipeRoot.resolvePathToFileLike(PipelineService.UNZIP_DIR); + if (tempDir != null && tempDir.exists() && !FileUtil.deleteDir(tempDir.toNioPathForWrite())) throw new IOException("Failed to delete temp directory"); String originalPath = path; From 332d21f5f103640c1e508574aa2d1c119989d223 Mon Sep 17 00:00:00 2001 From: XingY Date: Thu, 18 Sep 2025 14:47:48 -0700 Subject: [PATCH 2/3] XML parsing --- .../labkey/flow/analysis/model/CompensationMatrix.java | 10 +++++----- protein/src/org/labkey/protein/ProteinServiceImpl.java | 6 ++---- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/flow/enginesrc/org/labkey/flow/analysis/model/CompensationMatrix.java b/flow/enginesrc/org/labkey/flow/analysis/model/CompensationMatrix.java index 61d4668b3..ae6d97b19 100644 --- a/flow/enginesrc/org/labkey/flow/analysis/model/CompensationMatrix.java +++ b/flow/enginesrc/org/labkey/flow/analysis/model/CompensationMatrix.java @@ -24,6 +24,7 @@ import org.labkey.api.util.DateUtil; import org.labkey.api.util.JunitUtil; import org.labkey.api.util.PageFlowUtil; +import org.labkey.api.util.XmlBeansUtil; import org.labkey.flow.analysis.data.NumberArray; import org.w3c.dom.Document; import org.w3c.dom.Element; @@ -32,7 +33,6 @@ import javax.xml.namespace.QName; import javax.xml.parsers.DocumentBuilder; -import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.parsers.ParserConfigurationException; import javax.xml.transform.Transformer; import javax.xml.transform.TransformerFactory; @@ -131,9 +131,9 @@ public CompensationMatrix(InputStream is) throws Exception public CompensationMatrix(String name, String str) throws Exception { this(name); - DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance(); - dbf.setFeature(Constants.SAX_FEATURE_PREFIX + Constants.NAMESPACES_FEATURE, true); - DocumentBuilder db = dbf.newDocumentBuilder(); + // TODO: with `setNamespaceAware(true)`, is the following still needed? + // DocumentBuilderFactory.setFeature(Constants.SAX_FEATURE_PREFIX + Constants.NAMESPACES_FEATURE, true) + DocumentBuilder db = XmlBeansUtil.DOCUMENT_BUILDER_FACTORY.newDocumentBuilder(); try (var rdr = new StringReader(str)) { Document doc = db.parse(new InputSource(rdr)); @@ -556,7 +556,7 @@ public Document toXML() Document doc; try { - doc = DocumentBuilderFactory.newInstance().newDocumentBuilder().newDocument(); + doc = XmlBeansUtil.DOCUMENT_BUILDER_FACTORY.newDocumentBuilder().newDocument(); } catch (ParserConfigurationException e) { diff --git a/protein/src/org/labkey/protein/ProteinServiceImpl.java b/protein/src/org/labkey/protein/ProteinServiceImpl.java index 269edcdb2..eae2188a3 100644 --- a/protein/src/org/labkey/protein/ProteinServiceImpl.java +++ b/protein/src/org/labkey/protein/ProteinServiceImpl.java @@ -48,6 +48,7 @@ import org.labkey.api.security.User; import org.labkey.api.util.DeadlockPreventingException; import org.labkey.api.util.HtmlString; +import org.labkey.api.util.XmlBeansUtil; import org.labkey.api.util.logging.LogHelper; import org.labkey.api.view.ActionURL; import org.labkey.api.view.JspView; @@ -60,7 +61,6 @@ import org.xml.sax.SAXException; import javax.xml.parsers.DocumentBuilder; -import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.parsers.ParserConfigurationException; import java.io.BufferedReader; import java.io.IOException; @@ -314,9 +314,7 @@ public List load(@NotNull String accession, @Nullable Object arg } } - DocumentBuilderFactory dbf = - DocumentBuilderFactory.newInstance(); - DocumentBuilder db = dbf.newDocumentBuilder(); + DocumentBuilder db = XmlBeansUtil.DOCUMENT_BUILDER_FACTORY.newDocumentBuilder(); InputSource is = new InputSource(); is.setCharacterStream(new StringReader(response.toString())); From 932c507ac55e26293f9382e3e8cc91657dd1589f Mon Sep 17 00:00:00 2001 From: XingY Date: Fri, 19 Sep 2025 17:03:50 -0700 Subject: [PATCH 3/3] remove comment --- .../org/labkey/flow/analysis/model/CompensationMatrix.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/flow/enginesrc/org/labkey/flow/analysis/model/CompensationMatrix.java b/flow/enginesrc/org/labkey/flow/analysis/model/CompensationMatrix.java index ae6d97b19..75dbfac05 100644 --- a/flow/enginesrc/org/labkey/flow/analysis/model/CompensationMatrix.java +++ b/flow/enginesrc/org/labkey/flow/analysis/model/CompensationMatrix.java @@ -131,8 +131,6 @@ public CompensationMatrix(InputStream is) throws Exception public CompensationMatrix(String name, String str) throws Exception { this(name); - // TODO: with `setNamespaceAware(true)`, is the following still needed? - // DocumentBuilderFactory.setFeature(Constants.SAX_FEATURE_PREFIX + Constants.NAMESPACES_FEATURE, true) DocumentBuilder db = XmlBeansUtil.DOCUMENT_BUILDER_FACTORY.newDocumentBuilder(); try (var rdr = new StringReader(str)) {