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:28 UTC
[ofbiz-framework] 03/04: Improved: Secure the uploads (OFBIZ-12080)
This is an automated email from the ASF dual-hosted git repository.
jleroux pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
commit 04e4433b78860fd48512d945c86b357fbedc1e38
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
---
.../apache/ofbiz/content/data/DataServices.java | 32 ++++++++++++++++------
.../org/apache/ofbiz/security/SecuredUpload.java | 26 ++++++++++++++++++
2 files changed, 50 insertions(+), 8 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 563d6cb..aac85a0 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
@@ -145,6 +145,7 @@ public class SecuredUpload {
return false;
}
if (DENIEDFILEEXTENSIONS.contains(extension)) {
+<<<<<<< HEAD
Debug.logError("This file extension is not allowed for security reason", MODULE);
deleteBadFile(fileToCheck);
return false;
@@ -156,6 +157,19 @@ public class SecuredUpload {
if (fileToCheck.length() > 259) {
Debug.logError("Uploaded file name too long", MODULE);
} else if (p.toString().contains(imageServerUrl.replaceAll("/", "\\\\"))) {
+=======
+ Debug.logError("This file extension is not allowed for security reason", MODULE);
+ deleteBadFile(fileToCheck);
+ return false;
+ }
+
+ // Check the file and path names
+ if (org.apache.commons.lang3.SystemUtils.IS_OS_WINDOWS) {
+ // More about that: https://docs.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation
+ if (fileToCheck.length() > 259) {
+ Debug.logError("Uploaded file name too long", MODULE);
+ } else if (p.toString().contains(imageServerUrl.replaceAll("/", "\\\\"))) {
+>>>>>>> 668e801ada (Improved: Secure the uploads (OFBIZ-12080))
// TODO check this is still useful in at least 1 case
if (fileName.matches("[a-zA-Z0-9-_ ()]{1,249}.[a-zA-Z0-9-_ ]{1,10}")) { // "(" and ")" for duplicates files
wrongFile = false;
@@ -176,6 +190,7 @@ public class SecuredUpload {
}
}
}
+ }
if (wrongFile) {
Debug.logError("Uploaded file "
@@ -305,8 +320,13 @@ public class SecuredUpload {
boolean safeState = false;
boolean fallbackOnApacheCommonsImaging;
+<<<<<<< HEAD
if ((file != null) && file.exists() && file.canRead() && file.canWrite()) {
try (OutputStream fos = Files.newOutputStream(file.toPath(), StandardOpenOption.WRITE)) {
+=======
+ if ((file != null) && file.exists() && file.canRead() && file.canWrite()) {
+ try (OutputStream fos = Files.newOutputStream(file.toPath(), StandardOpenOption.WRITE)) {
+>>>>>>> 668e801ada (Improved: Secure the uploads (OFBIZ-12080))
// Get the image format
String formatName;
ImageInputStream iis = ImageIO.createImageInputStream(file);
@@ -383,10 +403,16 @@ public class SecuredUpload {
}
// Set state flag
safeState = true;
+<<<<<<< HEAD
} catch (IOException | ImageReadException | ImageWriteException e) {
Debug.logWarning(e, "Error during Image file " + fileName + " processing !", MODULE);
}
+=======
+ } catch (IOException | ImageReadException | ImageWriteException e) {
+ Debug.logWarning(e, "Error during Image file " + fileName + " processing !", MODULE);
+>>>>>>> 668e801ada (Improved: Secure the uploads (OFBIZ-12080))
}
+ }
return safeState;
}