This is an automated email from the ASF dual-hosted git repository.
pgil 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 c0f8783 Improved: Create a deny list to reject webshell tokens
(OFBIZ-12324)
c0f8783 is described below
commit c0f8783d8d5d4fe90f67c8c82da3950daafc4a25
Author: Gil Portenseigne <p...@apache.org>
AuthorDate: Wed Sep 22 15:03:30 2021 +0200
Improved: Create a deny list to reject webshell tokens (OFBIZ-12324)
Improve readability using stream api, while adding unit test for
refactoring validation.
Fix deniedWebShellTokens property where some blank space where removed
and '// python' comment was inlined.
---
framework/security/config/security.properties | 4 +-
.../org/apache/ofbiz/security/SecuredUpload.java | 26 ++-----------
.../apache/ofbiz/security/SecurityUtilTest.java | 45 ++++++++++++++++++++++
3 files changed, 51 insertions(+), 24 deletions(-)
diff --git a/framework/security/config/security.properties
b/framework/security/config/security.properties
index 08433a4..5816f0c 100644
--- a/framework/security/config/security.properties
+++ b/framework/security/config/security.properties
@@ -238,7 +238,9 @@ allowAllUploads=
#-- 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=freemarker,import=\"java,runtime.getruntime().exec(,<%@page,<script,<body>,<form,php,javascript,%eval,@eval,importos//Python,passthru,exec,shell_exec,assert,str_rot13,system,phpinfo,base64_decode,chmod,mkdir,fopen,fclose,newfile,import,upload,getfilename,download,getoutputstring,readfile
+deniedWebShellTokens=freemarker,import=\"java,runtime.getruntime().exec(,<%@
page,<script,<body>,<form,php,\
+ javascript,%eval,@eval,import
os,passthru,exec,shell_exec,assert,str_rot13,system,phpinfo,base64_decode,chmod,mkdir,\
+ fopen,fclose,new
file,import,upload,getfilename,download,getoutputstring,readfile
#-- 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 6741975..05f784d 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
@@ -39,7 +39,6 @@ import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
-import java.util.LinkedList;
import java.util.List;
import java.util.Set;
import java.util.UUID;
@@ -616,12 +615,7 @@ public class SecuredUpload {
}
public static boolean isValidText(String content, List<String> allowed)
throws IOException {
- for (String token : DENIEDWEBSHELLTOKENS) {
- if (!isValid(content, token, allowed)) {
- return false;
- }
- }
- return true;
+ return DENIEDWEBSHELLTOKENS.stream().allMatch(token ->
isValid(content, token, allowed));
}
private static boolean isValid(String content, String string, List<String>
allowed) {
@@ -637,26 +631,12 @@ public class SecuredUpload {
}
private static List<String> deniedFileExtensions() {
- List<String> deniedFileExtensions = new LinkedList<>();
String deniedExtensions = UtilProperties.getPropertyValue("security",
"deniedFileExtensions");
- if (UtilValidate.isNotEmpty(deniedExtensions)) {
- List<String> deniedFileExtensionsList =
StringUtil.split(deniedExtensions, ",");
- for (String deniedExtension : deniedFileExtensionsList) {
- deniedFileExtensions.add(deniedExtension);
- }
- }
- return deniedFileExtensions;
+ return UtilValidate.isNotEmpty(deniedExtensions) ?
StringUtil.split(deniedExtensions, ",") : new ArrayList<>();
}
private static List<String> deniedWebShellTokens() {
- List<String> deniedWebShellTokens = new LinkedList<>();
String deniedTokens = UtilProperties.getPropertyValue("security",
"deniedWebShellTokens");
- if (UtilValidate.isNotEmpty(deniedTokens)) {
- List<String> deniedWebShellTokensList =
StringUtil.split(deniedTokens, ",");
- for (String deniedToken : deniedWebShellTokensList) {
- deniedWebShellTokens.add(deniedToken);
- }
- }
- return 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 47b8bb6..4a11e91 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
@@ -18,6 +18,8 @@
*/
package org.apache.ofbiz.security;
+import java.io.IOException;
+import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
@@ -25,6 +27,7 @@ import org.junit.Test;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.fail;
public class SecurityUtilTest {
@Test
@@ -52,4 +55,46 @@ public class SecurityUtilTest {
adminPermissions, "SPECIFIC_MULTI_LEVEL_EXAMPLE_VIEW"));
assertFalse(SecurityUtil.checkMultiLevelAdminPermissionValidity(adminPermissions,
"HOTDEP_PARTYMGR_ADMIN"));
}
+
+ @Test
+ public void webShellTokensTesting() {
+ try {
+ assertTrue(SecuredUpload.isValidText("hack.getFileName",
List.of("getfilename")));
+ List<String> allowed = new ArrayList<>();
+ assertFalse(SecuredUpload.isValidText("hack.getFileName",
allowed));
+ assertFalse(SecuredUpload.isValidText("freemarker", allowed));
+ assertFalse(SecuredUpload.isValidText("import=\"java", allowed));
+
assertFalse(SecuredUpload.isValidText("runtime.getruntime().exec(", allowed));
+ assertFalse(SecuredUpload.isValidText("<%@ page", allowed));
+ assertFalse(SecuredUpload.isValidText("<script", allowed));
+ assertFalse(SecuredUpload.isValidText("<body>", allowed));
+ assertFalse(SecuredUpload.isValidText("<form", allowed));
+ assertFalse(SecuredUpload.isValidText("php", allowed));
+ assertFalse(SecuredUpload.isValidText("javascript", allowed));
+ assertFalse(SecuredUpload.isValidText("%eval", allowed));
+ assertFalse(SecuredUpload.isValidText("@eval", 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("phpinfo", allowed));
+ assertFalse(SecuredUpload.isValidText("base64_decode", allowed));
+ assertFalse(SecuredUpload.isValidText("chmod", allowed));
+ assertFalse(SecuredUpload.isValidText("mkdir", allowed));
+ assertFalse(SecuredUpload.isValidText("fopen", allowed));
+ assertFalse(SecuredUpload.isValidText("fclose", allowed));
+ assertFalse(SecuredUpload.isValidText("new file", allowed));
+ assertFalse(SecuredUpload.isValidText("import", allowed));
+ assertFalse(SecuredUpload.isValidText("upload", allowed));
+ assertFalse(SecuredUpload.isValidText("getfilename", allowed));
+ assertFalse(SecuredUpload.isValidText("download", allowed));
+ assertFalse(SecuredUpload.isValidText("getoutputstring", allowed));
+ assertFalse(SecuredUpload.isValidText("readfile", allowed));
+ } catch (IOException e) {
+ fail(String.format("IOException occured : %s", e.getMessage()));
+ }
+ }
}
