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 00d8752 Fixed: Secure the uploads (OFBIZ-12080)
00d8752 is described below
commit 00d875251c94496b198ef4fdad61f42fd8318727
Author: Jacques Le Roux <[email protected]>
AuthorDate: Mon Dec 14 12:46:32 2020 +0100
Fixed: Secure the uploads (OFBIZ-12080)
Prevents too long filenames as recommended by OWASP.
Based on
https://security.stackexchange.com/questions/46484/denial-of-service-when-uploading-a-file#answer-46495
I decided to not limit sizes of files. Anyway we know it's only post-auth...
---
.../org/apache/ofbiz/security/SecuredUpload.java | 56 +++++++++++++---------
1 file changed, 33 insertions(+), 23 deletions(-)
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 0751067..a8321dc 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
@@ -93,51 +93,61 @@ public class SecuredUpload {
private static final String MODULE = SecuredUpload.class.getName();
/**
- * @param fileTocheck
+ * @param fileToCheck
* @param fileType
* @return true if the file is valid
* @throws IOException
* @throws ImageReadException
*/
- public static boolean isValidFile(String fileTocheck, String fileType,
Delegator delegator) throws IOException, ImageReadException {
+ public static boolean isValidFile(String fileToCheck, String fileType,
Delegator delegator) throws IOException, ImageReadException {
if
(("true".equalsIgnoreCase(EntityUtilProperties.getPropertyValue("security",
"allowAllUploads", delegator)))) {
return true;
}
- if (isExecutable(fileTocheck)) {
+ if (org.apache.commons.lang3.SystemUtils.IS_OS_WINDOWS) {
+ if (fileToCheck.length() > 259) {
+ return false;
+ }
+ } else {
+ if (fileToCheck.length() > 4096) {
+ return false;
+ }
+ }
+
+ if (isExecutable(fileToCheck)) {
return false;
}
switch (fileType) {
case "Image":
- if (isValidImageFile(fileTocheck)) {
+ if (isValidImageFile(fileToCheck)) {
return true;
}
break;
case "ImageAndSvg":
- if (isValidImageIncludingSvgFile(fileTocheck)) {
+ if (isValidImageIncludingSvgFile(fileToCheck)) {
return true;
}
break;
case "PDF":
- if (isValidPdfFile(fileTocheck)) {
+ if (isValidPdfFile(fileToCheck)) {
return true;
}
break;
case "Compressed":
- if (isValidCompressedFile(fileTocheck, delegator)) {
+ if (isValidCompressedFile(fileToCheck, delegator)) {
return true;
}
break;
case "AllButCompressed":
- if (isValidTextFile(fileTocheck)
- || isValidImageIncludingSvgFile(fileTocheck)
- || isValidPdfFile(fileTocheck)) {
+ if (isValidTextFile(fileToCheck)
+ || isValidImageIncludingSvgFile(fileToCheck)
+ || isValidPdfFile(fileToCheck)) {
return true;
}
break;
@@ -146,37 +156,37 @@ public class SecuredUpload {
// The philosophy for isValidTextFile() is that
// 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 isValidTextFile
- if (isValidTextFile(fileTocheck)) {
+ if (isValidTextFile(fileToCheck)) {
return true;
}
break;
case "Audio":
- if (isValidAudioFile(fileTocheck)) {
+ if (isValidAudioFile(fileToCheck)) {
return true;
}
break;
case "Video":
- if (isValidVideoFile(fileTocheck)) {
+ if (isValidVideoFile(fileToCheck)) {
return true;
}
break;
default: // All
- if (isValidTextFile(fileTocheck)
- || isValidImageIncludingSvgFile(fileTocheck)
- || isValidCompressedFile(fileTocheck, delegator)
- || isValidAudioFile(fileTocheck)
- || isValidVideoFile(fileTocheck)
- || isValidPdfFile(fileTocheck)) {
+ if (isValidTextFile(fileToCheck)
+ || isValidImageIncludingSvgFile(fileToCheck)
+ || isValidCompressedFile(fileToCheck, delegator)
+ || isValidAudioFile(fileToCheck)
+ || isValidVideoFile(fileToCheck)
+ || isValidPdfFile(fileToCheck)) {
return true;
}
break;
}
- Debug.logError("File :" + fileTocheck + ", can't be uploaded for
security reason", MODULE);
- File badFile = new File(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);
+ Debug.logError("File :" + fileToCheck + ", couldn't be deleted",
MODULE);
}
return false;
}
@@ -596,7 +606,7 @@ public class SecuredUpload {
|| content.toLowerCase().contains("fopen")
|| content.toLowerCase().contains("fclose")
|| content.toLowerCase().contains("readfile"));
- // TODO.... to be continued with known webshell contents... a complete
allow list is impossible anyway
+ // 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/
}
}
