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/20 14:45:31 UTC
[ofbiz-framework] branch release22.01 updated (3b29cd3 -> 4d70280)
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 3b29cd3 Improved: Secure the uploads (OFBIZ-12080)
new b885423 Improved: no functional change, just removes a line
new 4d70280 Fixed: 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 | 21 ++++++++++++++---
.../org/apache/ofbiz/security/SecuredUpload.java | 26 +++++++++++-----------
.../ofbiz/service/engine/GroovyBaseScript.groovy | 20 ++++++++++++-----
3 files changed, 46 insertions(+), 21 deletions(-)
[ofbiz-framework] 02/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 4d70280cb86c65a096b71b560623d33bc563e960
Author: Jacques Le Roux <ja...@les7arts.com>
AuthorDate: Sun Feb 20 15:39:11 2022 +0100
Fixed: Secure the uploads (OFBIZ-12080)
Last commits put in an issue in DataServices.java and GroovyBaseScript.groovy.
I should have used SecuredUpload::isValidFileName with dataResourceName and
SecuredUpload::isValidFile with objectInfo. This fixes that.
Also fixes formatting checkstyle issues in SecuredUpload class.
Also fixes formatting checkstyle issues in SecuredUpload class and move allow
all and check line length in right places (it's about content)
---
.../apache/ofbiz/content/data/DataServices.java | 21 +++++++++++++++---
.../org/apache/ofbiz/security/SecuredUpload.java | 25 +++++++++++-----------
.../ofbiz/service/engine/GroovyBaseScript.groovy | 20 ++++++++++++-----
3 files changed, 46 insertions(+), 20 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 2bd71c1..cec71ac 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
@@ -199,12 +199,27 @@ public class DataServices {
public static Map<String, Object> createFileNoPerm(DispatchContext dctx, Map<String, ? extends Object> rcontext) throws IOException,
ImageReadException {
String originalFileName = (String) rcontext.get("dataResourceName");
+ String fileNameAndPath = (String) rcontext.get("objectInfo");
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);
+ File file = new File(fileNameAndPath);
+ if (!originalFileName.isEmpty()) {
+ // Check the file name
+ if (!org.apache.ofbiz.security.SecuredUpload.isValidFileName(originalFileName, delegator)) {
+ String errorMessage = UtilProperties.getMessage("SecurityUiLabels", "SupportedFileFormatsIncludingSvg", locale);
+ return ServiceUtil.returnError(errorMessage);
+ }
+ // TODO we could verify the file type (here "All") with dataResourceTypeId. Anyway it's done with isValidFile()
+ // We would just have a better error message
+ if (file.exists()) {
+ // Check if a webshell is not uploaded
+ if (!org.apache.ofbiz.security.SecuredUpload.isValidFile(fileNameAndPath, "All", delegator)) {
+ String errorMessage = UtilProperties.getMessage("SecurityUiLabels", "SupportedFileFormatsIncludingSvg", locale);
+ return ServiceUtil.returnError(errorMessage);
+ }
+ }
}
+
Map<String, Object> context = UtilMisc.makeMapWritable(rcontext);
context.put("skipPermissionCheck", "true");
return createFileMethod(dctx, context);
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..029db47 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
@@ -108,23 +108,12 @@ public class SecuredUpload {
}
public static boolean isValidFileName(String fileToCheck, Delegator delegator) throws IOException {
- // Allow all
- if (("true".equalsIgnoreCase(EntityUtilProperties.getPropertyValue("security", "allowAllUploads", delegator)))) {
- return true;
- }
-
// 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);
- return false;
- }
-
String imageServerUrl = EntityUtilProperties.getPropertyValue("catalog", "image.management.url", delegator);
Path p = Paths.get(fileToCheck);
boolean wrongFile = true;
@@ -197,12 +186,24 @@ public class SecuredUpload {
* @throws ImageReadException
*/
public static boolean isValidFile(String fileToCheck, String fileType, Delegator delegator) throws IOException, ImageReadException {
+ // Allow all
+ if (("true".equalsIgnoreCase(EntityUtilProperties.getPropertyValue("security", "allowAllUploads", delegator)))) {
+ return true;
+ }
+ // Check the file name
if (!isValidFileName(fileToCheck, delegator)) {
return false;
}
- // Check the the file content
+ // Check the file content
+
+ // Check max line length, default 10000
+ if (!checkMaxLinesLength(fileToCheck)) {
+ Debug.logError("For security reason lines over " + MAXLINELENGTH.toString() + " are not allowed", MODULE);
+ return false;
+ }
+
if (isExecutable(fileToCheck)) {
deleteBadFile(fileToCheck);
return false;
diff --git a/framework/service/src/main/groovy/org/apache/ofbiz/service/engine/GroovyBaseScript.groovy b/framework/service/src/main/groovy/org/apache/ofbiz/service/engine/GroovyBaseScript.groovy
index 377b05d..0c338f8 100644
--- a/framework/service/src/main/groovy/org/apache/ofbiz/service/engine/GroovyBaseScript.groovy
+++ b/framework/service/src/main/groovy/org/apache/ofbiz/service/engine/GroovyBaseScript.groovy
@@ -54,14 +54,24 @@ abstract class GroovyBaseScript extends Script {
: this.binding.getVariable('parameters').locale
}
if (serviceName.equals("createAnonFile")) {
- String dataResourceName = inputMap.get("dataResourceName")
- File file = new File(dataResourceName)
- if (file.exists()) {
- // Check if a webshell is not uploaded
- if (!org.apache.ofbiz.security.SecuredUpload.isValidFile(dataResourceName, "All", delegator)) {
+ String fileName = inputMap.get("dataResourceName")
+ String fileNameAndPath = inputMap.get("objectInfo")
+ File file = new File(fileNameAndPath)
+ if (!fileName.isEmpty()) {
+ // Check the file name
+ if (!org.apache.ofbiz.security.SecuredUpload.isValidFileName(fileName, delegator)) {
String errorMessage = UtilProperties.getMessage("SecurityUiLabels", "SupportedFileFormatsIncludingSvg", inputMap.locale)
throw new ExecutionServiceException(errorMessage)
}
+ // TODO we could verify the file type (here "All") with dataResourceTypeId. Anyway it's done with isValidFile()
+ // We would just have a better error message
+ if (file.exists()) {
+ // Check if a webshell is not uploaded
+ if (!org.apache.ofbiz.security.SecuredUpload.isValidFile(fileNameAndPath, "All", delegator)) {
+ String errorMessage = UtilProperties.getMessage("SecurityUiLabels", "SupportedFileFormatsIncludingSvg", inputMap.locale)
+ throw new ExecutionServiceException(errorMessage)
+ }
+ }
}
}
Map serviceContext = dctx.makeValidContext(serviceName, ModelService.IN_PARAM, inputMap)
[ofbiz-framework] 01/02: Improved: no functional change, just removes a line
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 b885423107bbee3ad72dc8e81f049dffc47d3abb
Author: Jacques Le Roux <ja...@les7arts.com>
AuthorDate: Sun Feb 20 09:27:58 2022 +0100
Improved: no functional change, just removes a line
This is because I pushed all from 22.01 branch instead of trunk and there are
checkstyle issues in trunk, in SecuredUpload.java
---
.../security/src/main/java/org/apache/ofbiz/security/SecuredUpload.java | 1 -
1 file changed, 1 deletion(-)
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 5173e22..563d6cb 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
@@ -119,7 +119,6 @@ public class SecuredUpload {
return false;
}
-
// Check max line length, default 10000
if (!checkMaxLinesLength(fileToCheck)) {
Debug.logError("For security reason lines over " + MAXLINELENGTH.toString() + " are not allowed", MODULE);