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

framework/security/config/security.properties

@@ -277,13 +277,12 @@ csvformat=CSVFormat.DEFAULT
 #--
 #-- If you cross issues while loading an image file because of a token found there, you may try to surround the string by spaces, as " tr " below.
 #-- Actually most of the tokens should but it's now a bit late for me. I mean to test all of them...
-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 ,\
+deniedWebShellTokens=java,beans,freemarker,script,javascript,body,form,jsp,c:out,taglib,prefix,php,exec,alert,\
+                    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,\
-                    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
-
+                    python,ruby,process,function,class,InputStream,to_server,wget,static,assign,webappPath,\
+                    ifconfig,route,crontab,netstat,uname,hostname,iptables,whoami,cmd,localhost,thread,require,gzdeflate,\
+                    execute,println,calc,touch,curl,base64,tcp,4444,base32,tr,xxd,bash,od,printf,echo
 
 #-- SHA-1 versions of tokens containing (as String) at least one deniedWebShellTokens
 #-- This is notably used to allow special values in query parameters.

framework/security/src/main/java/org/apache/ofbiz/security/SecuredUpload.java

@@ -47,6 +47,8 @@
 import java.util.Set;
 import java.util.UUID;
 
+import java.util.regex.Pattern;
+import java.util.stream.Collectors;
 import javax.imageio.ImageIO;
 import javax.imageio.ImageReader;
 import javax.imageio.stream.ImageInputStream;
@@ -110,7 +112,8 @@ public class SecuredUpload {
 
     private static final String MODULE = SecuredUpload.class.getName();
     private static final List<String> DENIEDFILEEXTENSIONS = getDeniedFileExtensions();
-    private static final List<String> DENIEDWEBSHELLTOKENS = getDeniedWebShellTokens();
+    private static final Pattern DENIEDWEBSHELLTOKENS = computeDeniedWebShellTokens();
+    private static final List<String> ALLOWEDWEBSHELLTOKENS = computeAllowedWebShellTokens();
     private static final Integer MAXLINELENGTH = UtilProperties.getPropertyAsInteger("security", "maxLineLength", 0);
     private static final Boolean ALLOWSTRINGCONCATENATIONINUPLOADEDFILES =
             UtilProperties.getPropertyAsBoolean("security", "allowStringConcatenationInUploadedFiles", false);
@@ -125,28 +128,31 @@ public class SecuredUpload {
     // check there is no web shell in the uploaded file
     // A file containing a reverse shell will be rejected.
     public static boolean isValidText(String content, List<String> allowed) throws IOException {
-        return isValidText(content, allowed, false);
-    }
-
-    public static boolean isValidText(String content, List<String> allowed, boolean isQuery) throws IOException {
         if (content == null) {
             return false;
         }
-        if (!isQuery) {
-            String contentWithoutSpaces = content.replaceAll(" ", "");
-            if ((contentWithoutSpaces.contains("\"+\"") || contentWithoutSpaces.contains("'+'"))
-                    && !ALLOWSTRINGCONCATENATIONINUPLOADEDFILES) {
-                Debug.logInfo("The uploaded file contains a string concatenation. It can't be uploaded for security reason", MODULE);
-                return false;
-            }
-        } else {
-            // Check the query string is safe, notably no reverse shell
-            List<String> queryParameters = StringUtil.split(content, "&");
-            return DENIEDWEBSHELLTOKENS.stream().allMatch(token -> isValid(queryParameters, token.toLowerCase(), allowed));
+        String contentWithoutSpaces = StringUtil.removeSpaces(content);
+        if ((contentWithoutSpaces.contains("\"+\"") || contentWithoutSpaces.contains("'+'"))
+                && !ALLOWSTRINGCONCATENATIONINUPLOADEDFILES) {
+            Debug.logInfo("The uploaded file contains a string concatenation. It can't be uploaded for security reason", MODULE);
+            return false;
         }
 
         // Check there is no web shell in an uploaded file
-        return DENIEDWEBSHELLTOKENS.stream().allMatch(token -> isValid(content.toLowerCase(), token.toLowerCase(), allowed));
+        return containsDeniedWebShellToken(List.of(content), allowed, getDeniedWebShellTokens());
+    }
+
+    public static boolean isValidQuery(String content, List<String> allowed) throws IOException {
+        if (content == null) {
+            return false;
+        }
+        // Check the query string is safe, notably no reverse shell
+        return !containsDeniedWebShellToken(StringUtil.split(content, "&"), allowed, getDeniedWebShellTokens());
+    }
+
+    public static boolean containsDeniedWebShellToken(List<String> contents, List<String> allowed, Pattern deniedWebShellTokens) {
+        return contents.stream().anyMatch(content -> deniedWebShellTokens.matcher(content.toLowerCase()).find()
+                && !isAllowed(content.toLowerCase(), allowed));
     }
 
     public static boolean isValidFileName(String fileToCheck, Delegator delegator) throws IOException {
@@ -245,7 +251,7 @@ public static boolean isValidFileName(String fileToCheck, Delegator delegator) t
      */
     public static boolean isValidFile(String fileToCheck, String fileType, Delegator delegator) throws IOException, ImageReadException {
         // Allow all
-        if (("true".equalsIgnoreCase(EntityUtilProperties.getPropertyValue("security", "allowAllUploads", delegator)))) {
+        if ("true".equalsIgnoreCase(EntityUtilProperties.getPropertyValue("security", "allowAllUploads", delegator))) {
             return true;
         }
 
@@ -850,32 +856,19 @@ private static boolean isValidTextFile(String fileName, Boolean encodedContent)
         return isValidText(content, allowed);
     }
 
-    // Check there is no web shell
-    private static boolean isValid(String content, String string, List<String> allowed) {
-        boolean isOK = !content.contains(string) || allowed.contains(string);
-        if (!isOK) {
-            Debug.logInfo("The uploaded file contains the string '" + string + "'. It can't be uploaded for security reason", MODULE);
+    private static boolean isAllowed(String content, List<String> allowed) {
+        List<String> allowedContents = new ArrayList<>(ALLOWEDWEBSHELLTOKENS);
+        if (allowed != null) {
+            allowedContents.addAll(allowed);
         }
-        return isOK;
-    }
-
-    // Check there is no reverse shell in query string
-    private static boolean isValid(List<String> queryParameters, String string, List<String> allowed) {
-        boolean isOK = true;
-
-        for (String parameter : queryParameters) {
-            if (!parameter.contains(string)
-                    || allowed.contains(HashCrypt.cryptBytes("SHA", "OFBiz", parameter.toLowerCase().getBytes(StandardCharsets.UTF_8)))) {
-                continue;
-            } else {
-                isOK = false;
-                break;
+        for (String allowContent : allowedContents) {
+            boolean allowedHashedTokenIdentified = allowContent.startsWith("$SHA")
+                    && allowContent.equals(HashCrypt.cryptBytes("SHA", "OFBiz", content.toLowerCase().getBytes(StandardCharsets.UTF_8)));
+            if (allowedHashedTokenIdentified || content.trim().toLowerCase().contains(allowContent)) {
+                return true;
             }
         }
-        if (!isOK) {
-            Debug.logInfo("The HTTP query string contains the string '" + string + "'. It can't be uploaded for security reason", MODULE);
-        }
-        return isOK;
+        return false;
     }
 
     private static void deleteBadFile(String fileToCheck) {
@@ -891,9 +884,36 @@ private static List<String> getDeniedFileExtensions() {
         return UtilValidate.isNotEmpty(deniedExtensions) ? StringUtil.split(deniedExtensions, ",") : new ArrayList<>();
     }
 
-    private static List<String> getDeniedWebShellTokens() {
-        String deniedTokens = UtilProperties.getPropertyValue("security", "deniedWebShellTokens");
-        return UtilValidate.isNotEmpty(deniedTokens) ? StringUtil.split(deniedTokens, ",") : new ArrayList<>();
+    public static Pattern getDeniedWebShellTokens() {
+        return DENIEDWEBSHELLTOKENS;
+    }
+
+    private static Pattern computeDeniedWebShellTokens() {
+        return computeDeniedWebShellTokensPattern(
+                StringUtil.split(UtilProperties.getPropertyValue("security", "deniedWebShellTokens").toLowerCase(), ","));
+    }
+
+    /**
+     * Build Pattern from list of token, based on regexp that complies to
+     * 1 - any non word suffix : ^[a-zA-Z0-9_]
+     * 2 - any non word prefix : ^[a-zA-Z0-9_]
+     *   OR any prefix with '%' followed by 2 to 5 chars. (for percent-encoding char in query, ie. %21 for !)
+     * @param tokens to support detection rule
+     * @return built pattern
+     */
+    public static Pattern computeDeniedWebShellTokensPattern(List<String> tokens) {
+        return Pattern.compile(tokens.stream()
+                .map(token -> "(%.{2,5}|[^\\w])" + token + "[^\\w]")
+                .collect(Collectors.joining("|")));
+    }
+
+    public static List<String> getAllowedTokens() {
+        return ALLOWEDWEBSHELLTOKENS;
+    }
+
+    private static List<String> computeAllowedWebShellTokens() {
+        String allowedTokens = UtilProperties.getPropertyValue("security", "allowedTokens");
+        return UtilValidate.isNotEmpty(allowedTokens) ? StringUtil.split(allowedTokens, ",") : List.of();
     }
 
     private static boolean checkMaxLinesLength(String fileToCheck) {

framework/security/src/test/java/org/apache/ofbiz/security/SecurityUtilTest.java

@@ -18,6 +18,8 @@
  */
 package org.apache.ofbiz.security;
 
+import java.util.regex.Pattern;
+import org.apache.ofbiz.base.util.UtilCodec;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
@@ -27,6 +29,7 @@
 import org.junit.Test;
 
 public class SecurityUtilTest {
+
     @Test
     public void basicAdminPermissionTesting() {
         List<String> adminPermissions = Arrays.asList("PARTYMGR", "EXAMPLE", "ACCTG_PREF");
@@ -53,4 +56,45 @@ public void multiLevelBadHierarchyPermissionTesting() {
         assertFalse(SecurityUtil.checkMultiLevelAdminPermissionValidity(adminPermissions, "HOTDEP_PARTYMGR_ADMIN"));
     }
 
+    @Test
+    public void isDeniedWebShell() {
+        Pattern testDeniedWebShellTokens = SecuredUpload.computeDeniedWebShellTokensPattern(List.of("suspecttoken", "allowedtoken"));
+        List.of(" suspecttoken.",
+                        " suspectToken.",
+                        " SuspectToken.",
+                        " SUSPECTTOKEN.",
+                        " suspectToken.",
+                        "<suspectToken ",
+                        "<suspectToken:",
+                        "< suspectToken :",
+                        "<%@ suspectToken ",
+                        "<?suspectToken ",
+                        " suspectToken(",
+                        "%suspectToken,",
+                        "/suspectToken*",
+                        "\"suspectToken\"",
+                        "*suspectToken|",
+                        "+suspectToken|",
+                        "=suspectToken|",
+                        "|suspectToken ")
+                .forEach(token -> {
+                    String encodedToken = UtilCodec.getEncoder("url").encode(token);
+                    assertTrue("failed to stop '" + token + "' encoded as '" + encodedToken + " with " + testDeniedWebShellTokens,
+                            SecuredUpload.containsDeniedWebShellToken(List.of(encodedToken), List.of(), testDeniedWebShellTokens));
+                });
+    }
+
+    @Test
+    public void isAllowedWebShell() {
+        Pattern testDeniedWebShellTokens = SecuredUpload.computeDeniedWebShellTokensPattern(List.of("suspecttoken", "allowedtoken"));
+        List.of(" suspecttokena ",
+                        "suspectToken",
+                        " allowedToken(",
+                        " allowedtoken(")
+                .forEach(token -> {
+                    String encodedToken = UtilCodec.getEncoder("url").encode(token);
+                    assertFalse("failed to allow '" + token + "' encoded as '" + encodedToken + " with " + testDeniedWebShellTokens,
+                            SecuredUpload.containsDeniedWebShellToken(List.of(encodedToken), List.of("allowedtoken"), testDeniedWebShellTokens));
+                });
+    }
 }

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

@@ -90,8 +90,6 @@ public class ControlFilter extends HttpFilter {
     private int errorCode;
     /** The list of all path prefixes that are allowed. */
     private Set<String> allowedPaths;
-    private static final List<String> ALLOWEDTOKENS = getAllowedTokens();
-
 
     @Override
     public void init(FilterConfig conf) throws ServletException {
@@ -151,11 +149,6 @@ private void redirect(HttpServletResponse resp, String contextPath) throws IOExc
         resp.sendRedirect(redirectPathIsUrl ? redirectPath : (contextPath + redirectPath));
     }
 
-    private static List<String> getAllowedTokens() {
-        String allowedTokens = UtilProperties.getPropertyValue("security", "allowedTokens");
-        return UtilValidate.isNotEmpty(allowedTokens) ? StringUtil.split(allowedTokens, ",") : new ArrayList<>();
-    }
-
     /**
      * Makes allowed paths pass through while redirecting the others to a fix location.
      * Reject wrong URLs
@@ -202,7 +195,8 @@ public void doFilter(HttpServletRequest req, HttpServletResponse resp, FilterCha
             if (queryString != null) {
                 queryString = URLDecoder.decode(queryString, "UTF-8");
                 if (UtilValidate.isUrl(queryString)
-                        || !SecuredUpload.isValidText(queryString.toLowerCase(), ALLOWEDTOKENS, true)
+                        //TODO : Solr dependency in framework, how to avoid, through webapp def ?
+                        || !SecuredUpload.isValidQuery(queryString, List.of())
                                 && isSolrTest()) {
                     Debug.logError("For security reason this URL is not accepted", MODULE);
                     throw new RuntimeException("For security reason this URL is not accepted");