Improved: [SECURITY] (CVE-2024-36104) Path traversal leading to RCE (…

…OFBIZ-13092)

Using allowedTokens was a bad idea (when all you have is a (new!) hammer,
everything looks like a nail).
It came to my mind that I already used  StringEscapeUtils::unescapeHtml4
in several places and in this case it's the tool of choice.
Actually I thought about  it when beginning, but was unsure it would be enough.
Now it's clear in my mind and this replaces allowedTokens by simply
StringEscapeUtils::unescapeHtml4

So this also removes the recently added allowedTokens in security.properties.
They are now useless.

Also it's quite better because it handles all cases known or unknown.
Better keep allowedTokens list as short as possible.

Conflicts handled by hand in both files

framework/security/config/security.properties

@@ -250,7 +250,7 @@ deniedWebShellTokens=java.,beans,freemarker,<script,javascript,<body,body ,<form
 #-- SHA-1 versions of tokens containing (as String) at least one deniedWebShellTokens
 #-- This is notably used to allow special values in query parameters.
 #-- If you add a token beware that it does not content ",". It's the separator.
-allowedTokens=$SHA$OFBiz$2naHrANKTniFcgLJk4oXr3IRQ48,$SHA$OFBiz$evAu1vcT5d1tjVXFTeVXU-6aNz8,$SHA$OFBiz$ORZaKvS7a0ee4gZb9P5hHuHnEyE,$SHA$OFBiz$T5DBu6tPuZzDCfYNci_23SrUa3Q,$SHA$OFBiz$BXhGVix7t3kfHrhNB0z9I0H9_rQ
+allowedTokens=$SHA$OFBiz$2naHrANKTniFcgLJk4oXr3IRQ48
 
 allowStringConcatenationInUploadedFiles=false
 

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

@@ -22,7 +22,6 @@
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.net.URLDecoder;
-import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
 import java.util.HashSet;
 import java.util.List;
@@ -37,7 +36,7 @@
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
-import org.apache.ofbiz.base.crypto.HashCrypt;
+import org.apache.commons.text.StringEscapeUtils;
 import org.apache.ofbiz.base.util.Debug;
 import org.apache.ofbiz.base.util.StringUtil;
 import org.apache.ofbiz.base.util.UtilProperties;
@@ -118,6 +117,12 @@ private static boolean isSolrTest() {
         return !GenericValue.getStackTraceAsString().contains("ControlFilterTests")
                 && null == System.getProperty("SolrDispatchFilter");
     }
+
+    private static List<String> getAllowedTokens() {
+        String allowedTokens = UtilProperties.getPropertyValue("security", "allowedTokens");
+        return UtilValidate.isNotEmpty(allowedTokens) ? StringUtil.split(allowedTokens, ",") : new ArrayList<>();
+    }
+
     @Override
     public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException {
         HttpServletRequest httpRequest = (HttpServletRequest) request;
@@ -145,8 +150,7 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
         if (httpRequest.getAttribute(FORWARDED_FROM_SERVLET) == null && !allowedPaths.isEmpty()) {
             // check to make sure the requested url is allowed
             // get the request URI without the webapp mount point
-            String requestUri = URLDecoder.decode(httpRequest.getRequestURI().substring(httpRequest.getContextPath().length()), "UTF-8");
-
+            String requestUri = StringEscapeUtils.unescapeHtml4(URLDecoder.decode(httpRequest.getRequestURI().substring(httpRequest.getContextPath().length()), "UTF-8"));
 
             // Reject wrong URLs
             String queryString = null;
@@ -169,18 +173,13 @@ && isSolrTest()) {
             }
 
             if (!requestUri.matches("/control/logout;jsessionid=[A-Z0-9]{32}\\.jvm1")) {
-                boolean bypass = true;
-                if (queryString != null) {
-                    List<String> queryStringList = StringUtil.splitWithStringSeparator(queryString.toLowerCase(), "&amp;");
-                    bypass = isAnyAllowedToken(queryStringList, ALLOWEDTOKENS);
-                }
-                if (requestUri != null && !bypass) { // "null" allows tests with Mockito. ControlFilterTests sends null.
+                if (requestUri != null) { // "null" allows tests with Mockito because ControlFilterTests sends null.
                     try {
                         String url = new URI(requestUri)
                                 .normalize().toString()
                                 .replaceAll(";", "")
                                 .replaceAll("(?i)%2e", "");
-                        if (!((HttpServletRequest) request).getRequestURL().toString().equals(url)) {
+                        if (!requestUri.toString().equals(url)) {
                             Debug.logError("For security reason this URL is not accepted", module);
                             throw new RuntimeException("For security reason this URL is not accepted");
                         }
@@ -233,24 +232,4 @@ && isSolrTest()) {
     public void destroy() {
 
     }
-
-    private static List<String> getAllowedTokens() {
-        String allowedTokens = UtilProperties.getPropertyValue("security", "allowedTokens");
-        return UtilValidate.isNotEmpty(allowedTokens) ? StringUtil.split(allowedTokens, ",") : new ArrayList<>();
-    }
-
-    // Check there is any allowedToken in URL
-    private static boolean isAnyAllowedToken(List<String> queryParameters, List<String> allowed) {
-        boolean isOK = false;
-        for (String parameter : queryParameters) {
-            parameter = parameter.substring(0, parameter.indexOf("=") + 1);
-            if (allowed.contains(HashCrypt.cryptBytes("SHA", "OFBiz", parameter.getBytes(StandardCharsets.UTF_8)))) {
-                isOK = true;
-                break;
-            } else {
-                continue;
-            }
-        }
-        return isOK;
-    }
 }