This is an automated email from the ASF dual-hosted git repository.
mridulpathak pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
The following commit(s) were added to refs/heads/trunk by this push:
new 783370b10e Improved: Return the validated file from
SecurityUtil.checkOfbizFileAllowList and use it for the file access
(OFBIZ-13632)
783370b10e is described below
commit 783370b10e6f9676767a91bb2b5f61ac784eca46
Author: Mridul Pathak <[email protected]>
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 0398e3447b..80f18c1d00 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
@@ -492,21 +492,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());
}
@@ -645,13 +652,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");
@@ -659,11 +666,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!");
@@ -673,8 +679,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));
}
@@ -1233,10 +1238,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);
}
@@ -1246,11 +1251,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);
}
@@ -1260,8 +1264,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 a2bf88c23d..a057615754 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 2b202d7fca..5adcf87f1a 100644
--- a/framework/security/config/security.properties
+++ b/framework/security/config/security.properties
@@ -178,7 +178,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 36688ba1e5..6590729612 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";
