This is an automated email from the ASF dual-hosted git repository.
jleroux pushed a commit to branch release18.12
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
The following commit(s) were added to refs/heads/release18.12 by this push:
new 50fc66c Fixed: Secure the uploads (OFBIZ-12080)
50fc66c is described below
commit 50fc66c8e051947ae7f664a68c344e7b41930722
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 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 364c066..bbace32 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 41be1a9..672fa43 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
@@ -55,14 +55,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)
