You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ofbiz.apache.org by ja...@apache.org on 2026/05/05 08:00:42 UTC
(ofbiz-framework) branch release24.09 updated: Fixed: Improve file path validation to work with mounted directories
This is an automated email from the ASF dual-hosted git repository.
jacopoc pushed a commit to branch release24.09
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
The following commit(s) were added to refs/heads/release24.09 by this push:
new d094e5a411 Fixed: Improve file path validation to work with mounted directories
d094e5a411 is described below
commit d094e5a4115c7f056096a3e1d6583a001aef28cc
Author: Jacopo Cappellato <ja...@gmail.com>
AuthorDate: Fri May 1 08:40:38 2026 +0200
Fixed: Improve file path validation to work with mounted directories
File.getCanonicalPath() resolves all symlinks in a path. OS-level mount points (Docker bind mounts, EFS) appear as symlinks. When a subdirectory like runtime/ is mounted, a file's canonical path resolves to the mount target (e.g. /mnt/efs/data/file.pdf), which no longer shares a prefix with the canonical path of ofbiz.home (/opt/ofbiz)
(cherry picked from commit b3eadf1cff2e3431e57c1f2002333fb22e0a458a)
---
.../ofbiz/content/data/DataResourceWorker.java | 15 +++-
.../org/apache/ofbiz/security/SecurityUtil.java | 13 ++--
.../apache/ofbiz/security/SecurityUtilTest.java | 82 ++++++++++++++++++++++
3 files changed, 104 insertions(+), 6 deletions(-)
diff --git a/applications/content/src/main/java/org/apache/ofbiz/content/data/DataResourceWorker.java b/applications/content/src/main/java/org/apache/ofbiz/content/data/DataResourceWorker.java
index 1948489166..aa49223991 100644
--- a/applications/content/src/main/java/org/apache/ofbiz/content/data/DataResourceWorker.java
+++ b/applications/content/src/main/java/org/apache/ofbiz/content/data/DataResourceWorker.java
@@ -37,6 +37,7 @@ import java.net.UnknownHostException;
import java.nio.ByteBuffer;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
+import java.nio.file.Path;
import java.nio.file.StandardOpenOption;
import java.util.Comparator;
import java.util.HashMap;
@@ -473,12 +474,24 @@ public class DataResourceWorker implements org.apache.ofbiz.widget.content.DataR
/**
* Checks that the given file is within the provided context root directory.
+ * Uses a dual-check strategy to support EFS/Docker mount points:
+ * 1. Canonical paths (resolves symlinks on both sides) — works for non-mounted paths.
+ * 2. Normalized absolute paths (collapses ".." without following symlinks) — fallback for
+ * when contextRoot or a subdirectory inside it is a mount point, causing canonical paths
+ * to diverge. Path traversal via ".." is still blocked by the normalization step.
*/
static void checkContextFileBoundary(File file, String contextRoot) throws GeneralException {
try {
String canonicalAllowed = new File(contextRoot).getCanonicalPath();
String canonicalFilePath = file.getCanonicalPath();
- if (!canonicalFilePath.startsWith(canonicalAllowed + File.separator)) {
+ boolean passesCanonical = canonicalFilePath.startsWith(canonicalAllowed + File.separator)
+ || canonicalFilePath.equals(canonicalAllowed);
+
+ Path normalizedAllowed = Path.of(contextRoot).toAbsolutePath().normalize();
+ Path normalizedFilePath = file.toPath().toAbsolutePath().normalize();
+ boolean passesNormalized = normalizedFilePath.startsWith(normalizedAllowed);
+
+ if (!passesCanonical && !passesNormalized) {
throw new GeneralException("Access to file denied: path resolves outside of the allowed directory");
}
} catch (IOException e) {
diff --git a/framework/security/src/main/java/org/apache/ofbiz/security/SecurityUtil.java b/framework/security/src/main/java/org/apache/ofbiz/security/SecurityUtil.java
index ce61571985..fd0d9f38a8 100644
--- a/framework/security/src/main/java/org/apache/ofbiz/security/SecurityUtil.java
+++ b/framework/security/src/main/java/org/apache/ofbiz/security/SecurityUtil.java
@@ -174,16 +174,19 @@ public final class SecurityUtil {
}
/**
- * Checks that the given file is within the OFBiz home directory and within one of the
- * subdirectories listed in {@code content.data.ofbiz.file.allowed.paths} (security.properties).
+ * Checks that the given file is within one of the subdirectories listed in
+ * {@code content.data.ofbiz.file.allowed.paths} (security.properties), relative to the OFBiz
+ * home directory. The check uses canonical paths (resolving symlinks on both sides), so
+ * EFS/Docker volume mounts on allowed subdirectories are handled correctly. A preliminary
+ * "must be under ofbiz.home" canonical check is intentionally absent: when an allowed
+ * subdirectory (e.g. {@code runtime/}) is a mount point, the file's canonical path diverges
+ * from {@code canonicalHome}, but the per-allowed-path comparison below still passes because
+ * it resolves both sides through the mount. Path traversal via {@code ../} is still blocked.
*/
public static void checkOfbizFileAllowList(File file) throws GeneralException {
try {
String canonicalHome = new File(System.getProperty("ofbiz.home")).getCanonicalPath();
String canonicalFilePath = file.getCanonicalPath();
- if (!canonicalFilePath.startsWith(canonicalHome + File.separator)) {
- throw new GeneralException("Access to file denied: path resolves outside of the OFBiz home directory");
- }
String allowedPathsStr = UtilProperties.getPropertyValue("security",
"content.data.ofbiz.file.allowed.paths", "applications/,themes/,plugins/,runtime/");
if (UtilValidate.isNotEmpty(allowedPathsStr)) {
diff --git a/framework/security/src/test/java/org/apache/ofbiz/security/SecurityUtilTest.java b/framework/security/src/test/java/org/apache/ofbiz/security/SecurityUtilTest.java
index 2dae4f7b88..13d6245fa6 100644
--- a/framework/security/src/test/java/org/apache/ofbiz/security/SecurityUtilTest.java
+++ b/framework/security/src/test/java/org/apache/ofbiz/security/SecurityUtilTest.java
@@ -20,13 +20,95 @@ package org.apache.ofbiz.security;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+import java.io.File;
+import java.nio.file.Files;
+import java.nio.file.Path;
import java.util.Arrays;
+import java.util.Comparator;
import java.util.List;
+import org.apache.ofbiz.base.util.GeneralException;
+import org.junit.After;
+import org.junit.Before;
import org.junit.Test;
public class SecurityUtilTest {
+
+ private Path tempHome;
+ private Path tempExternal;
+ private String previousOfbizHome;
+
+ @Before
+ public void setUpTempDirs() throws Exception {
+ tempHome = Files.createTempDirectory("ofbiz-home-test");
+ tempExternal = Files.createTempDirectory("ofbiz-ext-test");
+ previousOfbizHome = System.getProperty("ofbiz.home");
+ System.setProperty("ofbiz.home", tempHome.toString());
+ }
+
+ @After
+ public void tearDownTempDirs() throws Exception {
+ if (previousOfbizHome != null) {
+ System.setProperty("ofbiz.home", previousOfbizHome);
+ } else {
+ System.clearProperty("ofbiz.home");
+ }
+ deleteDirRecursively(tempHome);
+ deleteDirRecursively(tempExternal);
+ }
+
+ private static void deleteDirRecursively(Path dir) throws Exception {
+ if (dir != null && Files.exists(dir)) {
+ Files.walk(dir).sorted(Comparator.reverseOrder()).map(Path::toFile).forEach(File::delete);
+ }
+ }
+
+ @Test
+ public void checkOfbizFileAllowListAcceptsFileInAllowedDir() throws Exception {
+ Path runtimeDir = Files.createDirectory(tempHome.resolve("runtime"));
+ Path file = Files.createTempFile(runtimeDir, "upload", ".dat");
+ SecurityUtil.checkOfbizFileAllowList(file.toFile());
+ }
+
+ @Test
+ public void checkOfbizFileAllowListRejectsFileOutsideAllowedDir() throws Exception {
+ Path forbiddenDir = Files.createDirectory(tempHome.resolve("forbidden"));
+ Path file = Files.createTempFile(forbiddenDir, "upload", ".dat");
+ try {
+ SecurityUtil.checkOfbizFileAllowList(file.toFile());
+ fail("Expected GeneralException for file outside allowed dirs");
+ } catch (GeneralException e) {
+ assertTrue(e.getMessage().contains("not within an allowed directory"));
+ }
+ }
+
+ @Test
+ public void checkOfbizFileAllowListAcceptsFileUnderSymlinkedAllowedDir() throws Exception {
+ // Simulates an EFS/Docker volume mount: runtime/ is a symlink to an external directory.
+ // Prior to the fix, the home-boundary canonical check incorrectly rejected this because
+ // the file's canonical path diverged from canonicalHome.
+ Path runtimeLink = tempHome.resolve("runtime");
+ Files.createSymbolicLink(runtimeLink, tempExternal);
+ Path file = Files.createTempFile(tempExternal, "upload", ".dat");
+ SecurityUtil.checkOfbizFileAllowList(file.toFile());
+ }
+
+ @Test
+ public void checkOfbizFileAllowListRejectsPathTraversal() throws Exception {
+ Path runtimeDir = Files.createDirectory(tempHome.resolve("runtime"));
+ // Construct a path that tries to escape via ".." — getCanonicalPath resolves this.
+ File traversalFile = new File(runtimeDir.toFile(), "../../etc/passwd");
+ try {
+ SecurityUtil.checkOfbizFileAllowList(traversalFile);
+ fail("Expected GeneralException for path traversal attempt");
+ } catch (GeneralException e) {
+ assertTrue(e.getMessage().contains("not within an allowed directory"));
+ }
+ }
+
+
@Test
public void basicAdminPermissionTesting() {
List<String> adminPermissions = Arrays.asList("PARTYMGR", "EXAMPLE", "ACCTG_PREF");