This is an automated email from the ASF dual-hosted git repository. jleroux pushed a commit to branch release22.01 in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
commit 3b29cd3a78c4d4acc46547ac11979e70b11a0c24 Author: Jacques Le Roux <jacques.le.r...@les7arts.com> AuthorDate: Sat Feb 19 11:37:43 2022 +0100 Improved: Secure the uploads (OFBIZ-12080) In security.properties, adds some deniedFileExtensions and improves few comments (easier to merge later) In DataServices::createFileNoPerm filters file upload to verify the filename before the content is checked if necessary Manually merges SecuredUpload.java based on trunk Conflicts merged by hand in SecuredUpload.java --- .../apache/ofbiz/content/data/DataServices.java | 32 ++++++++++++++++------ .../org/apache/ofbiz/security/SecuredUpload.java | 21 +++++++------- 2 files changed, 35 insertions(+), 18 deletions(-) diff --git a/applications/content/src/main/java/org/apache/ofbiz/content/data/DataServices.java b/applications/content/src/main/java/org/apache/ofbiz/content/data/DataServices.java index 5dd75a7..2bd71c1 100644 --- a/applications/content/src/main/java/org/apache/ofbiz/content/data/DataServices.java +++ b/applications/content/src/main/java/org/apache/ofbiz/content/data/DataServices.java @@ -49,6 +49,7 @@ import org.apache.ofbiz.entity.Delegator; import org.apache.ofbiz.entity.GenericEntityException; import org.apache.ofbiz.entity.GenericValue; import org.apache.ofbiz.entity.util.EntityQuery; +import org.apache.ofbiz.security.SecuredUpload; import org.apache.ofbiz.service.DispatchContext; import org.apache.ofbiz.service.GenericServiceException; import org.apache.ofbiz.service.ModelService; @@ -195,7 +196,15 @@ public class DataServices { return createFileMethod(dctx, context); } - public static Map<String, Object> createFileNoPerm(DispatchContext dctx, Map<String, ? extends Object> rcontext) { + public static Map<String, Object> createFileNoPerm(DispatchContext dctx, Map<String, ? extends Object> rcontext) throws IOException, + ImageReadException { + String originalFileName = (String) rcontext.get("dataResourceName"); + Delegator delegator = dctx.getDelegator(); + Locale locale = (Locale) rcontext.get("locale"); + if (!SecuredUpload.isValidFile(originalFileName, "All", delegator)) { + String errorMessage = UtilProperties.getMessage("SecurityUiLabels", "SupportedTextFileFormats", locale); + return ServiceUtil.returnError(errorMessage); + } Map<String, Object> context = UtilMisc.makeMapWritable(rcontext); context.put("skipPermissionCheck", "true"); return createFileMethod(dctx, context); @@ -254,7 +263,9 @@ public class DataServices { if (UtilValidate.isNotEmpty(textData)) { try (OutputStreamWriter out = new OutputStreamWriter(new FileOutputStream(file), StandardCharsets.UTF_8);) { out.write(textData); - if (!org.apache.ofbiz.security.SecuredUpload.isValidFile(file.getAbsolutePath(), "Text", delegator)) { + // Check if a webshell is not uploaded + // TODO I believe the call below to SecuredUpload::isValidFile is now useless because of the same in createFileNoPerm + if (!SecuredUpload.isValidFile(file.getAbsolutePath(), "Text", delegator)) { String errorMessage = UtilProperties.getMessage("SecurityUiLabels", "SupportedTextFileFormats", locale); return ServiceUtil.returnError(errorMessage); } @@ -265,10 +276,11 @@ public class DataServices { } } else if (binData != null) { try { - // Check if a webshell is not uploaded Path tempFile = Files.createTempFile(null, null); Files.write(tempFile, binData.array(), StandardOpenOption.APPEND); - if (!org.apache.ofbiz.security.SecuredUpload.isValidFile(tempFile.toString(), "All", delegator)) { + // Check if a webshell is not uploaded + // TODO I believe the call below to SecuredUpload::isValidFile is now useless because of the same in createFileNoPerm + if (!SecuredUpload.isValidFile(tempFile.toString(), "All", delegator)) { String errorMessage = UtilProperties.getMessage("SecurityUiLabels", "SupportedFileFormatsIncludingSvg", locale); return ServiceUtil.returnError(errorMessage); } @@ -461,7 +473,8 @@ public class DataServices { try (OutputStreamWriter out = new OutputStreamWriter(new FileOutputStream(file), StandardCharsets.UTF_8);) { out.write(textData); // Check if a webshell is not uploaded - if (!org.apache.ofbiz.security.SecuredUpload.isValidFile(file.getAbsolutePath(), "Text", delegator)) { + // TODO I believe the call below to SecuredUpload::isValidFile is now useless because of the same in createFileNoPerm + if (!SecuredUpload.isValidFile(file.getAbsolutePath(), "Text", delegator)) { String errorMessage = UtilProperties.getMessage("SecurityUiLabels", "SupportedTextFileFormats", locale); return ServiceUtil.returnError(errorMessage); } @@ -473,9 +486,10 @@ public class DataServices { } else if (binData != null) { try { // Check if a webshell is not uploaded + // TODO I believe the call below to SecuredUpload::isValidFile is now useless because of the same in createFileNoPerm Path tempFile = Files.createTempFile(null, null); Files.write(tempFile, binData.array(), StandardOpenOption.APPEND); - if (!org.apache.ofbiz.security.SecuredUpload.isValidFile(tempFile.toString(), "Image", delegator)) { + if (!SecuredUpload.isValidFile(tempFile.toString(), "Image", delegator)) { String errorMessage = UtilProperties.getMessage("SecurityUiLabels", "SupportedFileFormatsIncludingSvg", locale); return ServiceUtil.returnError(errorMessage); } @@ -648,7 +662,8 @@ public class DataServices { try (FileOutputStream out = new FileOutputStream(file);) { out.write(imageData); // Check if a webshell is not uploaded - if (!org.apache.ofbiz.security.SecuredUpload.isValidFile(file.getAbsolutePath(), "All", delegator)) { + // TODO I believe the call below to SecuredUpload::isValidFile is now useless because of the same in createFileNoPerm + if (!SecuredUpload.isValidFile(file.getAbsolutePath(), "All", delegator)) { String errorMessage = UtilProperties.getMessage("SecurityUiLabels", "SupportedFileFormatsIncludingSvg", locale); return ServiceUtil.returnError(errorMessage); } @@ -707,7 +722,8 @@ public class DataServices { try (FileOutputStream out = new FileOutputStream(file);) { out.write(imageData); // Check if a webshell is not uploaded - if (!org.apache.ofbiz.security.SecuredUpload.isValidFile(file.getAbsolutePath(), "All", delegator)) { + // TODO I believe the call below to SecuredUpload::isValidFile is now useless because of the same in createFileNoPerm + if (!SecuredUpload.isValidFile(file.getAbsolutePath(), "All", delegator)) { String errorMessage = UtilProperties.getMessage("SecurityUiLabels", "SupportedFileFormatsIncludingSvg", locale); return ServiceUtil.returnError(errorMessage); } 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 1d117bd..5173e22 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 @@ -107,7 +107,7 @@ public class SecuredUpload { return DENIEDWEBSHELLTOKENS.stream().allMatch(token -> isValid(content, token, allowed)); } - private static boolean isValidFileName(String fileToCheck, Delegator delegator) throws IOException { + public static boolean isValidFileName(String fileToCheck, Delegator delegator) throws IOException { // Allow all if (("true".equalsIgnoreCase(EntityUtilProperties.getPropertyValue("security", "allowAllUploads", delegator)))) { return true; @@ -133,18 +133,19 @@ public class SecuredUpload { // Check extensions if (p != null && p.getFileName() != null) { String fileName = p.getFileName().toString(); // The file name is the farthest element from the root in the directory hierarchy. + String extension = FilenameUtils.getExtension(fileToCheck).toLowerCase(); // Prevents null byte in filename - if (fileName.contains("%00") - || fileName.contains("%0a") - || fileName.contains("%20") - || fileName.contains("%0d%0a") - || fileName.contains("/") - || fileName.contains("./") - || fileName.contains(".")) { - Debug.logError("Special bytes in filename are not allowed for security reason", MODULE); + if (extension.contains("%00") + || extension.contains("%0a") + || extension.contains("%20") + || extension.contains("%0d%0a") + || extension.contains("/") + || extension.contains("./") + || extension.contains(".")) { + Debug.logError("Special bytes in extension are not allowed for security reason", MODULE); return false; } - if (DENIEDFILEEXTENSIONS.contains(FilenameUtils.getExtension(fileToCheck).toLowerCase())) { + if (DENIEDFILEEXTENSIONS.contains(extension)) { Debug.logError("This file extension is not allowed for security reason", MODULE); deleteBadFile(fileToCheck); return false;