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 2021/09/03 12:07:15 UTC
[ofbiz-framework] branch release17.12 updated: Fixed:
CVE-2021-37608 vulnerability bypass (OFBIZ-12307)
This is an automated email from the ASF dual-hosted git repository.
jleroux pushed a commit to branch release17.12
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
The following commit(s) were added to refs/heads/release17.12 by this push:
new dfd71bf Fixed: CVE-2021-37608 vulnerability bypass (OFBIZ-12307)
dfd71bf is described below
commit dfd71bf7dd552a7cdc287bcb6e2da30cee4cd093
Author: Jacques Le Roux <ja...@les7arts.com>
AuthorDate: Fri Sep 3 13:47:15 2021 +0200
Fixed: CVE-2021-37608 vulnerability bypass (OFBIZ-12307)
The fix I did is two folds:
filters extensions (thanks to Zhujie's suggestion of a list of extensions to ban)
deletes bad files at the right place (thanks to thiscodecc's report)
Thanks: thiscodecc for the security report
---
framework/security/config/security.properties | 6 ++-
.../org/apache/ofbiz/security/SecuredUpload.java | 55 +++++++++++++++++-----
2 files changed, 47 insertions(+), 14 deletions(-)
diff --git a/framework/security/config/security.properties b/framework/security/config/security.properties
index c19ccc6..c0ff597 100644
--- a/framework/security/config/security.properties
+++ b/framework/security/config/security.properties
@@ -151,7 +151,7 @@ SameSiteCookieAttribute=
templateClassResolver=
-#-- UPLOAD: supported file formats are *safe* PNG, GIF, TIFF, JPEG, PDF, Audio and Video and ZIP
+#-- ===== UPLOAD: supported file formats are *safe* PNG, GIF, TIFF, JPEG, PDF, Audio and Video and ZIP
#--
#-- No proprietary file formats (Excel, Word, etc.) are handled OOTB.
#-- They can be handled by custom projects using https://github.com/righettod/document-upload-protection:
@@ -174,6 +174,10 @@ 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
+#-- OFBiz of course also check contents...
+deniedFileExtensions=html,htm,php,php2,hph3,php4,php5,asp,aspx,ascx,jsp,jspx,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
+#--
#-- 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
#-- As it name says, allowAllUploads opens all possibilities
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 3dfdcc8..59ef0fe 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
@@ -38,6 +38,7 @@ import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
+import java.util.LinkedList;
import java.util.List;
import java.util.Set;
import java.util.UUID;
@@ -60,8 +61,12 @@ import org.apache.commons.imaging.formats.jpeg.JpegImageParser;
import org.apache.commons.imaging.formats.png.PngImageParser;
import org.apache.commons.imaging.formats.tiff.TiffImageParser;
import org.apache.commons.io.FileUtils;
+import org.apache.commons.io.FilenameUtils;
import org.apache.ofbiz.base.util.Debug;
import org.apache.ofbiz.base.util.FileUtil;
+import org.apache.ofbiz.base.util.StringUtil;
+import org.apache.ofbiz.base.util.UtilProperties;
+import org.apache.ofbiz.base.util.UtilValidate;
import org.apache.ofbiz.entity.Delegator;
import org.apache.ofbiz.entity.util.EntityUtilProperties;
import org.apache.pdfbox.pdmodel.PDDocument;
@@ -91,6 +96,7 @@ public class SecuredUpload {
// Line #-- UPLOAD: supported file formats are *safe* PNG, GIF, TIFF, JPEG, PDF, Audio and Video and ZIP
private static final String MODULE = SecuredUpload.class.getName();
+ private static final List<String> deniedFileExtensions = deniedFileExtensions();
/**
* @param fileToCheck
@@ -107,28 +113,33 @@ public class SecuredUpload {
String imageServerUrl = EntityUtilProperties.getPropertyValue("catalog", "image.management.url", delegator);
Path p = Paths.get(fileToCheck);
- String file = p.getFileName().toString();
+ String fileName = p.getFileName().toString(); // The file name is the farthest element from the root in the directory hierarchy.
boolean wrongFile = true;
+
+ if (deniedFileExtensions.contains(FilenameUtils.getExtension(fileToCheck))) {
+ Debug.logError("This file extension is not allowed for security reason", MODULE);
+ deleteBadFile(fileToCheck);
+ return false;
+ }
+
if (org.apache.commons.lang3.SystemUtils.IS_OS_WINDOWS) {
- if (fileToCheck.length() > 259) {
+ if (fileToCheck.length() < 259) {
Debug.logError("Uploaded file name too long", MODULE);
- return false;
} else if (p.toString().contains(imageServerUrl.replaceAll("/", "\\\\"))) {
- if (file.matches("[a-zA-Z0-9-_ ()]{1,249}.[a-zA-Z0-9-_ ]{1,10}")) { // "(" and ")" for duplicates files
+ if (fileName.matches("[a-zA-Z0-9-_ ()]{1,249}.[a-zA-Z0-9-_ ]{1,10}")) { // "(" and ")" for duplicates files
wrongFile = false;
}
- } else if (file.matches("[a-zA-Z0-9-_ ]{1,249}.[a-zA-Z0-9-_ ]{1,10}")) {
+ } else if (fileName.matches("[a-zA-Z0-9-_ ]{1,249}.[a-zA-Z0-9-_ ]{1,10}")) {
wrongFile = false;
}
} else { // Suppose a *nix system
if (fileToCheck.length() > 4096) {
Debug.logError("Uploaded file name too long", MODULE);
- return false;
} else if (p.toString().contains(imageServerUrl)) {
- if (file.matches("[a-zA-Z0-9-_ ()]{1,4086}.[a-zA-Z0-9-_ ]{1,10}")) { // "(" and ")" for duplicates files
+ if (fileName.matches("[a-zA-Z0-9-_ ()]{1,4086}.[a-zA-Z0-9-_ ]{1,10}")) { // "(" and ")" for duplicates files
wrongFile = false;
}
- } else if (file.matches("[a-zA-Z0-9-_ ]{1,4086}.[a-zA-Z0-9-_ ]{1,10}")) {
+ } else if (fileName.matches("[a-zA-Z0-9-_ ]{1,4086}.[a-zA-Z0-9-_ ]{1,10}")) {
wrongFile = false;
}
}
@@ -139,10 +150,12 @@ public class SecuredUpload {
+ " only 1 dot as an input for the file name and the extension."
+ "The file name and extension should not be empty at all",
MODULE);
+ deleteBadFile(fileToCheck);
return false;
}
if (isExecutable(fileToCheck)) {
+ deleteBadFile(fileToCheck);
return false;
}
@@ -210,11 +223,7 @@ public class SecuredUpload {
}
break;
}
- Debug.logError("File :" + fileToCheck + ", can't be uploaded for security reason", MODULE);
- File badFile = new File(fileToCheck);
- if (!badFile.delete()) {
- Debug.logError("File :" + fileToCheck + ", couldn't be deleted", MODULE);
- }
+ deleteBadFile(fileToCheck);
return false;
}
@@ -642,4 +651,24 @@ public class SecuredUpload {
// TODO.... to be continued with known webshell contents... a complete allow list is impossible anyway...
// eg: https://www.acunetix.com/blog/articles/detection-prevention-introduction-web-shells-part-5/
}
+
+ private static void deleteBadFile(String fileToCheck) {
+ Debug.logError("File :" + fileToCheck + ", can't be uploaded for security reason", MODULE);
+ File badFile = new File(fileToCheck);
+ if (!badFile.delete()) {
+ Debug.logError("File :" + fileToCheck + ", couldn't be deleted", MODULE);
+ }
+ }
+
+ private static List<String> deniedFileExtensions() {
+ List<String> deniedFileExtensions = new LinkedList<>();
+ String deniedExtensions = UtilProperties.getPropertyValue("security", "deniedFileExtensions");
+ if (UtilValidate.isNotEmpty(deniedExtensions)) {
+ List<String> deniedFileExtensionsList = StringUtil.split(deniedExtensions, ",");
+ for (String deniedExtension : deniedFileExtensionsList) {
+ deniedFileExtensions.add(deniedExtension);
+ }
+ }
+ return deniedFileExtensions;
+ }
}