Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
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;
Expand Down Expand Up @@ -471,12 +472,24 @@ public static String buildRequestPrefix(Delegator delegator, Locale locale, Stri

/**
* 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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -174,16 +174,19 @@ public static void checkLocalFileAllowList(File file) throws GeneralException {
}

/**
* 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)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,95 @@

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");
Expand Down
Loading