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 <jacques.le.r...@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)
