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
The following commit(s) were added to refs/heads/trunk by this push:
new 30770e1 Fixed: Remote Code Execution (File Upload) Vulnerability
(OFBIZ-11948)
30770e1 is described below
commit 30770e1ceaa81198f3ba56a9dbc0dfb727a84d7a
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));
