You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ofbiz.apache.org by mr...@apache.org on 2026/10/01 16:19:31 UTC
(ofbiz-framework) branch release24.09 updated: Improved: Return the validated file from SecurityUtil.checkOfbizFileAllowList and use it for the file access (OFBIZ-13632)
This is an automated email from the ASF dual-hosted git repository.
mridulpathak 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 6815f43098 Improved: Return the validated file from SecurityUtil.checkOfbizFileAllowList and use it for the file access (OFBIZ-13632)
6815f43098 is described below
commit 6815f43098f2bbb5e23d05bacdb4d4770db16ce0
Author: Mridul Pathak <mr...@gmail.com>
AuthorDate: Thu Oct 1 18:49:10 2026 +0530
Improved: Return the validated file from SecurityUtil.checkOfbizFileAllowList and use it for the file access (OFBIZ-13632)
- SecurityUtil.checkOfbizFileAllowList() and DataResourceWorker.checkContextFileBoundary() now return the validated file, and CoreEvents.streamFile(), CommonEvents.openSourceFile(), DataResourceWorker.getContentFile() and DataResourceWorker.renderFile() use it for the file access instead of the original path.
- The unreachable branch for an empty allow-list is removed from checkOfbizFileAllowList(): UtilProperties.getPropertyValue() returns the default value for an empty setting, so the default list already applies. The comment in security.properties, which said an empty value allows any path within ofbiz.home, is corrected.
- In DataResourceWorker, the LOCAL_FILE and OFBIZ_FILE paths are checked before file.exists() is called.
The behavior for valid paths is unchanged.
---
.../ofbiz/content/data/DataResourceWorker.java | 41 ++++++++++++----------
.../java/org/apache/ofbiz/common/CommonEvents.java | 4 +--
framework/security/config/security.properties | 2 +-
.../org/apache/ofbiz/security/SecurityUtil.java | 31 ++++++++--------
.../org/apache/ofbiz/webapp/event/CoreEvents.java | 4 +--
5 files changed, 42 insertions(+), 40 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 dc0fd3656e..d730b443c3 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
@@ -489,21 +489,28 @@ public class DataResourceWorker implements org.apache.ofbiz.widget.content.DataR
* 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.
+ *
+ * @param file the file to check
+ * @param contextRoot the directory the file has to be in
+ * @return the validated file (canonical, or normalized when the canonical check does not apply),
+ * which callers should use for the actual file access
+ * @throws GeneralException when the file resolves outside of the context root
*/
- static void checkContextFileBoundary(File file, String contextRoot) throws GeneralException {
+ static File checkContextFileBoundary(File file, String contextRoot) throws GeneralException {
try {
String canonicalAllowed = new File(contextRoot).getCanonicalPath();
String canonicalFilePath = file.getCanonicalPath();
- boolean passesCanonical = canonicalFilePath.startsWith(canonicalAllowed + File.separator)
- || canonicalFilePath.equals(canonicalAllowed);
+ if (canonicalFilePath.startsWith(canonicalAllowed + File.separator)
+ || canonicalFilePath.equals(canonicalAllowed)) {
+ return new File(canonicalFilePath);
+ }
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");
+ if (normalizedFilePath.startsWith(normalizedAllowed)) {
+ return normalizedFilePath.toFile();
}
+ throw new GeneralException("Access to file denied: path resolves outside of the allowed directory");
} catch (IOException e) {
throw new GeneralException("Unable to validate file path: " + e.getMessage());
}
@@ -642,13 +649,13 @@ public class DataResourceWorker implements org.apache.ofbiz.widget.content.DataR
if ("LOCAL_FILE".equals(dataResourceTypeId) || "LOCAL_FILE_BIN".equals(dataResourceTypeId)) {
file = FileUtil.getFile(objectInfo);
- if (!file.exists()) {
- throw new FileNotFoundException("No file found: " + (objectInfo));
- }
if (!file.isAbsolute()) {
throw new GeneralException("File (" + objectInfo + ") is not absolute");
}
SecurityUtil.checkLocalFileAllowList(file);
+ if (!file.exists()) {
+ throw new FileNotFoundException("No file found: " + (objectInfo));
+ }
} else if ("OFBIZ_FILE".equals(dataResourceTypeId) || "OFBIZ_FILE_BIN".equals(dataResourceTypeId)) {
String prefix = System.getProperty("ofbiz.home");
@@ -656,11 +663,10 @@ public class DataResourceWorker implements org.apache.ofbiz.widget.content.DataR
if (objectInfo.indexOf('/') != 0 && prefix.lastIndexOf('/') != (prefix.length() - 1)) {
sep = "/";
}
- file = FileUtil.getFile(prefix + sep + objectInfo);
+ file = SecurityUtil.checkOfbizFileAllowList(FileUtil.getFile(prefix + sep + objectInfo));
if (!file.exists()) {
throw new FileNotFoundException("No file found: " + (prefix + sep + objectInfo));
}
- SecurityUtil.checkOfbizFileAllowList(file);
} else if ("CONTEXT_FILE".equals(dataResourceTypeId) || "CONTEXT_FILE_BIN".equals(dataResourceTypeId)) {
if (UtilValidate.isEmpty(contextRoot)) {
throw new GeneralException("Cannot find CONTEXT_FILE with an empty context root!");
@@ -670,8 +676,7 @@ public class DataResourceWorker implements org.apache.ofbiz.widget.content.DataR
if (objectInfo.indexOf('/') != 0 && contextRoot.lastIndexOf('/') != (contextRoot.length() - 1)) {
sep = "/";
}
- file = FileUtil.getFile(contextRoot + sep + objectInfo);
- checkContextFileBoundary(file, contextRoot);
+ file = checkContextFileBoundary(FileUtil.getFile(contextRoot + sep + objectInfo), contextRoot);
if (!file.exists()) {
throw new FileNotFoundException("No file found: " + (contextRoot + sep + objectInfo));
}
@@ -1230,10 +1235,10 @@ public class DataResourceWorker implements org.apache.ofbiz.widget.content.DataR
if (!file.isAbsolute()) {
throw new GeneralException("File (" + objectInfo + ") is not absolute");
}
+ SecurityUtil.checkLocalFileAllowList(file);
if (!file.exists()) {
throw new FileNotFoundException("No file found: " + file.getAbsolutePath());
}
- SecurityUtil.checkLocalFileAllowList(file);
try (InputStreamReader in = new InputStreamReader(new FileInputStream(file), StandardCharsets.UTF_8)) {
UtilIO.copy(in, out);
}
@@ -1243,11 +1248,10 @@ public class DataResourceWorker implements org.apache.ofbiz.widget.content.DataR
if (objectInfo.indexOf('/') != 0 && prefix.lastIndexOf('/') != (prefix.length() - 1)) {
sep = "/";
}
- File file = FileUtil.getFile(prefix + sep + objectInfo);
+ File file = SecurityUtil.checkOfbizFileAllowList(FileUtil.getFile(prefix + sep + objectInfo));
if (!file.exists()) {
throw new FileNotFoundException("No file found: " + file.getAbsolutePath());
}
- SecurityUtil.checkOfbizFileAllowList(file);
try (InputStreamReader in = new InputStreamReader(new FileInputStream(file), StandardCharsets.UTF_8)) {
UtilIO.copy(in, out);
}
@@ -1257,8 +1261,7 @@ public class DataResourceWorker implements org.apache.ofbiz.widget.content.DataR
if (objectInfo.indexOf('/') != 0 && prefix.lastIndexOf('/') != (prefix.length() - 1)) {
sep = "/";
}
- File file = FileUtil.getFile(prefix + sep + objectInfo);
- checkContextFileBoundary(file, rootDir);
+ File file = checkContextFileBoundary(FileUtil.getFile(prefix + sep + objectInfo), rootDir);
if (!file.exists()) {
throw new FileNotFoundException("No file found: " + file.getAbsolutePath());
}
diff --git a/framework/common/src/main/java/org/apache/ofbiz/common/CommonEvents.java b/framework/common/src/main/java/org/apache/ofbiz/common/CommonEvents.java
index 855378c9c2..a703401e79 100644
--- a/framework/common/src/main/java/org/apache/ofbiz/common/CommonEvents.java
+++ b/framework/common/src/main/java/org/apache/ofbiz/common/CommonEvents.java
@@ -419,11 +419,11 @@ public class CommonEvents {
if (!platformSpecificPath.contains(File.separator) && "\\".equals(File.separator)) {
platformSpecificPath = platformSpecificPath.replace("/", "\\");
}
- SecurityUtil.checkOfbizFileAllowList(new File(platformSpecificPath));
+ File sourceFile = SecurityUtil.checkOfbizFileAllowList(new File(platformSpecificPath));
// get line number
int lineNumber = 1;
if (UtilValidate.isNotEmpty(fragment)) {
- try (LineNumberReader lnr = new LineNumberReader(new FileReader(platformSpecificPath))) {
+ try (LineNumberReader lnr = new LineNumberReader(new FileReader(sourceFile))) {
String line;
while ((line = lnr.readLine()) != null) {
if (line.matches(".*name=\"" + Pattern.quote(fragment) + "\".*")) {
diff --git a/framework/security/config/security.properties b/framework/security/config/security.properties
index f32e1f3c1f..a53645b4d1 100644
--- a/framework/security/config/security.properties
+++ b/framework/security/config/security.properties
@@ -163,7 +163,7 @@ security.token.key=
# -- Allowed subdirectories (relative to ofbiz.home) for the OFBIZ_FILE / OFBIZ_FILE_BIN data resource types.
# -- Comma-separated list, no spaces after commas, no leading slashes.
# -- Only files whose resolved canonical path starts with one of these entries will be served.
-# -- Set to empty to allow any path within ofbiz.home (the ofbiz.home containment check still applies).
+# -- An empty or missing value means the default list below is used.
content.data.ofbiz.file.allowed.paths=applications/,themes/,plugins/,runtime/
# -- Allowed directories for the LOCAL_FILE / LOCAL_FILE_BIN data resource types (absolute paths).
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 fd0d9f38a8..f9025770cb 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
@@ -182,31 +182,30 @@ public final class SecurityUtil {
* 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.
+ * An empty or missing setting means the default list is used.
+ *
+ * @param file the file to check
+ * @return the canonical file, which callers should use for the actual file access
+ * @throws GeneralException when the file is not within an allowed directory
*/
- public static void checkOfbizFileAllowList(File file) throws GeneralException {
+ public static File checkOfbizFileAllowList(File file) throws GeneralException {
try {
String canonicalHome = new File(System.getProperty("ofbiz.home")).getCanonicalPath();
String canonicalFilePath = file.getCanonicalPath();
String allowedPathsStr = UtilProperties.getPropertyValue("security",
"content.data.ofbiz.file.allowed.paths", "applications/,themes/,plugins/,runtime/");
- if (UtilValidate.isNotEmpty(allowedPathsStr)) {
- boolean inAllowedPath = false;
- for (String relPath : allowedPathsStr.split(",")) {
- relPath = relPath.trim().replaceAll("^/+", "");
- if (UtilValidate.isEmpty(relPath)) {
- continue;
- }
- String canonicalAllowedDir = new File(canonicalHome, relPath).getCanonicalPath();
- if (canonicalFilePath.startsWith(canonicalAllowedDir + File.separator)
- || canonicalFilePath.equals(canonicalAllowedDir)) {
- inAllowedPath = true;
- break;
- }
+ for (String relPath : allowedPathsStr.split(",")) {
+ relPath = relPath.trim().replaceAll("^/+", "");
+ if (UtilValidate.isEmpty(relPath)) {
+ continue;
}
- if (!inAllowedPath) {
- throw new GeneralException("Access to file denied: path is not within an allowed directory");
+ String canonicalAllowedDir = new File(canonicalHome, relPath).getCanonicalPath();
+ if (canonicalFilePath.startsWith(canonicalAllowedDir + File.separator)
+ || canonicalFilePath.equals(canonicalAllowedDir)) {
+ return new File(canonicalFilePath);
}
}
+ throw new GeneralException("Access to file denied: path is not within an allowed directory");
} catch (IOException e) {
throw new GeneralException("Unable to validate file path: " + e.getMessage());
}
diff --git a/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/CoreEvents.java b/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/CoreEvents.java
index aab160ba77..caeb0cc2e9 100644
--- a/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/CoreEvents.java
+++ b/framework/webapp/src/main/java/org/apache/ofbiz/webapp/event/CoreEvents.java
@@ -506,9 +506,9 @@ public class CoreEvents {
String filePath = RequestHandler.getOverrideViewUri(request.getPathInfo());
// load the file
- File file = new File(filePath);
+ File file;
try {
- SecurityUtil.checkOfbizFileAllowList(file);
+ file = SecurityUtil.checkOfbizFileAllowList(new File(filePath));
} catch (GeneralException e) {
Debug.logError(e.getMessage(), MODULE);
return "error";