Improved: Prevent URL parameters manipulation (OFBIZ-13147)

Improves SecuredUpload.java in order to not accept Base64 encoded query strings even if "base64" is already in deniedWebShellTokens, better guarantee. Thanks to Nicolas. Just an indentation in ControlFilter. Add tcp and 4444 (for udp) in deniedWebShellTokens to stop reverse shells. Conflicts handled by hand framework/security/config/security.properties framework/security/src/main/java/org/apache/ofbiz/security/SecuredUpload.java framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ControlFilter.java
apache · Oct 24, 2024 · 721e2e8 · 721e2e8
1 parent 94e40f9
commit 721e2e8
Show file tree

Hide file tree

Showing 3 changed files with 43 additions and 4 deletions.
diff --git a/framework/security/config/security.properties b/framework/security/config/security.properties
@@ -240,7 +240,9 @@ deniedWebShellTokens=java.,beans,freemarker,<script,javascript,<body,body ,<form
                      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,calculate,curl,base64
+                     execute,println,calc,touch,curl,base64,tcp,4444
+
+allowStringConcatenationInUploadedFiles=false
 
 #-- Max line length for uploaded files, by default 10000
 maxLineLength=

diff --git a/framework/security/src/main/java/org/apache/ofbiz/security/SecuredUpload.java b/framework/security/src/main/java/org/apache/ofbiz/security/SecuredUpload.java
@@ -37,6 +37,7 @@
 import java.nio.file.Paths;
 import java.nio.file.StandardOpenOption;
 import java.util.ArrayList;
+import java.util.Base64;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.HashSet;
@@ -107,9 +108,45 @@ public class SecuredUpload {
     private static final List<String> DENIEDFILEEXTENSIONS = getDeniedFileExtensions();
     private static final List<String> DENIEDWEBSHELLTOKENS = getDeniedWebShellTokens();
     private static final Integer MAXLINELENGTH = UtilProperties.getPropertyAsInteger("security", "maxLineLength", 10000);
+    private static final Boolean ALLOWSTRINGCONCATENATIONINUPLOADEDFILES =
+            UtilProperties.getPropertyAsBoolean("security", "allowStringConcatenationInUploadedFiles", false);
+
+    /**
+     * For the content analyze if it's a base64 string with potential code injection
+     * @param content
+     * @param allowed
+     * @return
+     * @throws IOException
+     */
+    public static boolean isValidEncodedText(String content, List<String> allowed) throws IOException {
+        try {
+            return !isValidText(Base64.getDecoder().decode(content).toString(), allowed, false)
+                    || !isValidText(Base64.getMimeDecoder().decode(content).toString(), allowed, false)
+                    || !isValidText(Base64.getUrlDecoder().decode(content).toString(), allowed, false);
+        } catch (IllegalArgumentException e) {
+            // the encoded text isn't a Base64, allow it because there is no security risk
+            return true;
+        }
+    }
 
     public static boolean isValidText(String content, List<String> allowed) throws IOException {
-        return content != null ? DENIEDWEBSHELLTOKENS.stream().allMatch(token -> isValid(content, token.toLowerCase(), allowed)) : false;
+        return isValidText(content, allowed, true);
+    }
+
+    public static boolean isValidText(String content, List<String> allowed, boolean testEncodeContent) throws IOException {
+        if (content == null) {
+            return false;
+        }
+        String contentWithoutSpaces = content.replaceAll("\\s", "");
+        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;
+        }
+        if (testEncodeContent && !isValidEncodedText(content, allowed)) {
+            return false;
+        }
+        return DENIEDWEBSHELLTOKENS.stream().allMatch(token -> isValid(content, token.toLowerCase(), allowed));
     }
 
     public static boolean isValidFileName(String fileToCheck, Delegator delegator) throws IOException {

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
@@ -145,8 +145,8 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
                 // wt=javabin allows Solr tests, see https://cwiki.apache.org/confluence/display/solr/javabin
                 if (UtilValidate.isUrl(queryString)
                         || !SecuredUpload.isValidText(queryString, Collections.emptyList())
-                                && !(queryString.contains("JavaScriptEnabled=Y")
-                                        || queryString.contains("wt=javabin"))) {
+                        && !(queryString.contains("JavaScriptEnabled=Y")
+                                || queryString.contains("wt=javabin"))) {
                     Debug.logError("For security reason this URL is not accepted", module);
                     throw new RuntimeException("For security reason this URL is not accepted");
                 }