This is an automated email from the ASF dual-hosted git repository.
jleroux pushed a commit to branch release22.01
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
The following commit(s) were added to refs/heads/release22.01 by this push:
new b447f4d Fixed: Remote Code Execution (File Upload) Vulnerability
(OFBIZ-11948)
b447f4d is described below
commit b447f4dd3ffd32f4c80e0c3a90e4f78830fd6b0d
Author: Jacques Le Roux <jacques.le.r...@les7arts.com>
AuthorDate: Sat Feb 5 10:13:44 2022 +0100
Fixed: Remote Code Execution (File Upload) Vulnerability (OFBIZ-11948)
In SecuredUpload::isValidImageFile I initially used isValidText() and
thought
that decoding would be better so finally used isValidTextFile() instead. But
then valid images files are not passing. So this replaces isValidTextFile by
isValidText there.
Also while at it removes few other PHP tokens, now useless (hopefully, I
have
still to check encoded and encrypted PHP webshells), from
security::deniedWebShellTokens. The less tokens we have the better the
whole is
legible.
Improves related comments.
Modifies SecurityUtilTest::webShellTokensTesting accordingly
---
framework/security/config/security.properties | 11 +++++------
.../main/java/org/apache/ofbiz/security/SecuredUpload.java | 2 +-
.../test/java/org/apache/ofbiz/security/SecurityUtilTest.java | 9 +--------
3 files changed, 7 insertions(+), 15 deletions(-)
diff --git a/framework/security/config/security.properties
b/framework/security/config/security.properties
index 4bb2751..0b25cff 100644
--- a/framework/security/config/security.properties
+++ b/framework/security/config/security.properties
@@ -233,15 +233,14 @@
deniedFileExtensions=html,htm,php,php2,hph3,php4,php5,asp,aspx,ascx,jsp,jspx,cfm
#-- As it name says, allowAllUploads opens all possibilities
allowAllUploads=
#--
-#-- List of denied tokens often part of webshells
+#-- List of denied tokens often part of webshells. Note that, for now at
least, those are supposed to be used on a *nix system
#-- TODO.... to be continued with known webshell contents... a complete allow
list is impossible anyway...
-#-- eg:
https://www.acunetix.com/blog/articles/detection-prevention-introduction-web-shells-part-5/
#-- "freemarker" should be OK, should not be used in Freemarker templates, not
part of the syntax.
#-- Else "template.utility.Execute" is a good replacement but not as much
catching, who knows...
-deniedWebShellTokens=<%,<jsp,<?,#!,freemarker,<script,javascript,%eval,@eval,<body>,<form,\
- import
os,passthru,exec,shell_exec,assert,str_rot13,system,base64_decode,chmod,mkdir,\
- fopen,fclose,new
file,import,upload,getfilename,download,getoutputstring,readfile
-#-- IMPORTANT: when you change things here you need to do accordingly in
SecurityUtilTest::webShellTokensTesting--
+#-- If you are sure you are safe for a token you can remove it, etc.
+deniedWebShellTokens=<%,<jsp,<?,#!,freemarker,<script,javascript,eval,<body>,<form,\
+ chmod,mkdir,fopen,fclose,new
file,import,upload,getfilename,download,getoutputstring,readfile
+#-- IMPORTANT: when you change things here you need to do accordingly in
SecurityUtilTest::webShellTokensTesting and run "gradlew test" --
#-- Popup last-visited time from database after user has logged in.
#-- So users can know of any unauthorised access to their accounts.
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 0f09496..60f99cc 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
@@ -245,7 +245,7 @@ public class SecuredUpload {
|| imageFormat.equals(ImageFormats.TIFF)
|| imageFormat.equals(ImageFormats.JPEG))
&& imageMadeSafe(fileName)
- && isValidTextFile(fileName);
+ && isValidText(fileName, 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 18727d3..fe4207a 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
@@ -76,14 +76,7 @@ public class SecurityUtilTest {
assertFalse(SecuredUpload.isValidText("@eval", allowed));
assertFalse(SecuredUpload.isValidText("<body>", allowed));
assertFalse(SecuredUpload.isValidText("<form", allowed));
- assertFalse(SecuredUpload.isValidText("import os", allowed));
- assertFalse(SecuredUpload.isValidText("passthru", allowed));
- assertFalse(SecuredUpload.isValidText("exec", allowed));
- assertFalse(SecuredUpload.isValidText("shell_exec", allowed));
- assertFalse(SecuredUpload.isValidText("assert", allowed));
- assertFalse(SecuredUpload.isValidText("str_rot13", allowed));
- assertFalse(SecuredUpload.isValidText("system", allowed));
- assertFalse(SecuredUpload.isValidText("base64_decode", allowed));
+ assertFalse(SecuredUpload.isValidText("import", allowed));
assertFalse(SecuredUpload.isValidText("chmod", allowed));
assertFalse(SecuredUpload.isValidText("mkdir", allowed));
assertFalse(SecuredUpload.isValidText("fopen", allowed));
