Improved: Check parameters passed in URLs (OFBIZ-13295)

Thanks to Nicolas, improves previous commits that did not handle multipart
element

Also while at it, adds a space after "process" in deniedWebShellTokens of
security.properties. I stumbled upon it after trying to upload a file created
by a Canon camera. Anyway the deniedWebShellTokens comment suggest that when
it's necessary.

Conflicts handled by hand

Author: JacquesLeRoux

framework/common/src/main/java/org/apache/ofbiz/common/UrlServletHelper.java

@@ -50,6 +50,9 @@ private UrlServletHelper() { }
     public static void setRequestAttributes(ServletRequest request, Delegator delegator, ServletContext servletContext) {
         HttpServletRequest httpRequest = (HttpServletRequest) request;
         // check if multi tenant is enabled
+        if (delegator == null) {
+            delegator = WebAppUtil.getDelegator(servletContext);
+        }
         boolean useMultitenant = EntityUtil.isMultiTenantEnabled();
         if (useMultitenant) {
             // get tenant delegator by domain name

framework/security/config/security.properties

@@ -280,7 +280,7 @@ csvformat=CSVFormat.DEFAULT
 deniedWebShellTokens=java.,beans,freemarker,<script,javascript,<body,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,iframe,object,embed,onload,build,\
-                    python,perl ,/perl,ruby ,/ruby,process,function,class,InputStream,to_server,wget ,static,assign,webappPath,\
+                    python,perl ,/perl,ruby ,/ruby,process ,function,class,InputStream,to_server,wget ,static,assign,webappPath,\
                     ifconfig,route,crontab,netstat,uname ,hostname,iptables,whoami,"cmd",*cmd|,+cmd|,=cmd|,localhost,thread,require(,gzdeflate,\
                     execute,println,calc,touch,curl,base64, tcp ,4444,base32, tr , xxd ,bash, od ,|od ,printf,echo
 

framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ControlFilter.java

@@ -24,8 +24,8 @@
 import java.net.URLDecoder;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
-import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -45,9 +45,11 @@
 import org.apache.logging.log4j.ThreadContext;
 import org.apache.ofbiz.base.util.Debug;
 import org.apache.ofbiz.base.util.StringUtil;
+import org.apache.ofbiz.base.util.UtilGenerics;
 import org.apache.ofbiz.base.util.UtilHttp;
 import org.apache.ofbiz.base.util.UtilProperties;
 import org.apache.ofbiz.base.util.UtilValidate;
+import org.apache.ofbiz.common.UrlServletHelper;
 import org.apache.ofbiz.entity.GenericValue;
 import org.apache.ofbiz.security.SecuredFreemarker;
 import org.apache.ofbiz.security.SecuredUpload;
@@ -169,35 +171,33 @@ public void doFilter(HttpServletRequest req, HttpServletResponse resp, FilterCha
 
         // Prevents stream exploitation
         if (!isSolrTest()) {
+            UrlServletHelper.setRequestAttributes(req, null, req.getServletContext());
             Map<String, Object> parameters = UtilHttp.getParameterMap(req);
             boolean reject = false;
             if (!parameters.isEmpty()) {
                 for (String key : parameters.keySet()) {
                     Object object = parameters.get(key);
-                    if (object.getClass().equals(String.class)) {
-                        String val = (String) object;
-                        if (val.contains("<")) {
+                    if (object.getClass().equals(String.class)
+                            || object instanceof Collection) {
+                        try {
+                            List<String> toCheck = object.getClass().equals(String.class)
+                                    ? List.of((String) object)
+                                            : UtilGenerics.checkCollection(object, String.class);
+                            reject = toCheck.stream()
+                                    .anyMatch(val -> val.contains("<"));
+                        } catch (IllegalArgumentException e) {
+                            Debug.logWarning(e, MODULE);
                             reject = true;
                         }
-                    } else {
-                        @SuppressWarnings("unchecked")
-                        LinkedList<String> vals = (LinkedList<String>) parameters.get(key);
-                        for (String aVal : vals) {
-                            if (aVal.contains("<")) {
-                                reject = true;
-                            }
-                        }
                     }
-                }
-                if (reject) {
-                    Debug.logError("For security reason this URL is not accepted", MODULE);
-                    throw new RuntimeException("For security reason this URL is not accepted");
+                    if (reject) {
+                        Debug.logError("For security reason this URL is not accepted", MODULE);
+                        throw new RuntimeException("For security reason this URL is not accepted");
+                    }
                 }
             }
         }
 
-
-
         // Check if we are told to redirect everything.
         if (redirectAll) {
             // little trick here so we don't loop on ourselves