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.

framework/security/config/security.properties

@@ -278,7 +278,7 @@ 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,curl,base64
+                     execute,println,calc,touch,curl,base64,tcp,4444
 
 allowStringConcatenationInUploadedFiles=false
 

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

@@ -38,6 +38,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.HashSet;
 import java.util.Iterator;
@@ -119,14 +120,42 @@ public class SecuredUpload {
     private static final String FILENAMEVALIDCHARACTERS =
             UtilProperties.getPropertyValue("security", "fileNameValidCharacters", "[a-zA-Z0-9-_ ]");
 
+    /**
+     * 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 {
-        String contentWithoutSpaces = content.replace(" ", "");
+        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;
         }
-        return content != null ? DENIEDWEBSHELLTOKENS.stream().allMatch(token -> isValid(content, token.toLowerCase(), allowed)) : 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 {

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

@@ -64,7 +64,7 @@ public void webShellTokensTesting() {
                 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
+                execute,println,calc,touch,curl,base64,tcp
          */
         try {
             List<String> allowed = new ArrayList<>();
@@ -153,6 +153,8 @@ public void webShellTokensTesting() {
             assertFalse(SecuredUpload.isValidText("touch", allowed));
             assertFalse(SecuredUpload.isValidText("curl", allowed));
             assertFalse(SecuredUpload.isValidText("base64", allowed));
+            assertFalse(SecuredUpload.isValidText("tcp", allowed));
+            assertFalse(SecuredUpload.isValidText("4444", allowed));
         } catch (IOException e) {
             fail(String.format("IOException occured : %s", e.getMessage()));
         }

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

@@ -173,8 +173,8 @@ public void doFilter(HttpServletRequest req, HttpServletResponse resp, 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");
                 }