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/27 12:40:57 UTC
[ofbiz-framework] branch trunk updated (72405ab -> 2752e4e)
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 72405ab Fixed: Secure the uploads (OFBIZ-12080)
new fe4cefd Improved: Reflected XSS in content component (OFBIZ-11840)
new 2752e4e Fixed: Stored XSS in webappPath parameter from content/control/EditWebSite (OFBIZ-12584)
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:
.../org/apache/ofbiz/content/data/DataEvents.java | 28 ++++++++++------------
framework/security/config/security.properties | 13 +++++++---
.../apache/ofbiz/security/SecurityUtilTest.java | 4 ++--
.../ofbiz/service/engine/EntityAutoEngine.java | 23 ++++++++++++++++++
4 files changed, 47 insertions(+), 21 deletions(-)
[ofbiz-framework] 01/02: Improved: Reflected XSS in content component (OFBIZ-11840)
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 fe4cefd590216443720d83823906677aedcb402e
Author: Jacques Le Roux <ja...@les7arts.com>
AuthorDate: Sun Feb 27 10:14:59 2022 +0100
Improved: Reflected XSS in content component (OFBIZ-11840)
This was already fixed but we can do better by using SecuredUpload::isValidText
Also grabs some tokens to security.properties::deniedWebShellTokens from
DataEvents.java, and adds "alert" even it's harmless as is, as it's often used
by attackers to demonstrate issues, so will earn time.
Though an improvement, I backport as it's related to security
---
.../org/apache/ofbiz/content/data/DataEvents.java | 28 ++++++++++------------
framework/security/config/security.properties | 13 +++++++---
.../apache/ofbiz/security/SecurityUtilTest.java | 4 ++--
3 files changed, 24 insertions(+), 21 deletions(-)
diff --git a/applications/content/src/main/java/org/apache/ofbiz/content/data/DataEvents.java b/applications/content/src/main/java/org/apache/ofbiz/content/data/DataEvents.java
index 1a91686..9fa8c7f 100644
--- a/applications/content/src/main/java/org/apache/ofbiz/content/data/DataEvents.java
+++ b/applications/content/src/main/java/org/apache/ofbiz/content/data/DataEvents.java
@@ -21,6 +21,7 @@ package org.apache.ofbiz.content.data;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
+import java.util.Collections;
import java.util.Locale;
import java.util.Map;
@@ -41,6 +42,7 @@ import org.apache.ofbiz.entity.GenericEntityException;
import org.apache.ofbiz.entity.GenericValue;
import org.apache.ofbiz.entity.util.EntityQuery;
import org.apache.ofbiz.entity.util.EntityUtilProperties;
+import org.apache.ofbiz.security.SecuredUpload;
import org.apache.ofbiz.service.GenericServiceException;
import org.apache.ofbiz.service.LocalDispatcher;
import org.apache.ofbiz.service.ServiceUtil;
@@ -83,22 +85,16 @@ public class DataEvents {
String permissionService = EntityUtilProperties.getPropertyValue("content", "stream.permission.service",
"genericContentPermission", delegator);
- // @formatter:off (prevent unwanted formatting in Eclipse)
- // For OFBIZ-11840. It's counterintuitive to return success but it makes sense if you thing about it. It simply returns a blank screen.
- // To illustrate, only few payloads, onLoad related, are handled because it works everytime.
- // It could be improved by checking for all payloads.
- // As listed at https://portswigger.net/web-security/cross-site-scripting/cheat-sheet, at 2020-11-11 there are 8979 of them.
- // So a way could be to read all of them and test...
- // @formatter:on
-
- if (contentId.toLowerCase().contains("<svg")
- || contentId.toLowerCase().contains("<body")
- || contentId.toLowerCase().contains("<iframe")
- || contentId.toLowerCase().contains("<object")
- || contentId.toLowerCase().contains("<embed")
- || contentId.toLowerCase().contains("<a href='javas")
- || contentId.toLowerCase().contains("<a href=\"javas")
- || contentId.toLowerCase().contains("<script")) {
+ // For OFBIZ-11840, checks if a webshell is not uploaded
+ // It's counterintuitive to return success but it makes sense if you thing about it.
+ // It simply returns a blank screen.
+ try {
+ if (!SecuredUpload.isValidText(contentId, Collections.emptyList())) {
+ Debug.logError("================== Not saved for security reason ==================", MODULE);
+ return "success";
+ }
+ } catch (IOException e) {
+ Debug.logError("================== Not saved for security reason ==================", MODULE);
return "success";
}
diff --git a/framework/security/config/security.properties b/framework/security/config/security.properties
index d41def2..3d5d59e 100644
--- a/framework/security/config/security.properties
+++ b/framework/security/config/security.properties
@@ -243,15 +243,22 @@ deniedFileExtensions=html,htm,php,php1,php2,hph3,php4,php5,php6,php7,phps,asp,as
#-- people may like to allow more than what is allowed OOTB
#-- As it name says, allowAllUploads opens all possibilities
allowAllUploads=
+
#--
-#-- List of denied tokens often part of webshells. Note that, for now at least, those are supposed to be used on a *nix system
+#-- List of denied tokens often part of webshells. Note that, for now at least, most are supposed to be used on a *nix system
#-- TODO.... to be continued with known webshell contents... a complete allow list is impossible anyway...
+#--
+#-- It could notably be improved by checking for all Javascripts payloads.
+#-- As listed at https://portswigger.net/web-security/cross-site-scripting/cheat-sheet,
+#-- at 2022-02-25 there are 8929 of them considering all tags, all events and all browsers...!
+#--
#-- "freemarker" should be OK, should not be used in Freemarker templates, not part of the syntax.
#-- Else "template.utility.Execute" is a good replacement but not as much catching, who knows...
+#--
#-- If you are sure you are safe for a token you can remove it, etc.
-deniedWebShellTokens=java.,beans,freemarker,<script,javascript,<body,<form,<jsp:,<c:out,taglib,<prefix,<%@ page,<?php,exec(,\
+deniedWebShellTokens=java.,beans,freemarker,<script,javascript,<body,<form,<jsp:,<c:out,taglib,<prefix,<%@ page,<?php,exec(,alert(,\
%eval,@eval,eval(,runtime,import,passthru,shell_exec,assert,str_rot13,system,decode,include,page ,\
- chmod,mkdir,fopen,fclose,new file,upload,getfilename,download,getoutputstring,readfile,\
+ chmod,mkdir,fopen,fclose,new file,upload,getfilename,download,getoutputstring,readfile,iframe,object,embed,<svg ,\
python,perl ,/perl,ruby ,/ruby,process,function,class,InputStream,to_server,wget ,static,\
ifconfig,route,crontab,netstat,uname ,hostname,iptables,whoami,"cmd",*cmd|,+cmd|,=cmd|,localhost
diff --git a/framework/security/src/test/java/org/apache/ofbiz/security/SecurityUtilTest.java b/framework/security/src/test/java/org/apache/ofbiz/security/SecurityUtilTest.java
index 0afa0f6..3f76ace 100644
--- a/framework/security/src/test/java/org/apache/ofbiz/security/SecurityUtilTest.java
+++ b/framework/security/src/test/java/org/apache/ofbiz/security/SecurityUtilTest.java
@@ -59,9 +59,9 @@ public class SecurityUtilTest {
@Test
public void webShellTokensTesting() {
// Currently used
- // java.,beans,freemarker,<script,javascript,<body,<form,<jsp:,<c:out,taglib,<prefix,<%@ page,<?php,exec(,\
+ // java.,beans,freemarker,<script,javascript,<body,<form,<jsp:,<c:out,taglib,<prefix,<%@ page,<?php,exec(,alert(,\
// %eval,@eval,eval(,runtime,import,passthru,shell_exec,assert,str_rot13,system,decode,include,page ,\
- // chmod,mkdir,fopen,fclose,new file,upload,getfilename,download,getoutputstring,readfile,\
+ // chmod,mkdir,fopen,fclose,new file,upload,getfilename,download,getoutputstring,readfile,iframe,object,embed,<svg ,\
// python,perl ,/perl,ruby ,/ruby,process,function,class,InputStream,to_server,wget ,static,\
// ifconfig,route,crontab,netstat,uname,hostname,iptables,whoami,"cmd",*cmd|,+cmd|,=cmd|,localhost
[ofbiz-framework] 02/02: Fixed: Stored XSS in webappPath parameter from content/control/EditWebSite (OFBIZ-12584)
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 2752e4e16b45c08b5cd72ba0018eaf4bf1fccada
Author: Jacques Le Roux <ja...@les7arts.com>
AuthorDate: Sun Feb 27 13:35:48 2022 +0100
Fixed: Stored XSS in webappPath parameter from content/control/EditWebSite (OFBIZ-12584)
A user with rights to modify and/or create websites may insert malicious HTML
elements in the “webappPath” parameter from content/control/EditWebSite
resulting in XSS.
In order to trigger the XSS a victim needs to navigate to main page of the
modified website (eg webpos or ecommerce) and interact with the malicious
HTML elements (eg trigger the “onmouseover” event by navigating with the mouse
over the “form” and/or “a” tags).
Thanks to Matei "Mal" Badanoiu for reporting this post-auth vulnerabily
---
.../ofbiz/service/engine/EntityAutoEngine.java | 23 ++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/framework/service/src/main/java/org/apache/ofbiz/service/engine/EntityAutoEngine.java b/framework/service/src/main/java/org/apache/ofbiz/service/engine/EntityAutoEngine.java
index 19736b7..1445a49 100644
--- a/framework/service/src/main/java/org/apache/ofbiz/service/engine/EntityAutoEngine.java
+++ b/framework/service/src/main/java/org/apache/ofbiz/service/engine/EntityAutoEngine.java
@@ -18,6 +18,8 @@
*/
package org.apache.ofbiz.service.engine;
+import java.io.IOException;
+import java.util.Collections;
import java.util.HashMap;
import java.util.Iterator;
import java.util.LinkedList;
@@ -37,6 +39,7 @@ import org.apache.ofbiz.entity.model.ModelEntity;
import org.apache.ofbiz.entity.model.ModelField;
import org.apache.ofbiz.entity.model.ModelUtil;
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.ModelParam;
@@ -71,6 +74,9 @@ public final class EntityAutoEngine extends GenericAsyncEngine {
@Override
public Map<String, Object> runSync(String localName, ModelService modelService, Map<String, Object> parameters) throws GenericServiceException {
// static java service methods should be: public Map<String, Object> methodName(DispatchContext dctx, Map<String, Object> context)
+ if (!isValidText(parameters)) {
+ return ServiceUtil.returnError("Not saved for security reason!");
+ }
DispatchContext dctx = getDispatcher().getLocalContext(localName);
Locale locale = (Locale) parameters.get("locale");
Map<String, Object> result = ServiceUtil.returnSuccess();
@@ -614,4 +620,21 @@ public final class EntityAutoEngine extends GenericAsyncEngine {
UtilMisc.toMap("label", modelEntity.getTitle()), locale));
return result;
}
+
+ private static boolean isValidText(Map<String, Object> parameters) {
+ // TODO maybe more parameters will be needed in future...
+ String parameter = (String) parameters.get("webappPath");
+ if (parameter != null) {
+ try {
+ if (!SecuredUpload.isValidText(parameter, Collections.emptyList())) {
+ Debug.logError("================== Not saved for security reason ==================", MODULE);
+ return false;
+ }
+ } catch (IOException e) {
+ Debug.logError("================== Not saved for security reason ==================", MODULE);
+ return false;
+ }
+ }
+ return true;
+ }
}