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
commit 6b519ed9961833da7cedd1aadd232e3234aa8ad3 Author: Jacopo Cappellato <[email protected]> AuthorDate: Wed Mar 11 08:50:13 2026 +0100 Improved: Enhance file upload validation with allowlist and path checks Backported from trunk da0febe182 with minor modifications. --- .../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 | 389 ++++++++++++++++++++- .../ofbiz/service/engine/EntityAutoEngine.java | 7 + 11 files changed, 512 insertions(+), 38 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 9fa8c7fc49..4431e66e15 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 5999991364..47c9f596f7 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 48135bd773..3015b86b3b 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 2dc6f73707..ae002db6a3 100644 --- a/framework/security/config/security.properties +++ b/framework/security/config/security.properties @@ -270,7 +270,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 cd660d60b1..96a9af120f 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; @@ -99,7 +106,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; @@ -125,12 +136,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. @@ -259,6 +406,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; @@ -381,12 +536,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); } /** @@ -631,7 +792,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; } @@ -646,8 +811,8 @@ public class SecuredUpload { if (Objects.isNull(file) || !file.exists()) { return false; } - // Load stream in PDF parser - new PdfReader(file.getAbsolutePath()); // Just a check + // Load stream in PDF parser — just probe whether the file is a valid PDF + new PdfReader(file.getAbsolutePath()); return true; } catch (Exception e) { return false; @@ -953,11 +1118,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); @@ -975,8 +1152,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 @@ -1015,6 +1195,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);
