From 0641c29b07999ffe7cf34253930808019c055b98 Mon Sep 17 00:00:00 2001 From: tathagta15 Date: Wed, 17 Jun 2026 16:37:15 +0530 Subject: [PATCH] ACS-11995 Fix CodeQL Issues (#1288) --- .github/workflows/ci.yml | 3 + .github/workflows/dependency-graph.yml | 3 + .github/workflows/precommit_formatter.yml | 5 ++ .../alfresco/transformer/fs/FileManager.java | 43 ++++++++--- .../executors/AbstractCommandExecutor.java | 2 + .../transform/base/fs/FileManager.java | 71 ++++++++++++++++--- .../base/transform/TransformHandler.java | 2 + .../transformers/LibreOfficeTransformer.java | 4 ++ .../HtmlMetadataExtractor.java | 3 +- .../AppleIWorksContentTransformer.java | 3 + .../misc/transformers/EMLTransformer.java | 3 + .../HtmlParserContentTransformer.java | 5 +- .../OOXMLThumbnailContentTransformer.java | 6 +- .../StringExtractingContentTransformer.java | 5 +- .../TextToPdfContentTransformer.java | 5 +- pom.xml | 7 +- 16 files changed, 147 insertions(+), 23 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3fa5ac53..44759444 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -34,6 +34,9 @@ env: GIT_PASSWORD: ${{ secrets.BOT_GITHUB_TOKEN }} GITHUB_ACTIONS_DEPLOY_TIMEOUT: 120 +permissions: + contents: read + jobs: pre_commit: runs-on: ubuntu-latest diff --git a/.github/workflows/dependency-graph.yml b/.github/workflows/dependency-graph.yml index 1611a05c..7d626553 100644 --- a/.github/workflows/dependency-graph.yml +++ b/.github/workflows/dependency-graph.yml @@ -10,6 +10,9 @@ on: workflow_call: workflow_dispatch: +permissions: + contents: write + jobs: scan-dependencies: runs-on: ubuntu-latest diff --git a/.github/workflows/precommit_formatter.yml b/.github/workflows/precommit_formatter.yml index 3205fe70..c0aecb64 100644 --- a/.github/workflows/precommit_formatter.yml +++ b/.github/workflows/precommit_formatter.yml @@ -5,10 +5,15 @@ on: branches: - precommit/** +permissions: + contents: read + jobs: format-code: name: "Reformat code" runs-on: ubuntu-latest + permissions: + contents: write if: contains(github.event.head_commit.message, '[reformat code]') steps: - uses: actions/checkout@v4 diff --git a/deprecated/alfresco-transformer-base/src/main/java/org/alfresco/transformer/fs/FileManager.java b/deprecated/alfresco-transformer-base/src/main/java/org/alfresco/transformer/fs/FileManager.java index 2b4c9ed3..1dccc432 100644 --- a/deprecated/alfresco-transformer-base/src/main/java/org/alfresco/transformer/fs/FileManager.java +++ b/deprecated/alfresco-transformer-base/src/main/java/org/alfresco/transformer/fs/FileManager.java @@ -83,12 +83,14 @@ public class FileManager { filename = checkFilename(false, filename); LogEntry.setTarget(filename); - return TempFileProvider.createTempFile("target_", "_" + filename); + File created = TempFileProvider.createTempFile("target_", "_" + filename); + return assertInsideTempDir(created); } public static void deleteFile(final File file) throws Exception { - if (!file.delete()) + final File safeFile = assertInsideTempDir(file); + if (!safeFile.delete()) { throw new Exception("Failed to delete file"); } @@ -105,8 +107,8 @@ public class FileManager */ private static String checkFilename(boolean source, String filename) { - filename = getFilename(filename); - if (filename == null || filename.isEmpty()) + filename = filename == null ? "" : new File(filename).getName(); + if (filename.isEmpty()) { String sourceOrTarget = source ? "source" : "target"; HttpStatus statusCode = source ? BAD_REQUEST : INTERNAL_SERVER_ERROR; @@ -115,11 +117,32 @@ public class FileManager return filename; } - private static void save(MultipartFile multipartFile, File file) + private static File assertInsideTempDir(File candidate) { try { - Files.copy(multipartFile.getInputStream(), file.toPath(), + File tempRoot = TempFileProvider.getTempDir(); + String candidateCanonical = candidate.getCanonicalPath(); + String parentCanonical = tempRoot.getCanonicalPath(); + if (!candidateCanonical.startsWith(parentCanonical + File.separator) + && !candidateCanonical.equals(parentCanonical)) + { + throw new TransformException(INTERNAL_SERVER_ERROR, "Resolved file escapes the temp directory"); + } + return new File(candidateCanonical); + } + catch (IOException e) + { + throw new TransformException(INTERNAL_SERVER_ERROR, "Unable to resolve canonical path", e); + } + } + + private static void save(MultipartFile multipartFile, File file) + { + final File safeFile = assertInsideTempDir(file); + try + { + Files.copy(multipartFile.getInputStream(), safeFile.toPath(), StandardCopyOption.REPLACE_EXISTING); } catch (IOException e) @@ -130,9 +153,10 @@ public class FileManager public static void save(Resource body, File file) { + final File safeFile = assertInsideTempDir(file); try { - Files.copy(body.getInputStream(), file.toPath(), StandardCopyOption.REPLACE_EXISTING); + Files.copy(body.getInputStream(), safeFile.toPath(), StandardCopyOption.REPLACE_EXISTING); } catch (IOException e) { @@ -142,9 +166,10 @@ public class FileManager private static Resource load(File file) { + final File safeFile = assertInsideTempDir(file); try { - Resource resource = new UrlResource(file.toURI()); + Resource resource = new UrlResource(safeFile.toURI()); if (resource.exists() || resource.isReadable()) { return resource; @@ -222,7 +247,7 @@ public class FileManager String filename = multipartFile.getOriginalFilename(); long size = multipartFile.getSize(); filename = checkFilename(true, filename); - File file = TempFileProvider.createTempFile("source_", "_" + filename); + File file = assertInsideTempDir(TempFileProvider.createTempFile("source_", "_" + filename)); request.setAttribute(SOURCE_FILE, file); save(multipartFile, file); LogEntry.setSource(filename, size); diff --git a/engines/base/src/main/java/org/alfresco/transform/base/executors/AbstractCommandExecutor.java b/engines/base/src/main/java/org/alfresco/transform/base/executors/AbstractCommandExecutor.java index 93053dd1..7537a64f 100644 --- a/engines/base/src/main/java/org/alfresco/transform/base/executors/AbstractCommandExecutor.java +++ b/engines/base/src/main/java/org/alfresco/transform/base/executors/AbstractCommandExecutor.java @@ -30,6 +30,7 @@ import static org.springframework.http.HttpStatus.BAD_REQUEST; import static org.springframework.http.HttpStatus.INTERNAL_SERVER_ERROR; import static org.alfresco.transform.base.executors.RuntimeExec.ExecutionResult; +import static org.alfresco.transform.base.fs.FileManager.assertWithinTempDir; import java.io.File; import java.util.Map; @@ -48,6 +49,7 @@ public abstract class AbstractCommandExecutor implements CommandExecutor @Override public void run(Map properties, File targetFile, Long timeout) { + targetFile = assertWithinTempDir(targetFile); timeout = timeout != null && timeout > 0 ? timeout : 0; final ExecutionResult result = transformCommand.execute(properties, timeout); diff --git a/engines/base/src/main/java/org/alfresco/transform/base/fs/FileManager.java b/engines/base/src/main/java/org/alfresco/transform/base/fs/FileManager.java index f1575bea..38fafbd0 100644 --- a/engines/base/src/main/java/org/alfresco/transform/base/fs/FileManager.java +++ b/engines/base/src/main/java/org/alfresco/transform/base/fs/FileManager.java @@ -41,6 +41,8 @@ import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; import java.net.MalformedURLException; +import java.net.URI; +import java.net.URISyntaxException; import java.net.URL; import java.nio.file.Files; import java.util.UUID; @@ -57,6 +59,7 @@ import org.alfresco.transform.base.logging.LogEntry; import org.alfresco.transform.common.ExtensionService; import org.alfresco.transform.exceptions.TransformException; +@SuppressWarnings("PMD.GodClass") public class FileManager { public static final String SOURCE_FILE = "sourceFile"; @@ -65,6 +68,34 @@ public class FileManager private FileManager() {} + static File assertContained(File candidate, File parent) + { + try + { + String candidateCanonical = candidate.getCanonicalPath(); + String parentCanonical = parent.getCanonicalPath(); + if (!candidateCanonical.equals(parentCanonical) + && !candidateCanonical.startsWith(parentCanonical + File.separator)) + { + throw new TransformException(BAD_REQUEST, "The resolved path escapes the temp directory"); + } + return new File(candidateCanonical); + } + catch (IOException e) + { + throw new TransformException(BAD_REQUEST, "Unable to resolve canonical path", e); + } + } + + public static File assertWithinTempDir(File file) + { + if (file == null) + { + return null; + } + return assertContained(file, new File(System.getProperty("java.io.tmpdir"))); + } + public static File createSourceFile(HttpServletRequest request, InputStream inputStream, String sourceMimetype, String sourceFileName) { try @@ -74,14 +105,15 @@ public class FileManager ? TempFileProvider.createTempFile("source_", extension) : TempFileProvider.createFileWithinUUIDTempDir(sourceFileName); - Files.copy(inputStream, file.toPath(), REPLACE_EXISTING); + File safeFile = assertContained(file, file.getParentFile()); + Files.copy(inputStream, safeFile.toPath(), REPLACE_EXISTING); if (request != null) { - request.setAttribute(SOURCE_FILE, file); + request.setAttribute(SOURCE_FILE, safeFile); } - LogEntry.setSource(file.getName(), file.length()); - return file; + LogEntry.setSource(safeFile.getName(), safeFile.length()); + return safeFile; } catch (Exception e) { @@ -94,7 +126,8 @@ public class FileManager try { String extension = "." + ExtensionService.getExtensionForTargetMimetype(targetMimetype, sourceMimetype); - File file = TempFileProvider.createTempFile("target_", extension); + File raw = TempFileProvider.createTempFile("target_", extension); + File file = assertContained(raw, raw.getParentFile()); if (request != null) { request.setAttribute(TARGET_FILE, file); @@ -160,9 +193,26 @@ public class FileManager { try { - return new URL(directUrl).openStream(); + URL url = new URL(directUrl); + String protocol = url.getProtocol(); + if ("http".equalsIgnoreCase(protocol) || "https".equalsIgnoreCase(protocol)) + { + String host = url.getHost(); + if (host == null || !host.matches("[A-Za-z0-9._\\-]+")) + { + throw new TransformException(BAD_REQUEST, "Direct Access Url host is not allowed."); + } + return new URI(protocol, null, host, url.getPort(), + url.getPath(), url.getQuery(), null).toURL().openStream(); + } + if ("file".equalsIgnoreCase(protocol)) + { + File localFile = assertWithinTempDir(new File(url.toURI())); + return Files.newInputStream(localFile.toPath()); + } + throw new TransformException(BAD_REQUEST, "Direct Access Url protocol is not allowed."); } - catch (IllegalArgumentException e) + catch (URISyntaxException | IllegalArgumentException | MalformedURLException e) { throw new TransformException(BAD_REQUEST, "Direct Access Url is invalid.", e); } @@ -234,7 +284,12 @@ public class FileManager { throw new TransformException(INSUFFICIENT_STORAGE, "Failed to create temp directory: " + tempDir); } - return new File(tempDir, sourceFileName); + String baseName = new File(sourceFileName == null ? "" : sourceFileName).getName(); + if (baseName.isEmpty() || ".".equals(baseName) || "..".equals(baseName)) + { + throw new TransformException(BAD_REQUEST, "The source filename is invalid"); + } + return assertContained(new File(tempDir, baseName), tempDir); } private static File getTempDir() diff --git a/engines/base/src/main/java/org/alfresco/transform/base/transform/TransformHandler.java b/engines/base/src/main/java/org/alfresco/transform/base/transform/TransformHandler.java index 9cbcce82..6165e726 100644 --- a/engines/base/src/main/java/org/alfresco/transform/base/transform/TransformHandler.java +++ b/engines/base/src/main/java/org/alfresco/transform/base/transform/TransformHandler.java @@ -32,6 +32,7 @@ import static org.springframework.http.HttpStatus.BAD_REQUEST; import static org.springframework.http.HttpStatus.CREATED; import static org.springframework.http.HttpStatus.INTERNAL_SERVER_ERROR; +import static org.alfresco.transform.base.fs.FileManager.assertWithinTempDir; import static org.alfresco.transform.base.fs.FileManager.createAttachment; import static org.alfresco.transform.base.fs.FileManager.createTargetFile; import static org.alfresco.transform.base.fs.FileManager.getDirectAccessUrlInputStream; @@ -378,6 +379,7 @@ public class TransformHandler private OutputStream getOutputStreamFromFile(File targetFile) throws IOException { + targetFile = assertWithinTempDir(targetFile); return new BufferedOutputStream(new FileOutputStream(targetFile)); } diff --git a/engines/libreoffice/src/main/java/org/alfresco/transform/libreoffice/transformers/LibreOfficeTransformer.java b/engines/libreoffice/src/main/java/org/alfresco/transform/libreoffice/transformers/LibreOfficeTransformer.java index 700fa9de..e15062fb 100644 --- a/engines/libreoffice/src/main/java/org/alfresco/transform/libreoffice/transformers/LibreOfficeTransformer.java +++ b/engines/libreoffice/src/main/java/org/alfresco/transform/libreoffice/transformers/LibreOfficeTransformer.java @@ -29,6 +29,8 @@ package org.alfresco.transform.libreoffice.transformers; import static org.springframework.http.HttpStatus.BAD_REQUEST; import static org.springframework.http.HttpStatus.INTERNAL_SERVER_ERROR; +import static org.alfresco.transform.base.fs.FileManager.assertWithinTempDir; + import java.io.File; import java.io.IOException; import java.io.Serializable; @@ -137,6 +139,8 @@ public class LibreOfficeTransformer implements JavaExecutor, CustomTransformerFi public void transform(String sourceMimetype, String targetMimetype, Map transformOptions, File sourceFile, File targetFile, TransformManager transformManager) { + sourceFile = assertWithinTempDir(sourceFile); + targetFile = assertWithinTempDir(targetFile); call(sourceFile, targetFile); } diff --git a/engines/misc/src/main/java/org/alfresco/transform/misc/metadataExtractors/HtmlMetadataExtractor.java b/engines/misc/src/main/java/org/alfresco/transform/misc/metadataExtractors/HtmlMetadataExtractor.java index 8f460c9a..dc6a880a 100644 --- a/engines/misc/src/main/java/org/alfresco/transform/misc/metadataExtractors/HtmlMetadataExtractor.java +++ b/engines/misc/src/main/java/org/alfresco/transform/misc/metadataExtractors/HtmlMetadataExtractor.java @@ -26,6 +26,7 @@ */ package org.alfresco.transform.misc.metadataExtractors; +import static org.alfresco.transform.base.fs.FileManager.assertWithinTempDir; import static org.alfresco.transform.base.metadata.AbstractMetadataExtractorEmbedder.Type.EXTRACTOR; import java.io.File; @@ -104,7 +105,7 @@ public class HtmlMetadataExtractor extends AbstractMetadataExtractorEmbedder // This Extractor retries if the encoding needs to be changed, so we need to reread the source, // so cannot use the input stream provided, as it will get closed. - final File sourceFile = transformManager.createSourceFile(); + final File sourceFile = assertWithinTempDir(transformManager.createSourceFile()); HTMLEditorKit.ParserCallback callback = new HTMLEditorKit.ParserCallback() { StringBuffer title = null; diff --git a/engines/misc/src/main/java/org/alfresco/transform/misc/transformers/AppleIWorksContentTransformer.java b/engines/misc/src/main/java/org/alfresco/transform/misc/transformers/AppleIWorksContentTransformer.java index b0233e83..b00428cc 100644 --- a/engines/misc/src/main/java/org/alfresco/transform/misc/transformers/AppleIWorksContentTransformer.java +++ b/engines/misc/src/main/java/org/alfresco/transform/misc/transformers/AppleIWorksContentTransformer.java @@ -26,6 +26,7 @@ */ package org.alfresco.transform.misc.transformers; +import static org.alfresco.transform.base.fs.FileManager.assertWithinTempDir; import static org.alfresco.transform.common.Mimetype.MIMETYPE_IMAGE_JPEG; import java.io.BufferedInputStream; @@ -83,6 +84,8 @@ public class AppleIWorksContentTransformer implements CustomTransformerFileAdapt public void transform(String sourceMimetype, String targetMimetype, Map transformOptions, File sourceFile, File targetFile, TransformManager transformManager) { + sourceFile = assertWithinTempDir(sourceFile); + targetFile = assertWithinTempDir(targetFile); logger.debug("Performing IWorks to jpeg transform with sourceMimetype={} targetMimetype={}", sourceMimetype, targetMimetype); diff --git a/engines/misc/src/main/java/org/alfresco/transform/misc/transformers/EMLTransformer.java b/engines/misc/src/main/java/org/alfresco/transform/misc/transformers/EMLTransformer.java index 794198da..090ceaea 100644 --- a/engines/misc/src/main/java/org/alfresco/transform/misc/transformers/EMLTransformer.java +++ b/engines/misc/src/main/java/org/alfresco/transform/misc/transformers/EMLTransformer.java @@ -26,6 +26,7 @@ */ package org.alfresco.transform.misc.transformers; +import static org.alfresco.transform.base.fs.FileManager.assertWithinTempDir; import static org.alfresco.transform.common.Mimetype.MIMETYPE_HTML; import static org.alfresco.transform.common.Mimetype.MIMETYPE_MULTIPART_ALTERNATIVE; import static org.alfresco.transform.common.Mimetype.MIMETYPE_TEXT_PLAIN; @@ -81,6 +82,8 @@ public class EMLTransformer implements CustomTransformerFileAdaptor public void transform(String sourceMimetype, String targetMimetype, Map transformOptions, File sourceFile, File targetFile, TransformManager transformManager) throws Exception { + sourceFile = assertWithinTempDir(sourceFile); + targetFile = assertWithinTempDir(targetFile); logger.debug("Performing RFC822 to text transform."); // Use try with resource try (InputStream contentInputStream = new BufferedInputStream( diff --git a/engines/misc/src/main/java/org/alfresco/transform/misc/transformers/HtmlParserContentTransformer.java b/engines/misc/src/main/java/org/alfresco/transform/misc/transformers/HtmlParserContentTransformer.java index e49b9255..c2549aad 100644 --- a/engines/misc/src/main/java/org/alfresco/transform/misc/transformers/HtmlParserContentTransformer.java +++ b/engines/misc/src/main/java/org/alfresco/transform/misc/transformers/HtmlParserContentTransformer.java @@ -26,6 +26,7 @@ */ package org.alfresco.transform.misc.transformers; +import static org.alfresco.transform.base.fs.FileManager.assertWithinTempDir; import static org.alfresco.transform.common.RequestParamMap.HTML_COLLAPSE; import static org.alfresco.transform.common.RequestParamMap.SOURCE_ENCODING; @@ -89,8 +90,10 @@ public class HtmlParserContentTransformer implements CustomTransformerFileAdapto @Override public void transform(final String sourceMimetype, final String targetMimetype, final Map transformOptions, - final File sourceFile, final File targetFile, TransformManager transformManager) throws Exception + File sourceFile, File targetFile, TransformManager transformManager) throws Exception { + sourceFile = assertWithinTempDir(sourceFile); + targetFile = assertWithinTempDir(targetFile); String sourceEncoding = transformOptions.get(SOURCE_ENCODING); checkEncodingParameter(sourceEncoding, SOURCE_ENCODING); boolean collapse; diff --git a/engines/misc/src/main/java/org/alfresco/transform/misc/transformers/OOXMLThumbnailContentTransformer.java b/engines/misc/src/main/java/org/alfresco/transform/misc/transformers/OOXMLThumbnailContentTransformer.java index 5620a214..0e5b26f9 100644 --- a/engines/misc/src/main/java/org/alfresco/transform/misc/transformers/OOXMLThumbnailContentTransformer.java +++ b/engines/misc/src/main/java/org/alfresco/transform/misc/transformers/OOXMLThumbnailContentTransformer.java @@ -26,6 +26,8 @@ */ package org.alfresco.transform.misc.transformers; +import static org.alfresco.transform.base.fs.FileManager.assertWithinTempDir; + import java.io.File; import java.io.IOException; import java.io.InputStream; @@ -68,8 +70,10 @@ public class OOXMLThumbnailContentTransformer implements CustomTransformerFileAd @Override public void transform(final String sourceMimetype, final String targetMimetype, final Map parameters, - final File sourceFile, final File targetFile, TransformManager transformManager) throws Exception + File sourceFile, File targetFile, TransformManager transformManager) throws Exception { + sourceFile = assertWithinTempDir(sourceFile); + targetFile = assertWithinTempDir(targetFile); if (logger.isDebugEnabled()) { logger.debug("Performing OOXML to jpeg transform with sourceMimetype=" + sourceMimetype diff --git a/engines/misc/src/main/java/org/alfresco/transform/misc/transformers/StringExtractingContentTransformer.java b/engines/misc/src/main/java/org/alfresco/transform/misc/transformers/StringExtractingContentTransformer.java index 0355ef9c..43e3e600 100644 --- a/engines/misc/src/main/java/org/alfresco/transform/misc/transformers/StringExtractingContentTransformer.java +++ b/engines/misc/src/main/java/org/alfresco/transform/misc/transformers/StringExtractingContentTransformer.java @@ -26,6 +26,7 @@ */ package org.alfresco.transform.misc.transformers; +import static org.alfresco.transform.base.fs.FileManager.assertWithinTempDir; import static org.alfresco.transform.common.RequestParamMap.SOURCE_ENCODING; import static org.alfresco.transform.common.RequestParamMap.TARGET_ENCODING; @@ -79,8 +80,10 @@ public class StringExtractingContentTransformer implements CustomTransformerFile */ @Override public void transform(final String sourceMimetype, final String targetMimetype, final Map transformOptions, - final File sourceFile, final File targetFile, TransformManager transformManager) throws Exception + File sourceFile, File targetFile, TransformManager transformManager) throws Exception { + sourceFile = assertWithinTempDir(sourceFile); + targetFile = assertWithinTempDir(targetFile); String sourceEncoding = transformOptions.get(SOURCE_ENCODING); String targetEncoding = transformOptions.get(TARGET_ENCODING); diff --git a/engines/misc/src/main/java/org/alfresco/transform/misc/transformers/TextToPdfContentTransformer.java b/engines/misc/src/main/java/org/alfresco/transform/misc/transformers/TextToPdfContentTransformer.java index 4f203f0a..54cdadaa 100644 --- a/engines/misc/src/main/java/org/alfresco/transform/misc/transformers/TextToPdfContentTransformer.java +++ b/engines/misc/src/main/java/org/alfresco/transform/misc/transformers/TextToPdfContentTransformer.java @@ -26,6 +26,7 @@ */ package org.alfresco.transform.misc.transformers; +import static org.alfresco.transform.base.fs.FileManager.assertWithinTempDir; import static org.alfresco.transform.common.RequestParamMap.PAGE_LIMIT; import static org.alfresco.transform.common.RequestParamMap.PDF_FONT; import static org.alfresco.transform.common.RequestParamMap.PDF_FONT_SIZE; @@ -149,8 +150,10 @@ public class TextToPdfContentTransformer implements CustomTransformerFileAdaptor @Override public void transform(final String sourceMimetype, final String targetMimetype, final Map transformOptions, - final File sourceFile, final File targetFile, TransformManager transformManager) throws Exception + File sourceFile, File targetFile, TransformManager transformManager) throws Exception { + sourceFile = assertWithinTempDir(sourceFile); + targetFile = assertWithinTempDir(targetFile); String sourceEncoding = transformOptions.get(SOURCE_ENCODING); String stringPageLimit = transformOptions.get(PAGE_LIMIT); int pageLimit = -1; diff --git a/pom.xml b/pom.xml index 84216518..1bb0697a 100644 --- a/pom.xml +++ b/pom.xml @@ -218,12 +218,17 @@ commons-codec 1.17.2 - + org.bouncycastle bcprov-jdk18on ${dependency.bcprov.version} + + org.bouncycastle + bcpkix-jdk18on + ${dependency.bcprov.version} + org.apache.tomcat.embed tomcat-embed-core