[ofbiz-framework] 01/02: Fixed: Secure the uploads (OFBIZ-12080)

[jleroux]

This is an automated email from the ASF dual-hosted git repository.

jleroux pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git

commit 0380ab85fd62eb727e12a98b8d8ec7025e4aaddf
Author: Jacques Le Roux <jacques.le.r...@les7arts.com>
AuthorDate: Thu Mar 3 17:22:50 2022 +0100

    Fixed: Secure the uploads (OFBIZ-12080)
    
    Removes non satisfying last line (because images may contain those strings) 
in
    security.properties and SecurityUtilTest.java
    
    In SecuredUpload, renames deniedFileExtensions and deniedWebShellTokens to
    respectively getDeniedFileExtensions and getDeniedWebShellTokens
---
 framework/security/config/security.properties                    | 2 --
 .../src/main/java/org/apache/ofbiz/security/SecuredUpload.java   | 8 ++++----
 .../test/java/org/apache/ofbiz/security/SecurityUtilTest.java    | 9 +--------
 3 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/framework/security/config/security.properties 
b/framework/security/config/security.properties
index e827a23..039cd5a 100644
--- a/framework/security/config/security.properties
+++ b/framework/security/config/security.properties
@@ -261,8 +261,6 @@ 
deniedWebShellTokens=java.,beans,freemarker,<script,javascript,<body,<form,<jsp:
                      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,\
                      ifconfig,route,crontab,netstat,uname 
,hostname,iptables,whoami,"cmd",*cmd|,+cmd|,=cmd|,localhost,\
-                     ",","+",',','+'
-#-- Last line is a non satisfying (because images may contain those strings) 
temporary solution before looking at Freemarker::WhitelistMemberAccessPolicy
 
 #-- 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
index 8fbe8e5..fbf2a6a 100644
--- 
a/framework/security/src/main/java/org/apache/ofbiz/security/SecuredUpload.java
+++ 
b/framework/security/src/main/java/org/apache/ofbiz/security/SecuredUpload.java
@@ -99,8 +99,8 @@ public class SecuredUpload {
     // Supported file formats are *safe* PNG, GIF, TIFF, JPEG, PDF, Audio, 
Video, Text, and ZIP
 
     private static final String MODULE = SecuredUpload.class.getName();
-    private static final List<String> DENIEDFILEEXTENSIONS = 
deniedFileExtensions();
-    private static final List<String> DENIEDWEBSHELLTOKENS = 
deniedWebShellTokens();
+    private static final List<String> DENIEDFILEEXTENSIONS = 
getDeniedFileExtensions();
+    private static final List<String> DENIEDWEBSHELLTOKENS = 
getDeniedWebShellTokens();
     private static final Integer MAXLINELENGTH = 
UtilProperties.getPropertyAsInteger("security", "maxLineLength", 10000);
 
     public static boolean isValidText(String content, List<String> allowed) 
throws IOException {
@@ -688,12 +688,12 @@ public class SecuredUpload {
         }
     }
 
-    private static List<String> deniedFileExtensions() {
+    private static List<String> getDeniedFileExtensions() {
         String deniedExtensions = UtilProperties.getPropertyValue("security", 
"deniedFileExtensions");
         return UtilValidate.isNotEmpty(deniedExtensions) ? 
StringUtil.split(deniedExtensions, ",") : new ArrayList<>();
     }
 
-    private static List<String> deniedWebShellTokens() {
+    private static List<String> getDeniedWebShellTokens() {
         String deniedTokens = UtilProperties.getPropertyValue("security", 
"deniedWebShellTokens");
         return UtilValidate.isNotEmpty(deniedTokens) ? 
StringUtil.split(deniedTokens, ",") : new ArrayList<>();
     }
diff --git 
a/framework/security/src/test/java/org/apache/ofbiz/security/SecurityUtilTest.java
 
b/framework/security/src/test/java/org/apache/ofbiz/security/SecurityUtilTest.java
index 9e5ee7b..698d1c8 100644
--- 
a/framework/security/src/test/java/org/apache/ofbiz/security/SecurityUtilTest.java
+++ 
b/framework/security/src/test/java/org/apache/ofbiz/security/SecurityUtilTest.java
@@ -63,10 +63,7 @@ public class SecurityUtilTest {
         // 
%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,\
-        // 
ifconfig,route,crontab,netstat,uname,hostname,iptables,whoami,"cmd",*cmd|,+cmd|,=cmd|,localhost
-        // ",","+",',','+'
-        // Last line is a non satisfying (because images may contain those 
strings) temporary solution before looking at
-        // Freemarker::WhitelistMemberAccessPolicy
+        // ifconfig,route,crontab,netstat,uname 
,hostname,iptables,whoami,"cmd",*cmd|,+cmd|,=cmd|,localhost,\
 
         try {
             List<String> allowed = new ArrayList<>();
@@ -144,10 +141,6 @@ public class SecurityUtilTest {
             assertFalse(SecuredUpload.isValidText("=cmd|", allowed));
             assertFalse(SecuredUpload.isValidText("localhost", allowed));
 
-            assertFalse(SecuredUpload.isValidText("\",\"", allowed));
-            assertFalse(SecuredUpload.isValidText("\"+\"", allowed));
-            assertFalse(SecuredUpload.isValidText("','", allowed));
-            assertFalse(SecuredUpload.isValidText("'+'", allowed));
         } catch (IOException e) {
             fail(String.format("IOException occured : %s", e.getMessage()));
         }