[ofbiz-framework] 01/02: Improved: Reflected XSS in content component (OFBIZ-11840)

[jl...@apache.org]

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

commit 34d68d5f6f9037323f73f7780d2d3ecd6b030d9e
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
    
    Conflicts handled by hand in data/DataEvents.java
---
 .../java/org/apache/ofbiz/content/data/DataEvents.java   | 16 ++++++++++++----
 framework/security/config/security.properties            | 13 ++++++++++---
 2 files changed, 22 insertions(+), 7 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 1c81434..1c233ca 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;
@@ -82,10 +84,16 @@ public class DataEvents {
         // get the permission service required for streaming data; default is always the genericContentPermission
         String permissionService = EntityUtilProperties.getPropertyValue("content", "stream.permission.service", "genericContentPermission", delegator);
 
-        // This is counterintuitive but it works, for OFBIZ-11840
-        // It could be improved by checking for possible events associated with svg
-        // As listed at https://portswigger.net/web-security/cross-site-scripting/cheat-sheet
-        if (contentId.contains("<svg")) {
+        // 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 9614d7f..47f83e1 100644
--- a/framework/security/config/security.properties
+++ b/framework/security/config/security.properties
@@ -215,15 +215,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