This is an automated email from the ASF dual-hosted git repository.
jacopoc 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 da0febe182 Improved: Enhance file upload validation with allowlist and
path checks
da0febe182 is described below
commit da0febe18237aeb4ab0333773c4fe18bb3037699
Author: Jacopo Cappellato <[email protected]>
AuthorDate: Wed Mar 11 08:50:13 2026 +0100
Improved: Enhance file upload validation with allowlist and path checks
---
.../org/apache/ofbiz/content/data/DataEvents.java | 10 +-
.../product/catalog/category/EditCategory.groovy | 6 +
.../config/EditProductConfigItemContent.groovy | 6 +
.../catalog/imagemanagement/ImageUpload.groovy | 6 +
.../catalog/imagemanagement/SetDefaultImage.groovy | 12 +
.../catalog/product/EditProductContent.groovy | 6 +
.../ofbiz/product/product/ProductServices.java | 74 +++-
.../ofbiz/base/util/HttpRequestFileUpload.java | 19 +-
framework/security/config/security.properties | 15 +-
.../org/apache/ofbiz/security/SecuredUpload.java | 400 ++++++++++++++++++++-
.../ofbiz/service/engine/EntityAutoEngine.java | 7 +
11 files changed, 519 insertions(+), 42 deletions(-)
diff --git
a/applications/content/src/main/java/org/apache/ofbiz/content/data/DataEvents.java
b/applications/content/src/main/java/org/apache/ofbiz/content/data/DataEvents.java
index 3a87a1c363..8661d0bbb7 100644
---
a/applications/content/src/main/java/org/apache/ofbiz/content/data/DataEvents.java
+++
b/applications/content/src/main/java/org/apache/ofbiz/content/data/DataEvents.java
@@ -85,9 +85,13 @@ public class DataEvents {
String permissionService =
EntityUtilProperties.getPropertyValue("content", "stream.permission.service",
"genericContentPermission", delegator);
- // For OFBIZ-11840, checks if a webshell is not uploaded
- // It's counterintuitive to return success but it makes sense if you
thing about it.
- // It simply returns a blank screen.
+ // For OFBIZ-11840, validate contentId using a strict allow-list
instead of a
+ // deny-list: entity keys must match [a-zA-Z0-9_:\-]{1,255}. An
allow-list
+ // cannot be bypassed by encoding tricks or token splitting.
+ if (!SecuredUpload.isValidEntityKey(contentId)) {
+ Debug.logError("contentId parameter has invalid format, rejected
for security reason", MODULE);
+ return "success";
+ }
try {
if (!SecuredUpload.isValidText(contentId,
Collections.emptyList())) {
Debug.logError("================== Not saved for security
reason ==================", MODULE);
diff --git
a/applications/product/src/main/groovy/org/apache/ofbiz/product/catalog/category/EditCategory.groovy
b/applications/product/src/main/groovy/org/apache/ofbiz/product/catalog/category/EditCategory.groovy
index d204f9f491..221fe3c919 100644
---
a/applications/product/src/main/groovy/org/apache/ofbiz/product/catalog/category/EditCategory.groovy
+++
b/applications/product/src/main/groovy/org/apache/ofbiz/product/catalog/category/EditCategory.groovy
@@ -74,6 +74,12 @@ if (fileType) {
contentType = '--' + contentType
}
+ // Guard against path traversal: the resolved save directory must remain
inside imageServerPath
+ if (!java.nio.file.Paths.get(imageServerPath + '/' +
filePathPrefix).normalize()
+ .startsWith(java.nio.file.Paths.get(imageServerPath).normalize()))
{
+ logError('Path traversal attempt detected in category image upload')
+ return error(UtilProperties.getMessage('SecurityUiLabels',
'SupportedImageFormats', locale))
+ }
defaultFileName = filenameToUse + '_temp'
uploadObject = new HttpRequestFileUpload()
uploadObject.setOverrideFilename(defaultFileName)
diff --git
a/applications/product/src/main/groovy/org/apache/ofbiz/product/catalog/config/EditProductConfigItemContent.groovy
b/applications/product/src/main/groovy/org/apache/ofbiz/product/catalog/config/EditProductConfigItemContent.groovy
index bf19db8cfd..2ad99f7108 100644
---
a/applications/product/src/main/groovy/org/apache/ofbiz/product/catalog/config/EditProductConfigItemContent.groovy
+++
b/applications/product/src/main/groovy/org/apache/ofbiz/product/catalog/config/EditProductConfigItemContent.groovy
@@ -88,6 +88,12 @@ if (fileType) {
contentType = '--' + contentType
}
+ // Guard against path traversal: the resolved save directory must remain
inside imageServerPath
+ if (!java.nio.file.Paths.get(imageServerPath + '/' +
filePathPrefix).normalize()
+ .startsWith(java.nio.file.Paths.get(imageServerPath).normalize()))
{
+ logError('Path traversal attempt detected in config item image upload')
+ return error(UtilProperties.getMessage('SecurityUiLabels',
'SupportedImageFormats', locale))
+ }
defaultFileName = filenameToUse + '_temp'
uploadObject = new HttpRequestFileUpload()
uploadObject.setOverrideFilename(defaultFileName)
diff --git
a/applications/product/src/main/groovy/org/apache/ofbiz/product/catalog/imagemanagement/ImageUpload.groovy
b/applications/product/src/main/groovy/org/apache/ofbiz/product/catalog/imagemanagement/ImageUpload.groovy
index dcb2b14257..d15816597d 100644
---
a/applications/product/src/main/groovy/org/apache/ofbiz/product/catalog/imagemanagement/ImageUpload.groovy
+++
b/applications/product/src/main/groovy/org/apache/ofbiz/product/catalog/imagemanagement/ImageUpload.groovy
@@ -87,6 +87,12 @@ if (fileType) {
contentType = '--' + contentType
}
+ // Guard against path traversal: the resolved save directory must remain
inside imageServerPath
+ if (!java.nio.file.Paths.get(imageServerPath + '/' +
filePathPrefix).normalize()
+ .startsWith(java.nio.file.Paths.get(imageServerPath).normalize()))
{
+ logError('Path traversal attempt detected in product image upload')
+ return error(UtilProperties.getMessage('SecurityUiLabels',
'SupportedImageFormats', locale))
+ }
defaultFileName = filenameToUse + '_temp'
uploadObject = new HttpRequestFileUpload()
uploadObject.setOverrideFilename(defaultFileName)
diff --git
a/applications/product/src/main/groovy/org/apache/ofbiz/product/catalog/imagemanagement/SetDefaultImage.groovy
b/applications/product/src/main/groovy/org/apache/ofbiz/product/catalog/imagemanagement/SetDefaultImage.groovy
index a67abc2e30..5cacb00cdc 100644
---
a/applications/product/src/main/groovy/org/apache/ofbiz/product/catalog/imagemanagement/SetDefaultImage.groovy
+++
b/applications/product/src/main/groovy/org/apache/ofbiz/product/catalog/imagemanagement/SetDefaultImage.groovy
@@ -106,6 +106,18 @@ if (fileType) {
contentType = '--' + contentType
}
+ // Guard against path traversal: the resolved save directory must remain
inside imageServerPath
+ if (!java.nio.file.Paths.get(imageServerPath + '/' +
filePathPrefix).normalize()
+ .startsWith(java.nio.file.Paths.get(imageServerPath).normalize()))
{
+ logError("Path traversal attempt detected in default image upload,
productId: ${productId}")
+ return error('Invalid image path')
+ }
+ // Guard against path traversal: the management path must remain inside
imageManagementPath
+ if (!java.nio.file.Paths.get(imageManagementPath + '/' +
productId).normalize()
+
.startsWith(java.nio.file.Paths.get(imageManagementPath).normalize())) {
+ logError("Path traversal attempt detected in image management path,
productId: ${productId}")
+ return error('Invalid image path')
+ }
defaultFileName = 'temp_' + dataResourceName
checkPathFile = imageManagementPath + '/' + productId + '/' +
dataResourceName
BufferedImage bufImg
diff --git
a/applications/product/src/main/groovy/org/apache/ofbiz/product/catalog/product/EditProductContent.groovy
b/applications/product/src/main/groovy/org/apache/ofbiz/product/catalog/product/EditProductContent.groovy
index 7832b55a2f..007b7c159b 100644
---
a/applications/product/src/main/groovy/org/apache/ofbiz/product/catalog/product/EditProductContent.groovy
+++
b/applications/product/src/main/groovy/org/apache/ofbiz/product/catalog/product/EditProductContent.groovy
@@ -88,6 +88,12 @@ if (fileType) {
contentType = '--' + contentType
}
+ // Guard against path traversal: the resolved save directory must remain
inside imageServerPath
+ if (!java.nio.file.Paths.get(imageServerPath + '/' +
filePathPrefix).normalize()
+ .startsWith(java.nio.file.Paths.get(imageServerPath).normalize()))
{
+ logError('Path traversal attempt detected in product image upload')
+ return error(UtilProperties.getMessage('SecurityUiLabels',
'SupportedImageFormats', locale))
+ }
defaultFileName = filenameToUse + '_temp'
uploadObject = new HttpRequestFileUpload()
uploadObject.setOverrideFilename(defaultFileName)
diff --git
a/applications/product/src/main/java/org/apache/ofbiz/product/product/ProductServices.java
b/applications/product/src/main/java/org/apache/ofbiz/product/product/ProductServices.java
index e5b546f071..73f3e04b6e 100644
---
a/applications/product/src/main/java/org/apache/ofbiz/product/product/ProductServices.java
+++
b/applications/product/src/main/java/org/apache/ofbiz/product/product/ProductServices.java
@@ -21,11 +21,12 @@ package org.apache.ofbiz.product.product;
import java.io.File;
import java.io.FileNotFoundException;
import java.io.IOException;
-import java.io.RandomAccessFile;
import java.math.BigDecimal;
import java.nio.ByteBuffer;
import java.nio.file.Files;
import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.nio.file.StandardCopyOption;
import java.nio.file.StandardOpenOption;
import java.sql.Timestamp;
import java.util.Arrays;
@@ -991,6 +992,7 @@ public class ProductServices {
String imageUrlPrefix =
FlexibleStringExpander.expandString(EntityUtilProperties.getPropertyValue("catalog",
"image.url.prefix", delegator), imageContext);
imageServerPath = imageServerPath.endsWith("/") ?
imageServerPath.substring(0, imageServerPath.length() - 1) : imageServerPath;
+ Path imageServerNormalizedPath =
Paths.get(imageServerPath).normalize();
imageUrlPrefix = imageUrlPrefix.endsWith("/") ?
imageUrlPrefix.substring(0, imageUrlPrefix.length() - 1) : imageUrlPrefix;
FlexibleStringExpander filenameExpander =
FlexibleStringExpander.getInstance(imageFilenameFormat);
String viewNumber =
String.valueOf(productContentTypeId.charAt(productContentTypeId.length() - 1));
@@ -1027,8 +1029,15 @@ public class ProductServices {
/* Write the new image file */
String targetDirectory = imageServerPath + "/" + filePathPrefix;
+ // Guard against path traversal: the resolved directory must
remain inside imageServerPath
+ Path resolvedTargetDir = Paths.get(targetDirectory).normalize();
+ if (!resolvedTargetDir.startsWith(imageServerNormalizedPath)) {
+ Debug.logError("Path traversal attempt detected in product
image upload, productId: " + productId, MODULE);
+ return
ServiceUtil.returnError(UtilProperties.getMessage(RESOURCE,
+ "ProductImageViewUnableWriteFile",
UtilMisc.toMap("fileName", targetDirectory), locale));
+ }
try {
- File targetDir = new File(targetDirectory);
+ File targetDir = resolvedTargetDir.toFile();
// Create the new directory
if (!targetDir.exists()) {
boolean created = targetDir.mkdirs();
@@ -1074,21 +1083,31 @@ public class ProductServices {
}
// Write
try {
- String fileToCheck = imageServerPath + "/" + fileLocation +
"." + extension.getString("fileExtensionId");
- File file = new File(fileToCheck);
+ String fileExtId = extension.getString("fileExtensionId");
+ // Guard against path traversal in the resolved file path
+ Path resolvedFilePath = Paths.get(imageServerPath,
fileLocation + "." + fileExtId).normalize();
+ if (!resolvedFilePath.startsWith(imageServerNormalizedPath)) {
+ Debug.logError("Path traversal attempt detected in product
image file path, productId: " + productId, MODULE);
+ return
ServiceUtil.returnError(UtilProperties.getMessage(RESOURCE,
+ "ProductImageViewUnableWriteFile",
UtilMisc.toMap("fileName", resolvedFilePath.toString()), locale));
+ }
+ File file = resolvedFilePath.toFile();
try {
- Path tempFile = Files.createTempFile(null, null);
+ // Use the intended extension so that isValidFile's
allow-list check applies correctly
+ // (e.g. rejects htm/html before content inspection) and
so that the imageMadeSafe()
+ // call inside isValidFile re-encodes and sanitizes the
image in-place in the temp file.
+ Path tempFile = Files.createTempFile(null, "." +
fileExtId);
Files.write(tempFile, imageData.array(),
StandardOpenOption.APPEND);
// Check if a webshell is not uploaded
if
(!org.apache.ofbiz.security.SecuredUpload.isValidFile(tempFile.toString(),
"Image", delegator)) {
String errorMessage =
UtilProperties.getMessage("SecurityUiLabels", "SupportedImageFormats", locale);
+ new File(tempFile.toString()).deleteOnExit();
return ServiceUtil.returnError(errorMessage);
}
- File tempFileToDelete = new File(tempFile.toString());
- tempFileToDelete.deleteOnExit();
- RandomAccessFile out = new RandomAccessFile(fileToCheck,
"rw");
- out.write(imageData.array());
- out.close();
+ // Copy from the sanitized temp file, not from the
original byte array, so that
+ // metadata and payloads stripped by imageMadeSafe() are
not reintroduced.
+ Files.copy(tempFile, resolvedFilePath,
StandardCopyOption.REPLACE_EXISTING);
+ new File(tempFile.toString()).deleteOnExit();
} catch (FileNotFoundException e) {
Debug.logError(e, MODULE);
return
ServiceUtil.returnError(UtilProperties.getMessage(RESOURCE,
@@ -1348,6 +1367,7 @@ public class ProductServices {
String imageUrlPrefix =
FlexibleStringExpander.expandString(EntityUtilProperties.getPropertyValue("catalog",
"image.url.prefix", delegator), imageContext);
imageServerPath = imageServerPath.endsWith("/") ?
imageServerPath.substring(0, imageServerPath.length() - 1) : imageServerPath;
+ Path imageServerNormalizedPath =
Paths.get(imageServerPath).normalize();
imageUrlPrefix = imageUrlPrefix.endsWith("/") ?
imageUrlPrefix.substring(0, imageUrlPrefix.length() - 1) : imageUrlPrefix;
FlexibleStringExpander filenameExpander =
FlexibleStringExpander.getInstance(imageFilenameFormat);
String id = productPromoId + "_Image_" +
productPromoContentTypeId.charAt(productPromoContentTypeId.length() - 1);
@@ -1375,29 +1395,45 @@ public class ProductServices {
filenameToUse += "." + extension.getString("fileExtensionId");
}
- File makeResourceDirectory = new File(imageServerPath + "/" +
filePathPrefix);
+ // Guard against path traversal: the resolved directory must
remain inside imageServerPath
+ Path resolvedResourceDir = Paths.get(imageServerPath,
filePathPrefix).normalize();
+ if (!resolvedResourceDir.startsWith(imageServerNormalizedPath)) {
+ Debug.logError("Path traversal attempt detected in product
promo image upload, productPromoId: " + productPromoId, MODULE);
+ return
ServiceUtil.returnError(UtilProperties.getMessage(RESOURCE,
+ "ProductImageViewUnableWriteFile",
UtilMisc.toMap("fileName", imageServerPath + "/" + filePathPrefix), locale));
+ }
+ File makeResourceDirectory = resolvedResourceDir.toFile();
if (!makeResourceDirectory.exists()) {
if (!makeResourceDirectory.mkdirs()) {
Debug.logError("Directory :" +
makeResourceDirectory.getPath() + ", couldn't be created", MODULE);
}
}
- String fileToCheck = imageServerPath + "/" + filePathPrefix +
filenameToUse;
- File file = new File(fileToCheck);
+ // Guard against path traversal in the resolved file path
+ Path resolvedFilePath = Paths.get(imageServerPath, filePathPrefix
+ filenameToUse).normalize();
+ if (!resolvedFilePath.startsWith(imageServerNormalizedPath)) {
+ Debug.logError("Path traversal attempt detected in product
promo image file path, productPromoId: " + productPromoId, MODULE);
+ return
ServiceUtil.returnError(UtilProperties.getMessage(RESOURCE,
+ "ProductImageViewUnableWriteFile",
UtilMisc.toMap("fileName", resolvedFilePath.toString()), locale));
+ }
+ File file = resolvedFilePath.toFile();
+ String fileExtId = extension != null ?
extension.getString("fileExtensionId") : "";
try {
- Path tempFile = Files.createTempFile(null, null);
+ // Use the intended extension so that isValidFile's allow-list
check applies correctly
+ // and imageMadeSafe() sanitizes the image in-place in the
temp file.
+ Path tempFile = Files.createTempFile(null, fileExtId.isEmpty()
? null : "." + fileExtId);
Files.write(tempFile, imageData.array(),
StandardOpenOption.APPEND);
// Check if a webshell is not uploaded
if
(!org.apache.ofbiz.security.SecuredUpload.isValidFile(tempFile.toString(),
"Image", delegator)) {
String errorMessage =
UtilProperties.getMessage("SecurityUiLabels", "SupportedImageFormats", locale);
+ new File(tempFile.toString()).deleteOnExit();
return ServiceUtil.returnError(errorMessage);
}
- File tempFileToDelete = new File(tempFile.toString());
- tempFileToDelete.deleteOnExit();
- RandomAccessFile out = new RandomAccessFile(file, "rw");
- out.write(imageData.array());
- out.close();
+ // Copy from the sanitized temp file, not from the original
byte array, so that
+ // metadata and payloads stripped by imageMadeSafe() are not
reintroduced.
+ Files.copy(tempFile, resolvedFilePath,
StandardCopyOption.REPLACE_EXISTING);
+ new File(tempFile.toString()).deleteOnExit();
} catch (FileNotFoundException e) {
Debug.logError(e, MODULE);
return
ServiceUtil.returnError(UtilProperties.getMessage(RESOURCE,
diff --git
a/framework/base/src/main/java/org/apache/ofbiz/base/util/HttpRequestFileUpload.java
b/framework/base/src/main/java/org/apache/ofbiz/base/util/HttpRequestFileUpload.java
index 7e7a079357..0c72894e06 100644
---
a/framework/base/src/main/java/org/apache/ofbiz/base/util/HttpRequestFileUpload.java
+++
b/framework/base/src/main/java/org/apache/ofbiz/base/util/HttpRequestFileUpload.java
@@ -270,10 +270,27 @@ public class HttpRequestFileUpload {
}
fos.flush();
+ // If the saved override filename carries no extension
but the original client filename does,
+ // temporarily rename the file to add that extension
so that isAllowedExtension inside
+ // isValidFile can compare it against the per-type
allow-list.
+ String fileToValidate = fileTocheck;
+ if (overrideFilename != null && filename != null
+ && filename.lastIndexOf('.') > 0
+ && filenameToUse.lastIndexOf('.') < 0) {
+ String originalExt =
filename.substring(filename.lastIndexOf('.')).toLowerCase(java.util.Locale.ROOT);
+ new File(fileTocheck).renameTo(new
File(fileTocheck + originalExt));
+ fileToValidate = fileTocheck + originalExt;
+ }
// Check if a webshell is not uploaded
- if
(!org.apache.ofbiz.security.SecuredUpload.isValidFile(fileTocheck, fileType,
delegator)) {
+ if
(!org.apache.ofbiz.security.SecuredUpload.isValidFile(fileToValidate, fileType,
delegator)) {
+ if (!fileToValidate.equals(fileTocheck)) {
+ new File(fileToValidate).renameTo(new
File(fileTocheck));
+ }
return false;
}
+ if (!fileToValidate.equals(fileTocheck)) {
+ new File(fileToValidate).renameTo(new
File(fileTocheck));
+ }
} catch (ImageReadException e) {
Debug.logError(e, MODULE);
return false;
diff --git a/framework/security/config/security.properties
b/framework/security/config/security.properties
index 10d2e5562d..8d5b7c7bc9 100644
--- a/framework/security/config/security.properties
+++ b/framework/security/config/security.properties
@@ -285,7 +285,20 @@ templateClassResolver=
#-- For text files, the philosophy is we can't presume of all possible text
contents used for attacks with payloads
#-- At least there is an easy way to prevent them in
SecuredUpload::isValidTextFile
#--
-#-- List of denied files suffixes to be uploaded
+#-- Extension ALLOW-LIST per file type (primary gate, checked before the
deny-list below).
+#-- Only extensions listed here are accepted for each file type.
+#-- Comma-separated, case-insensitive. Modify with care: every entry is a
potential attack surface.
+allowedExtensionsImage=jpg,jpeg,png,gif,tiff,tif
+allowedExtensionsImageAndSvg=jpg,jpeg,png,gif,tiff,tif,svg
+allowedExtensionsPdf=pdf
+allowedExtensionsAudio=mp3,wav,ogg,flac,mp4,m4a,aac
+allowedExtensionsVideo=mp4,mov,avi,mkv,webm,wmv,mpg,mpeg,3gp,flv
+allowedExtensionsText=txt
+allowedExtensionsCsv=csv
+allowedExtensionsCompressed=zip
+
+#-- List of denied files suffixes to be uploaded (secondary defence; kept for
direct callers
+#-- of isValidFileName that operate without a file-type context, e.g. service
validators).
#-- OFBiz of course also check contents...
deniedFileExtensions=html,htm,php,php1,php2,hph3,php4,php5,php6,php7,phps,asp,aspx,asa,asax,ascx,ashx,asmx,jsp,jspa,jspx,jsw,jsv,jspf,jtml,cfm,cfc,bat,exe,com,dll,\
vbs,js,reg,cgi,asis,sh,phtm,pht,phtml,shtm,inc,asp,cdx,asa,cer,py,pl,shtml,hta,ps1,tag,pgif,htaccess,phar,inc,cgi,wss,do,action
diff --git
a/framework/security/src/main/java/org/apache/ofbiz/security/SecuredUpload.java
b/framework/security/src/main/java/org/apache/ofbiz/security/SecuredUpload.java
index c0c5759d8a..5f46c3cedf 100644
---
a/framework/security/src/main/java/org/apache/ofbiz/security/SecuredUpload.java
+++
b/framework/security/src/main/java/org/apache/ofbiz/security/SecuredUpload.java
@@ -46,16 +46,23 @@ import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.Iterator;
+import java.util.LinkedHashMap;
+import java.util.LinkedHashSet;
import java.util.List;
+import java.util.Locale;
+import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.UUID;
+import java.util.regex.Pattern;
import java.util.zip.DataFormatException;
import java.util.zip.Inflater;
import javax.imageio.ImageIO;
import javax.imageio.ImageReader;
import javax.imageio.stream.ImageInputStream;
+import javax.xml.parsers.DocumentBuilder;
+import javax.xml.parsers.DocumentBuilderFactory;
import javax.xml.parsers.ParserConfigurationException;
import org.apache.batik.anim.dom.SAXSVGDocumentFactory;
@@ -102,7 +109,11 @@ import org.apache.tika.sax.BasicContentHandlerFactory;
import org.apache.tika.sax.ContentHandlerFactory;
import org.apache.tika.sax.RecursiveParserWrapperHandler;
import org.mustangproject.ZUGFeRD.ZUGFeRDImporter;
+import org.w3c.dom.Attr;
import org.w3c.dom.Document;
+import org.w3c.dom.NamedNodeMap;
+import org.w3c.dom.Node;
+import org.w3c.dom.NodeList;
import org.xml.sax.SAXException;
import com.drew.imaging.ImageMetadataReader;
@@ -128,12 +139,148 @@ public class SecuredUpload {
private static final Boolean ALLOWSTRINGCONCATENATIONINUPLOADEDFILES =
UtilProperties.getPropertyAsBoolean("security",
"allowStringConcatenationInUploadedFiles", false);
+ /**
+ * Per-type extension allow-lists loaded from {@code security.properties}.
+ * Keys match the {@code fileType} argument of {@link #isValidFile}.
+ * "AllButCompressed" and "All" are derived automatically as unions of the
others.
+ */
+ private static final Map<String, Set<String>> ALLOWED_EXTENSIONS_BY_TYPE =
buildAllowedExtensionsMap();
+
+ /**
+ * SVG element names (lower-cased) whose presence in an SVG file is
unconditionally rejected.
+ * These elements either execute script, embed foreign content, or enable
timing-based attacks.
+ */
+ private static final Set<String> DENIED_SVG_ELEMENTS =
Collections.unmodifiableSet(new HashSet<>(Arrays.asList(
+ "script", "foreignobject", "animate", "animatecolor",
+ "animatemotion", "animatetransform", "set", "handler",
"listener")));
+
+ /**
+ * Allow-list pattern for entity primary-key values (e.g. {@code
contentId}).
+ * Accepts ASCII letters, digits, underscores, hyphens and colons — the
character
+ * set used by all OFBiz auto-generated and user-supplied IDs.
+ */
+ private static final Pattern ENTITY_KEY_PATTERN =
Pattern.compile("[a-zA-Z0-9_:\\-]{1,255}");
+
+ /**
+ * Allow-list pattern for webapp-relative path parameters (e.g. {@code
webappPath}).
+ * Accepts ASCII letters, digits and the path punctuation {@code / . - _}.
+ */
+ private static final Pattern WEBAPP_PATH_PATTERN =
Pattern.compile("[a-zA-Z0-9/_\\.\\-]{1,4096}");
+
// "(" and ")" for duplicates files
private static final String FILENAMEVALIDCHARACTERS_DUPLICATES =
UtilProperties.getPropertyValue("security",
"fileNameValidCharactersDuplicates", "[a-zA-Z0-9-_ ()]");
private static final String FILENAMEVALIDCHARACTERS =
UtilProperties.getPropertyValue("security",
"fileNameValidCharacters", "[a-zA-Z0-9-_ ]");
+ // -----------------------------------------------------------------------
+ // Allow-list validators (preferred over the deny-list in isValidText)
+ // -----------------------------------------------------------------------
+
+ /**
+ * Allow-list validator for entity primary-key values such as {@code
contentId}.
+ * <p>Only ASCII letters, digits, underscores, hyphens and colons are
accepted —
+ * the character set used by OFBiz auto-generated and user-supplied IDs.
+ * <p>This is an <em>allow-list</em> check and does not rely on a
deny-list, so it
+ * cannot be bypassed by encoding tricks or token splitting.
+ *
+ * @param key the string to validate (must not be {@code null} or empty)
+ * @return {@code true} when {@code key} conforms to the expected format
+ */
+ public static boolean isValidEntityKey(String key) {
+ if (UtilValidate.isEmpty(key)) {
+ Debug.logError("Entity key is null or empty", MODULE);
+ return false;
+ }
+ if (!ENTITY_KEY_PATTERN.matcher(key).matches()) {
+ Debug.logError("Entity key contains disallowed characters: " +
key, MODULE);
+ return false;
+ }
+ return true;
+ }
+
+ /**
+ * Allow-list validator for webapp-relative path parameters such as {@code
webappPath}.
+ * <p>Accepts only ASCII letters, digits and the path punctuation {@code /
. - _}.
+ * Path-traversal sequences ({@code ..}) and null bytes are always
rejected regardless
+ * of encoding.
+ * <p>This is an <em>allow-list</em> check and does not rely on a
deny-list.
+ *
+ * @param path the string to validate
+ * @return {@code true} when {@code path} is safe to use as a webapp path
+ */
+ public static boolean isValidWebAppPath(String path) {
+ if (UtilValidate.isEmpty(path)) {
+ Debug.logError("Webapp path is null or empty", MODULE);
+ return false;
+ }
+ if (path.indexOf('\0') >= 0) {
+ Debug.logError("Webapp path contains a null byte", MODULE);
+ return false;
+ }
+ if (path.contains("..")) {
+ Debug.logError("Path traversal sequence '..' is not allowed in
webapp path: " + path, MODULE);
+ return false;
+ }
+ if (!WEBAPP_PATH_PATTERN.matcher(path).matches()) {
+ Debug.logError("Webapp path contains disallowed characters: " +
path, MODULE);
+ return false;
+ }
+ return true;
+ }
+
+ /**
+ * Allow-list validator for text content (uploaded files, string
parameters).
+ * <p>Accepts all printable Unicode code points and the four universally
legitimate
+ * whitespace characters (U+0009 HT, U+000A LF, U+000D CR, U+0020 SPACE).
+ * All other C0 control characters (U+0000–U+001F) and C1 control
characters
+ * (U+007F–U+009F) are rejected.
+ * <p>This approach is significantly more robust than the token deny-list
in
+ * {@link #isValidText} because it operates on decoded Unicode code points.
+ * It cannot be bypassed by token splitting, alternate encodings, or
obfuscation
+ * with whitespace. Its intent is to guarantee that stored text can safely
+ * round-trip through any character-level processing without unexpected
+ * control-character effects, <em>not</em> to detect every conceivable
injection
+ * payload — that responsibility belongs to the surrounding context (output
+ * encoding, CSP, FreeMarker SAFER_RESOLVER, etc.).
+ *
+ * @param content the text content to validate; {@code null} is rejected
+ * @return {@code true} when all code points are within the allowed set
+ */
+ public static boolean isValidTextContent(String content) {
+ if (content == null) {
+ return false;
+ }
+ for (int i = 0; i < content.length();) {
+ int cp = content.codePointAt(i);
+ i += Character.charCount(cp);
+ // Allow standard whitespace: HT, LF, CR
+ if (cp == 0x09 || cp == 0x0A || cp == 0x0D) {
+ continue;
+ }
+ // Reject all other C0 control characters, including the null byte
(U+0000)
+ if (cp < 0x20) {
+ Debug.logInfo("Text content rejected: contains C0 control
character U+"
+ + String.format("%04X", cp), MODULE);
+ return false;
+ }
+ // Reject DEL (U+007F) and C1 control characters (U+0080–U+009F)
+ if (cp >= 0x7F && cp <= 0x9F) {
+ Debug.logInfo("Text content rejected: contains C1 control
character U+"
+ + String.format("%04X", cp), MODULE);
+ return false;
+ }
+ // Lone surrogate code points should not appear in valid Java
strings, but
+ // guard against them explicitly to prevent bypass via malformed
UTF-16.
+ if (cp >= 0xD800 && cp <= 0xDFFF) {
+ Debug.logInfo("Text content rejected: contains surrogate code
point", MODULE);
+ return false;
+ }
+ // All other printable Unicode code points are accepted.
+ }
+ return true;
+ }
+
// Cover method of the same name below. Historically used with 84
references when below was created
// check there is no web shell in the uploaded file
// A file containing a reverse shell will be rejected.
@@ -273,6 +420,14 @@ public class SecuredUpload {
return true;
}
+ // Primary gate: file extension must be in the allow-list for the
declared file type.
+ // This is the strongest control — only explicitly whitelisted
extensions pass.
+ // The deny-list in isValidFileName acts as a secondary defence.
+ if (!isAllowedExtension(fileToCheck, fileType)) {
+ deleteBadFile(fileToCheck);
+ return false;
+ }
+
// Check the file name
if (!isValidFileName(fileToCheck, delegator)) { // Useless when the
file is internally generated, but not sure for all cases
return false;
@@ -395,12 +550,18 @@ public class SecuredUpload {
Path filePath = Paths.get(fileName);
byte[] bytesFromFile = Files.readAllBytes(filePath);
ImageFormat imageFormat = Imaging.guessFormat(bytesFromFile);
- return (imageFormat.equals(ImageFormats.PNG)
+ boolean knownRasterFormat = imageFormat.equals(ImageFormats.PNG)
|| imageFormat.equals(ImageFormats.GIF)
|| imageFormat.equals(ImageFormats.TIFF)
- || imageFormat.equals(ImageFormats.JPEG))
- && imageMadeSafe(fileName)
- && isValidTextFile(fileName, false);
+ || imageFormat.equals(ImageFormats.JPEG);
+ if (!knownRasterFormat) {
+ return false;
+ }
+ // imageMadeSafe() re-encodes the pixel data and overwrites the file
on disk,
+ // stripping all metadata and steganographic payloads. Running
isValidTextFile
+ // on the resulting binary pixel data would be both redundant and
unreliable,
+ // so it is intentionally omitted here.
+ return imageMadeSafe(fileName);
}
/**
@@ -645,7 +806,11 @@ public class SecuredUpload {
} catch (IOException e) {
return false;
}
- return isValidTextFile(fileName, true); // Validate content to
prevent webshell
+ // DOM-level inspection: block dangerous elements (<script>,
<foreignObject>, …)
+ // and unsafe attribute values (on* event handlers,
javascript:/data: URIs).
+ // This is stronger than the text-token deny-list which can be
bypassed by
+ // splitting tokens or using alternate serialisations.
+ return isSafeSvgContent(fileName);
}
return false;
}
@@ -660,9 +825,10 @@ public class SecuredUpload {
if (Objects.isNull(file) || !file.exists()) {
return false;
}
- // Load stream in PDF parser
- new PdfReader(file.getAbsolutePath()); // Just a check
- return true;
+ // Load stream in PDF parser — just probe whether the file is a
valid PDF
+ try (PdfReader ignored = new PdfReader(file.getAbsolutePath())) {
+ return true;
+ }
} catch (Exception e) {
// If it's not a PDF then exception will be thrown and return will
be false
return false;
@@ -685,9 +851,11 @@ public class SecuredUpload {
return safeState;
}
// Load stream in PDF parser
- PdfReader reader = new PdfReader(file.getAbsolutePath());
- // Check 1: detect if the document contains any JavaScript code
- String jsCode = reader.getJavaScript();
+ String jsCode;
+ try (PdfReader reader = new PdfReader(file.getAbsolutePath())) {
+ // Check 1: detect if the document contains any JavaScript code
+ jsCode = reader.getJavaScript();
+ }
if (!Objects.isNull(jsCode)) {
return safeState;
}
@@ -971,11 +1139,23 @@ public class SecuredUpload {
}
/**
- * Does this text file contains a Freemarker Server-Side Template
Injection (SSTI) using freemarker.template.utility.Execute? Etc.
- * @param fileName must be an UTF-8 encoded text file
- * @param encodedContent true id the file content is encoded
- * @return true if the text file does not contains a Freemarker SSTI or
other issues
- * @throws IOException
+ * Validates that the file at {@code fileName} is safe plain text.
+ * <p>Three checks are applied in order:
+ * <ol>
+ * <li><strong>UTF-8 encoding</strong> (when {@code encodedContent} is
{@code true}) — rejects
+ * binary files masquerading as text.</li>
+ * <li><strong>Dangerous XML constructs</strong> — rejects {@code
xlink:href="http"} (external
+ * resource loading) and {@code <!ENTITY} (Billion-Laughs /
XXE).</li>
+ * <li><strong>Character-class allow-list</strong> ({@link
#isValidTextContent}) — accepts only
+ * printable Unicode code points and standard whitespace. This is
an <em>allow-list</em>
+ * strategy and cannot be bypassed by token splitting, alternate
encodings, or obfuscation
+ * with whitespace, unlike the token deny-list previously used
here.</li>
+ * </ol>
+ *
+ * @param fileName path to the file to validate
+ * @param encodedContent {@code true} to additionally enforce UTF-8
validity
+ * @return {@code true} when the file passes all checks
+ * @throws IOException if the file cannot be read
*/
private static boolean isValidTextFile(String fileName, Boolean
encodedContent) throws IOException {
Path filePath = Paths.get(fileName);
@@ -993,8 +1173,11 @@ public class SecuredUpload {
Debug.logInfo("Linked images inside or Entity in SVG are not
allowed for security reason", MODULE);
return false;
}
- ArrayList<String> allowed = new ArrayList<>();
- return isValidText(content, allowed);
+ // Use the character-class allow-list instead of the token deny-list.
+ // The deny-list can be bypassed by token splitting, alternate
encodings and
+ // obfuscation; isValidTextContent operates at the decoded code-point
level
+ // and is immune to those bypass techniques.
+ return isValidTextContent(content);
}
// Check there is no web shell
@@ -1033,6 +1216,187 @@ public class SecuredUpload {
}
}
+ // -----------------------------------------------------------------------
+ // Extension allow-list helpers
+ // -----------------------------------------------------------------------
+
+ /**
+ * Builds the per-file-type extension allow-list map from {@code
security.properties}.
+ * Each key matches a {@code fileType} value accepted by {@link
#isValidFile}.
+ * "AllButCompressed" and "All" are derived as unions of the individual
sets.
+ */
+ private static Map<String, Set<String>> buildAllowedExtensionsMap() {
+ Set<String> image = loadAllowedExtensions("allowedExtensionsImage",
+ "jpg,jpeg,png,gif,tiff,tif");
+ Set<String> imageAndSvg =
loadAllowedExtensions("allowedExtensionsImageAndSvg",
+ "jpg,jpeg,png,gif,tiff,tif,svg");
+ Set<String> pdf = loadAllowedExtensions("allowedExtensionsPdf",
+ "pdf");
+ Set<String> audio = loadAllowedExtensions("allowedExtensionsAudio",
+ "mp3,wav,ogg,flac,mp4,m4a,aac");
+ Set<String> video = loadAllowedExtensions("allowedExtensionsVideo",
+ "mp4,mov,avi,mkv,webm,wmv,mpg,mpeg,3gp,flv");
+ Set<String> text = loadAllowedExtensions("allowedExtensionsText",
+ "txt");
+ Set<String> csv = loadAllowedExtensions("allowedExtensionsCsv",
+ "csv");
+ Set<String> compressed =
loadAllowedExtensions("allowedExtensionsCompressed",
+ "zip");
+
+ Set<String> allButCompressed = new LinkedHashSet<>();
+ allButCompressed.addAll(image);
+ allButCompressed.addAll(imageAndSvg);
+ allButCompressed.addAll(pdf);
+ allButCompressed.addAll(audio);
+ allButCompressed.addAll(video);
+ allButCompressed.addAll(text);
+ allButCompressed.addAll(csv);
+
+ Set<String> all = new LinkedHashSet<>(allButCompressed);
+ all.addAll(compressed);
+
+ Map<String, Set<String>> map = new LinkedHashMap<>();
+ map.put("Image", image);
+ map.put("ImageAndSvg", imageAndSvg);
+ map.put("PDF", pdf);
+ map.put("Audio", audio);
+ map.put("Video", video);
+ map.put("Text", text);
+ map.put("CSV", csv);
+ map.put("Compressed", compressed);
+ map.put("AllButCompressed",
Collections.unmodifiableSet(allButCompressed));
+ map.put("All", Collections.unmodifiableSet(all));
+ return Collections.unmodifiableMap(map);
+ }
+
+ /**
+ * Loads a comma-separated list of lowercase file extensions from
+ * {@code security.properties} under the given key, falling back to
+ * {@code defaultValue} when the property is absent or blank.
+ */
+ private static Set<String> loadAllowedExtensions(String propertyKey,
String defaultValue) {
+ String value = UtilProperties.getPropertyValue("security",
propertyKey, defaultValue);
+ Set<String> exts = new LinkedHashSet<>();
+ for (String ext : value.split(",")) {
+ String trimmed = ext.trim().toLowerCase(Locale.ROOT);
+ if (!trimmed.isEmpty()) {
+ exts.add(trimmed);
+ }
+ }
+ return Collections.unmodifiableSet(exts);
+ }
+
+ /**
+ * Returns {@code true} if the extension of {@code fileToCheck} is in the
+ * allow-list for the given {@code fileType}. Falls back to the "All" set
+ * for unknown file-type strings so that callers are never silently blocked
+ * by a misconfigured type name.
+ */
+ private static boolean isAllowedExtension(String fileToCheck, String
fileType) {
+ Set<String> allowed = ALLOWED_EXTENSIONS_BY_TYPE.getOrDefault(
+ fileType, ALLOWED_EXTENSIONS_BY_TYPE.get("All"));
+ String extension =
FilenameUtils.getExtension(fileToCheck).toLowerCase(Locale.ROOT);
+ if (allowed.contains(extension)) {
+ return true;
+ }
+ Debug.logError("File extension [." + extension + "] is not in the
allow-list for"
+ + " file type [" + fileType + "]. To permit it, add it to the"
+ + " allowedExtensions" + fileType + " property in
security.properties.", MODULE);
+ return false;
+ }
+
+ // -----------------------------------------------------------------------
+ // SVG DOM safety check
+ // -----------------------------------------------------------------------
+
+ /**
+ * Parses {@code fileName} as XML and walks the DOM tree looking for
elements
+ * or attributes that can execute code or load external resources.
+ *
+ * <p>This replaces the text-token deny-list ({@code isValidTextFile}) for
SVG
+ * files. Token scanning can be bypassed by splitting keywords across
lines,
+ * using alternate encodings, or exploiting SVG/XML serialisation
ambiguities.
+ * DOM-level inspection operates on the fully-parsed structure and is
therefore
+ * immune to those bypass techniques.
+ *
+ * <p>The parser is hardened against XXE and DOCTYPE-injection before use.
+ *
+ * @return {@code true} if no unsafe element or attribute was found
+ */
+ private static boolean isSafeSvgContent(String fileName) {
+ try {
+ DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
+ // Harden against XXE and DOCTYPE-based injection attacks
+
dbf.setFeature("http://apache.org/xml/features/disallow-doctype-decl";, true);
+
dbf.setFeature("http://xml.org/sax/features/external-general-entities";, false);
+
dbf.setFeature("http://xml.org/sax/features/external-parameter-entities";,
false);
+ dbf.setExpandEntityReferences(false);
+ dbf.setNamespaceAware(true);
+ DocumentBuilder db = dbf.newDocumentBuilder();
+ Document doc = db.parse(new File(fileName));
+ return checkSvgNode(doc.getDocumentElement());
+ } catch (Exception e) {
+ Debug.logWarning(e, "SVG content safety check failed for: " +
fileName, MODULE);
+ return false;
+ }
+ }
+
+ /**
+ * Recursively walks an SVG DOM tree and returns {@code false} on the first
+ * unsafe element or attribute found.
+ *
+ * <p>Rejected elements: anything in {@link #DENIED_SVG_ELEMENTS} (e.g.
+ * {@code <script>}, {@code <foreignObject>}, animation elements).
+ * Rejected attributes:
+ * <ul>
+ * <li>{@code on*} event-handler attributes (e.g. {@code onclick},
{@code onload})</li>
+ * <li>{@code href}, {@code xlink:href}, {@code src}, {@code action}
pointing to a
+ * {@code javascript:}, {@code data:}, or {@code vbscript:} URI
scheme</li>
+ * </ul>
+ */
+ private static boolean checkSvgNode(Node node) {
+ if (node.getNodeType() == Node.ELEMENT_NODE) {
+ String localName = node.getLocalName() != null ?
node.getLocalName() : node.getNodeName();
+ if
(DENIED_SVG_ELEMENTS.contains(localName.toLowerCase(Locale.ROOT))) {
+ Debug.logInfo("SVG rejected: contains denied element <" +
localName + ">", MODULE);
+ return false;
+ }
+ NamedNodeMap attrs = node.getAttributes();
+ if (attrs != null) {
+ for (int i = 0; i < attrs.getLength(); i++) {
+ Attr attr = (Attr) attrs.item(i);
+ String attrName = attr.getName().toLowerCase(Locale.ROOT);
+ String attrValue = attr.getValue() == null ? ""
+ : attr.getValue().trim().toLowerCase(Locale.ROOT);
+ // Block on* event-handler attributes
+ if (attrName.startsWith("on")) {
+ Debug.logInfo("SVG rejected: event-handler attribute ["
+ + attr.getName() + "]", MODULE);
+ return false;
+ }
+ // Block unsafe URI schemes in link/source attributes
+ if (attrName.equals("href") ||
attrName.equals("xlink:href")
+ || attrName.equals("src") ||
attrName.equals("action")) {
+ if (attrValue.startsWith("javascript:")
+ || attrValue.startsWith("data:")
+ || attrValue.startsWith("vbscript:")) {
+ Debug.logInfo("SVG rejected: unsafe URI scheme in
attribute ["
+ + attr.getName() + "]", MODULE);
+ return false;
+ }
+ }
+ }
+ }
+ }
+ NodeList children = node.getChildNodes();
+ for (int i = 0; i < children.getLength(); i++) {
+ if (!checkSvgNode(children.item(i))) {
+ return false;
+ }
+ }
+ return true;
+ }
+
private static List<String> getDeniedFileExtensions() {
String deniedExtensions = UtilProperties.getPropertyValue("security",
"deniedFileExtensions");
return UtilValidate.isNotEmpty(deniedExtensions) ?
StringUtil.split(deniedExtensions, ",") : new ArrayList<>();
diff --git
a/framework/service/src/main/java/org/apache/ofbiz/service/engine/EntityAutoEngine.java
b/framework/service/src/main/java/org/apache/ofbiz/service/engine/EntityAutoEngine.java
index 1445a49a4e..e95261b982 100644
---
a/framework/service/src/main/java/org/apache/ofbiz/service/engine/EntityAutoEngine.java
+++
b/framework/service/src/main/java/org/apache/ofbiz/service/engine/EntityAutoEngine.java
@@ -625,6 +625,13 @@ public final class EntityAutoEngine extends
GenericAsyncEngine {
// TODO maybe more parameters will be needed in future...
String parameter = (String) parameters.get("webappPath");
if (parameter != null) {
+ // Use an allow-list path validator instead of the token deny-list:
+ // only ASCII letters, digits and / . - _ are accepted, and
path-traversal
+ // sequences (..) and null bytes are always rejected regardless of
encoding.
+ if (!SecuredUpload.isValidWebAppPath(parameter)) {
+ Debug.logError("================== Not saved for security
reason ==================", MODULE);
+ return false;
+ }
try {
if (!SecuredUpload.isValidText(parameter,
Collections.emptyList())) {
Debug.logError("================== Not saved for security
reason ==================", MODULE);
