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
The following commit(s) were added to refs/heads/trunk by this push:
new 3c9cd71 Fixed: Secure the uploads (OFBIZ-12080)
3c9cd71 is described below
commit 3c9cd7139ac02b84eb2c3b0fbc06b5a21d4492d1
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 | 49 +++++++++++-----------
.../ofbiz/service/engine/GroovyBaseScript.groovy | 20 ++++++---
3 files changed, 58 insertions(+), 32 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 b93320a..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;
@@ -158,24 +147,24 @@ public class SecuredUpload {
} else if
(p.toString().contains(imageServerUrl.replaceAll("/", "\\\\"))) {
// 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;
+ }
+ } else if (fileName.matches("[a-zA-Z0-9-_
]{1,249}.[a-zA-Z0-9-_ ]{1,10}")) {
wrongFile = false;
}
- } else if (fileName.matches("[a-zA-Z0-9-_ ]{1,249}.[a-zA-Z0-9-_
]{1,10}")) {
- wrongFile = false;
- }
- } else { // Suppose a *nix system
- if (fileToCheck.length() > 4096) {
- Debug.logError("Uploaded file name too long", MODULE);
- } else if (p.toString().contains(imageServerUrl)) {
+ } else { // Suppose a *nix system
+ if (fileToCheck.length() > 4096) {
+ Debug.logError("Uploaded file name too long", MODULE);
+ } else if (p.toString().contains(imageServerUrl)) {
// TODO check this is still useful in at least 1 case
- if (fileName.matches("[a-zA-Z0-9-_ ()]{1,4086}.[a-zA-Z0-9-_
]{1,10}")) { // "(" and ")" for duplicates files
+ if (fileName.matches("[a-zA-Z0-9-_
()]{1,4086}.[a-zA-Z0-9-_ ]{1,10}")) { // "(" and ")" for duplicates files
+ wrongFile = false;
+ }
+ } else if (fileName.matches("[a-zA-Z0-9-_
]{1,4086}.[a-zA-Z0-9-_ ]{1,10}")) {
wrongFile = false;
}
- } else if (fileName.matches("[a-zA-Z0-9-_ ]{1,4086}.[a-zA-Z0-9-_
]{1,10}")) {
- wrongFile = false;
}
}
- }
if (wrongFile) {
Debug.logError("Uploaded file "
@@ -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;
@@ -385,7 +386,7 @@ public class SecuredUpload {
safeState = true;
} catch (IOException | ImageReadException | ImageWriteException e)
{
Debug.logWarning(e, "Error during Image file " + fileName + "
processing !", MODULE);
- }
+ }
}
return safeState;
}
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)
