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;
