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 <jacques.le.r...@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