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:25 UTC
[ofbiz-framework] branch trunk updated (3f69a54 -> b7f1d28)
This is an automated email from the ASF dual-hosted git repository.
jleroux pushed a change to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git.
from 3f69a54 Fixed: Secure the uploads (OFBIZ-12080)
new 30cb6da Fixed: Secure the uploads (OFBIZ-12080)
new 9b497d5 Improved: Secure the uploads (OFBIZ-12080)
new 04e4433 Improved: Secure the uploads (OFBIZ-12080)
new b7f1d28 Improved: Secure the uploads (OFBIZ-12080)
The 4 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 | 32 ++++++++++++-----
framework/security/config/security.properties | 4 +--
.../org/apache/ofbiz/security/SecuredUpload.java | 42 ++++++++++++++--------
3 files changed, 53 insertions(+), 25 deletions(-)
[ofbiz-framework] 02/04: Improved: 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 trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
commit 9b497d55cbf1f19a8280cbed5e581208a1d937eb
Author: Jacques Le Roux <ja...@les7arts.com>
AuthorDate: Sat Feb 19 11:44:32 2022 +0100
Improved: Secure the uploads (OFBIZ-12080)
Manually merges SecuredUpload.java based on 18.12
---
.../org/apache/ofbiz/security/SecuredUpload.java | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
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 1d117bd..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
@@ -107,7 +107,7 @@ public class SecuredUpload {
return DENIEDWEBSHELLTOKENS.stream().allMatch(token -> isValid(content, token, allowed));
}
- private static boolean isValidFileName(String fileToCheck, Delegator delegator) throws IOException {
+ public static boolean isValidFileName(String fileToCheck, Delegator delegator) throws IOException {
// Allow all
if (("true".equalsIgnoreCase(EntityUtilProperties.getPropertyValue("security", "allowAllUploads", delegator)))) {
return true;
@@ -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);
@@ -133,18 +132,19 @@ public class SecuredUpload {
// Check extensions
if (p != null && p.getFileName() != null) {
String fileName = p.getFileName().toString(); // The file name is the farthest element from the root in the directory hierarchy.
+ String extension = FilenameUtils.getExtension(fileToCheck).toLowerCase();
// Prevents null byte in filename
- if (fileName.contains("%00")
- || fileName.contains("%0a")
- || fileName.contains("%20")
- || fileName.contains("%0d%0a")
- || fileName.contains("/")
- || fileName.contains("./")
- || fileName.contains(".")) {
- Debug.logError("Special bytes in filename are not allowed for security reason", MODULE);
+ if (extension.contains("%00")
+ || extension.contains("%0a")
+ || extension.contains("%20")
+ || extension.contains("%0d%0a")
+ || extension.contains("/")
+ || extension.contains("./")
+ || extension.contains(".")) {
+ Debug.logError("Special bytes in extension are not allowed for security reason", MODULE);
return false;
}
- if (DENIEDFILEEXTENSIONS.contains(FilenameUtils.getExtension(fileToCheck).toLowerCase())) {
+ if (DENIEDFILEEXTENSIONS.contains(extension)) {
Debug.logError("This file extension is not allowed for security reason", MODULE);
deleteBadFile(fileToCheck);
return false;
[ofbiz-framework] 01/04: 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 trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
commit 30cb6da0b160ed2ff712709710cd9d95fca4f93c
Author: Jacques Le Roux <ja...@les7arts.com>
AuthorDate: Fri Feb 18 18:35:09 2022 +0100
Fixed: Secure the uploads (OFBIZ-12080)
Prevents special bytes in filename
Adds some deniedFileExtensions
---
framework/security/config/security.properties | 4 ++--
.../main/java/org/apache/ofbiz/security/SecuredUpload.java | 14 +++++++++++++-
2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/framework/security/config/security.properties b/framework/security/config/security.properties
index fa96158..576e69f 100644
--- a/framework/security/config/security.properties
+++ b/framework/security/config/security.properties
@@ -236,8 +236,8 @@ templateClassResolver=
#--
#-- List of denied files suffixes to be uploaded
#-- OFBiz of course also check contents...
-deniedFileExtensions=html,htm,php,php1,php2,hph3,php4,php5,asp,aspx,asa,asax,ascx,ashx,asmx,jsp,jspa,jspx,jsw,jsv,jspf,jtml,cfm,cfc,bat,exe,com,dll,\
- vbs,js,reg,cgi,htaccess,asis,sh,phtm,shtm,inc,asp,cdx,asa,cer,py,pl,shtml,hta,ps1,tag
+deniedFileExtensions=html,htm,php,php1,php2,hph3,php4,php5,php6,php7,phps,asp,aspx,asa,asax,ascx,ashx,asmx,jsp,jspa,jspx,jsw,jsv,jspf,jtml,cfm,cfc,bat,exe,com,dll,\
+ vbs,js,reg,cgi,htaccess,asis,sh,phtm,pht,phtml,shtm,inc,asp,cdx,asa,cer,py,pl,shtml,hta,ps1,tag,pgif,htaccess,phar,inc,cgi,wss,do,action
#--
#-- The upload vulnerability is only a post-auth (needs a credential with suitable permissions),
#-- people may like to allow more than what is allowed OOTB
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 793e68a..1d117bd 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
@@ -113,12 +113,13 @@ public class SecuredUpload {
return true;
}
- // Prevent double extensions
+ // 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);
@@ -132,6 +133,17 @@ public class SecuredUpload {
// Check extensions
if (p != null && p.getFileName() != null) {
String fileName = p.getFileName().toString(); // The file name is the farthest element from the root in the directory hierarchy.
+ // Prevents null byte in filename
+ if (fileName.contains("%00")
+ || fileName.contains("%0a")
+ || fileName.contains("%20")
+ || fileName.contains("%0d%0a")
+ || fileName.contains("/")
+ || fileName.contains("./")
+ || fileName.contains(".")) {
+ Debug.logError("Special bytes in filename are not allowed for security reason", MODULE);
+ return false;
+ }
if (DENIEDFILEEXTENSIONS.contains(FilenameUtils.getExtension(fileToCheck).toLowerCase())) {
Debug.logError("This file extension is not allowed for security reason", MODULE);
deleteBadFile(fileToCheck);
[ofbiz-framework] 03/04: Improved: 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 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;
}
[ofbiz-framework] 04/04: Improved: 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 trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
commit b7f1d28142dca7dc3c7e9fd2d1e6675f6980bc7e
Author: Jacques Le Roux <ja...@les7arts.com>
AuthorDate: Sat Feb 19 11:51:14 2022 +0100
Improved: Secure the uploads (OFBIZ-12080)
Manually merges SecuredUpload.java based on 18.12, 2nd time because of previous
commit :/
---
.../org/apache/ofbiz/security/SecuredUpload.java | 48 +++++-----------------
1 file changed, 11 insertions(+), 37 deletions(-)
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 aac85a0..b93320a 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,7 +145,6 @@ 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;
@@ -157,37 +156,23 @@ 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;
- }
- } 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 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)) {
// 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
- wrongFile = false;
- }
- } else if (fileName.matches("[a-zA-Z0-9-_ ]{1,4086}.[a-zA-Z0-9-_ ]{1,10}")) {
+ 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;
}
}
}
@@ -320,13 +305,8 @@ 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);
@@ -403,16 +383,10 @@ 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;
}