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

JacquesLeRoux · JacquesLeRoux · commit 357da7f17e3d · 2025-10-10T20:39:38.000+02:00
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.
diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/util/UtilHttp.java b/framework/base/src/main/java/org/apache/ofbiz/base/util/UtilHttp.java
@@ -65,16 +65,12 @@
 import java.util.stream.Stream;
 
 import javax.net.ssl.SSLContext;
-import jakarta.servlet.http.HttpServletRequest;
-import jakarta.servlet.http.HttpServletResponse;
-import jakarta.servlet.http.HttpSession;
 
 import org.apache.commons.fileupload2.core.DiskFileItem;
 import org.apache.commons.fileupload2.core.DiskFileItemFactory;
 import org.apache.commons.fileupload2.core.FileItem;
 import org.apache.commons.fileupload2.core.FileUploadException;
 import org.apache.commons.fileupload2.jakarta.JakartaServletFileUpload;
-
 import org.apache.commons.lang.RandomStringUtils;
 import org.apache.http.NameValuePair;
 import org.apache.http.client.utils.URLEncodedUtils;
@@ -94,6 +90,10 @@
 import com.google.re2j.Matcher;
 import com.google.re2j.Pattern;
 
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletResponse;
+import jakarta.servlet.http.HttpSession;
+
 /**
  * HttpUtil - Misc HTTP Utility Functions
  */
@@ -230,7 +230,6 @@ public static Map<String, Object> getMultiPartParameterMap(HttpServletRequest re
         if (isMultiPart) {
             // create the progress listener and add it to the session
             String encoding = request.getCharacterEncoding();
-            Charset charset = Charset.forName(encoding);
             JakartaServletFileUpload<DiskFileItem, DiskFileItemFactory> upload = UtilHttp.getServletFileUpload(request);
             FileUploadProgressListener listener = new FileUploadProgressListener();
             upload.setProgressListener(listener);
diff --git a/framework/common/src/main/java/org/apache/ofbiz/common/UrlServletHelper.java b/framework/common/src/main/java/org/apache/ofbiz/common/UrlServletHelper.java
@@ -21,14 +21,6 @@
 import java.io.IOException;
 import java.util.List;
 
-import jakarta.servlet.RequestDispatcher;
-import jakarta.servlet.ServletContext;
-import jakarta.servlet.ServletException;
-import jakarta.servlet.ServletRequest;
-import jakarta.servlet.ServletResponse;
-import jakarta.servlet.http.HttpServletRequest;
-import jakarta.servlet.http.HttpServletResponse;
-
 import org.apache.ofbiz.base.util.Debug;
 import org.apache.ofbiz.base.util.StringUtil;
 import org.apache.ofbiz.base.util.UtilValidate;
@@ -41,6 +33,14 @@
 import org.apache.ofbiz.webapp.WebAppUtil;
 import org.apache.ofbiz.webapp.website.WebSiteWorker;
 
+import jakarta.servlet.RequestDispatcher;
+import jakarta.servlet.ServletContext;
+import jakarta.servlet.ServletException;
+import jakarta.servlet.ServletRequest;
+import jakarta.servlet.ServletResponse;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletResponse;
+
 public final class UrlServletHelper {
 
     private static final String MODULE = UrlServletHelper.class.getName();
@@ -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
diff --git a/framework/security/config/security.properties b/framework/security/config/security.properties
@@ -283,7 +283,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
 
diff --git a/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ControlFilter.java b/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;
@@ -37,9 +37,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;
@@ -164,24 +166,24 @@ public void doFilter(HttpServletRequest req, HttpServletResponse resp, FilterCha
         HttpSession session = req.getSession();
 
         // Prevents stream exploitation
+        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) {
@@ -190,8 +192,6 @@ public void doFilter(HttpServletRequest req, HttpServletResponse resp, FilterCha
             }
         }
 
-
-
         // Check if we are told to redirect everything.
         if (redirectAll) {
             // little trick here so we don't loop on ourselves
