You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ofbiz.apache.org by jl...@apache.org on 2022/02/19 11:10:22 UTC
[ofbiz-framework] branch release22.01 updated (b476d67 -> 3b29cd3)
This is an automated email from the ASF dual-hosted git repository.
jleroux pushed a change to branch release22.01
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git.
from b476d67 Fixed: Secure the uploads (OFBIZ-12080)
new e25470c Fixed: Secure the uploads (OFBIZ-12080)
new 3b29cd3 Improved: Secure the uploads (OFBIZ-12080)
The 2 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
.../apache/ofbiz/content/data/DataServices.java | 32 ++++++++++++++++------
framework/security/config/security.properties | 4 +--
.../org/apache/ofbiz/security/SecuredUpload.java | 19 +++++++++++--
3 files changed, 42 insertions(+), 13 deletions(-)
[ofbiz-framework] 01/02: Fixed: Secure the uploads (OFBIZ-12080)
Posted by jl...@apache.org.
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 e25470ce1347e75d4b7a0e97c3c93c2d6dce1e14
Author: Jacques Le Roux <ja...@les7arts.com>
AuthorDate: Fri Feb 18 18:35:09 2022 +0100
Fixed: Secure the uploads (OFBIZ-12080)
Prevents special bytes in filename
Adds some deniedFileExtensions
---
framework/security/config/security.properties | 4 ++--
.../main/java/org/apache/ofbiz/security/SecuredUpload.java | 14 +++++++++++++-
2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/framework/security/config/security.properties b/framework/security/config/security.properties
index fa96158..576e69f 100644
--- a/framework/security/config/security.properties
+++ b/framework/security/config/security.properties
@@ -236,8 +236,8 @@ templateClassResolver=
#--
#-- List of denied files suffixes to be uploaded
#-- OFBiz of course also check contents...
-deniedFileExtensions=html,htm,php,php1,php2,hph3,php4,php5,asp,aspx,asa,asax,ascx,ashx,asmx,jsp,jspa,jspx,jsw,jsv,jspf,jtml,cfm,cfc,bat,exe,com,dll,\
- vbs,js,reg,cgi,htaccess,asis,sh,phtm,shtm,inc,asp,cdx,asa,cer,py,pl,shtml,hta,ps1,tag
+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,htaccess,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
#--
#-- The upload vulnerability is only a post-auth (needs a credential with suitable permissions),
#-- people may like to allow more than what is allowed OOTB
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 793e68a..1d117bd 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
@@ -113,12 +113,13 @@ public class SecuredUpload {
return true;
}
- // Prevent double extensions
+ // Prevents double extensions
if (StringUtils.countMatches(fileToCheck, ".") > 1) {
Debug.logError("Double extensions are not allowed for security reason", MODULE);
return false;
}
+
// Check max line length, default 10000
if (!checkMaxLinesLength(fileToCheck)) {
Debug.logError("For security reason lines over " + MAXLINELENGTH.toString() + " are not allowed", MODULE);
@@ -132,6 +133,17 @@ 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.
+ // 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);
+ return false;
+ }
if (DENIEDFILEEXTENSIONS.contains(FilenameUtils.getExtension(fileToCheck).toLowerCase())) {
Debug.logError("This file extension is not allowed for security reason", MODULE);
deleteBadFile(fileToCheck);
[ofbiz-framework] 02/02: Improved: Secure the uploads (OFBIZ-12080)
Posted by jl...@apache.org.
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 <ja...@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;