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 34584ea2df Improved: Prevent URL parameters manipulation (OFBIZ-13147)
34584ea2df is described below
commit 34584ea2dfabb78750e5b31b84b86edf0009e489
Author: Jacques Le Roux <jacques.le.r...@les7arts.com>
AuthorDate: Wed Nov 27 08:49:20 2024 +0100
Improved: Prevent URL parameters manipulation (OFBIZ-13147)
We don't need hashed deniedWebShellTokens, only hashed allowedTokens, get
back
to simple text.
Simplifies SecuredUpload:
No need for Base64 handling, using deniedWebShellTokens is enough.
Improves performance, remove duplicated calls inside loops or
stream().allMatch
Improves comments.
---
framework/security/config/security.properties | 9 +++-
.../org/apache/ofbiz/security/SecuredUpload.java | 52 +++++++---------------
.../apache/ofbiz/webapp/control/ControlFilter.java | 2 +-
3 files changed, 24 insertions(+), 39 deletions(-)
diff --git a/framework/security/config/security.properties
b/framework/security/config/security.properties
index fbcbf175f1..c11ec8a660 100644
--- a/framework/security/config/security.properties
+++ b/framework/security/config/security.properties
@@ -274,13 +274,18 @@ csvformat=CSVFormat.DEFAULT
#--
#-- If you are sure you are safe for a token you can remove it, etc.
#-- If you add a token beware that it does not content ",". It's the separator.
-deniedWebShellTokens=$SHA$OFBiz$c_93W08vqLMlJHjOZ7_A6Wcaenw,$SHA$OFBiz$SigPYIfwat32A80hsAOakW0uH5A,$SHA$OFBiz$--GB0cWVhqHm-dWklW-zlGAIMkU,$SHA$OFBiz$4LL0rcLbpJHftX4g1WeF8ThuKyQ,$SHA$OFBiz$pUBkkg8Z-CiOTIYhIR1kU3DgXqY,$SHA$OFBiz$kpcFR3kDCOtNybDHn8ZPLuCVrOk,$SHA$OFBiz$zadWo3Yv2v9ArAgtj5Hdy1yjjAA,$SHA$OFBiz$gcjailglxcjBO361A7K66-4daLs,$SHA$OFBiz$5z4tXuvujvU8WlSrn3i11oUNFZo,$SHA$OFBiz$uYjP2BSE6bJ8V2QuXPWgiiwcss0,$SHA$OFBiz$fjfa3KJJBB3t7rGS5wh6outrKoY,$SHA$OFBiz$z-t-R4DxwjsPhagQBrQRCBdf3BY,$SH
[...]
+deniedWebShellTokens=java.,beans,freemarker,<script,javascript,<body,body
,<form,<jsp:,<c:out,taglib,<prefix,<%@ page,<?php,exec(,alert(,\
+
%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,assign,webappPath,\
+ ifconfig,route,crontab,netstat,uname
,hostname,iptables,whoami,"cmd",*cmd|,+cmd|,=cmd|,localhost,thread,require,gzdeflate,\
+
execute,println,calc,touch,curl,base64,tcp,4444,base32,xxd,bash
#-- SHA-1 versions of tokens containing (as String) at least one
deniedWebShellTokens
#-- This is notably used to allow special values in query parameters.
#-- If you add a token beware that it does not content ",". It's the separator.
-allowedTokens=$SHA$OFBiz$488OJhFI6NUQlvuqRVFHq6_KN8w
+allowedTokens=$SHA$OFBiz$2naHrANKTniFcgLJk4oXr3IRQ48
allowStringConcatenationInUploadedFiles=false
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 f158262c3f..c4fd73fc05 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.nio.file.Path;
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;
@@ -101,7 +100,7 @@ import com.lowagie.text.pdf.PdfReader;
public class SecuredUpload {
- // To check if a webshell is not uploaded
+ // To check if a webshell is not uploaded or a reverse shell put in the
query string
// This can be helpful:
// https://en.wikipedia.org/wiki/File_format
@@ -122,52 +121,32 @@ 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 isNotValidEncodedText(String content, List<String>
allowed) throws IOException {
- try {
- return isValidText(new String(Base64.getDecoder().decode(content),
StandardCharsets.UTF_8), allowed);
- } catch (IllegalArgumentException e) {
- // the encoded text isn't a Base64, allow it because there is no
security risk
- return false;
- }
- }
-
// Cover method of the same name below. Historically used with 84
references when below was created
- // This is used for checking there is no web shell in an uploaded file
- // A file containing a reverse shell, base64 encoded or not, will be
rejected.
+ // check there is no web shell in the uploaded file
+ // A file containing a reverse shell will be rejected.
public static boolean isValidText(String content, List<String> allowed)
throws IOException {
return isValidText(content, allowed, false);
}
- public static boolean isValidText(String content, List<String> allowed,
boolean testEncodeContent) throws IOException {
+ public static boolean isValidText(String content, List<String> allowed,
boolean isQuery) throws IOException {
if (content == null) {
return false;
}
- if (!testEncodeContent) {
+ if (!isQuery) {
String contentWithoutSpaces = content.replaceAll(" ", "");
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;
}
- }
- // This is used for checking there is no reverse shell in a query
string
- if (testEncodeContent && isNotValidEncodedText(content, allowed)) {
- return false;
- } else if (testEncodeContent) {
- // e.g. split parameters of an at all non encoded HTTP query
string
+ } else {
+ // Check the query string is safe, notably no reverse shell
List<String> queryParameters = StringUtil.split(content, "&");
- return DENIEDWEBSHELLTOKENS.stream().allMatch(token ->
isValid(queryParameters, token, allowed));
+ return DENIEDWEBSHELLTOKENS.stream().allMatch(token ->
isValid(queryParameters, token.toLowerCase(), allowed));
}
- // This is used for checking there is no web shell in an uploaded file
- return DENIEDWEBSHELLTOKENS.stream().allMatch(token ->
isValid(content, token.toLowerCase(), allowed));
+ // Check there is no web shell in an uploaded file
+ return DENIEDWEBSHELLTOKENS.stream().allMatch(token ->
isValid(content.toLowerCase(), token.toLowerCase(), allowed));
}
public static boolean isValidFileName(String fileToCheck, Delegator
delegator) throws IOException {
@@ -872,21 +851,22 @@ public class SecuredUpload {
return isValidText(content, allowed);
}
- // This is used for checking there is no web shell
+ // Check there is no web shell
private static boolean isValid(String content, String string, List<String>
allowed) {
- boolean isOK = !content.toLowerCase().contains(string) ||
allowed.contains(string);
+ boolean isOK = !content.contains(string) || allowed.contains(string);
if (!isOK) {
Debug.logInfo("The uploaded file contains the string '" + string +
"'. It can't be uploaded for security reason", MODULE);
}
return isOK;
}
- // This is used for checking there is no reverse shell
+ // Check there is no reverse shell in query string
private static boolean isValid(List<String> queryParameters, String
string, List<String> allowed) {
boolean isOK = true;
+
for (String parameter : queryParameters) {
- if (!HashCrypt.cryptBytes("SHA", "OFBiz",
parameter.getBytes(StandardCharsets.UTF_8)).contains(string)
- || allowed.contains(HashCrypt.cryptBytes("SHA", "OFBiz",
parameter.getBytes(StandardCharsets.UTF_8)))) {
+ if (!parameter.contains(string)
+ || allowed.contains(HashCrypt.cryptBytes("SHA", "OFBiz",
parameter.toLowerCase().getBytes(StandardCharsets.UTF_8)))) {
continue;
} else {
isOK = false;
diff --git
a/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ControlFilter.java
b/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ControlFilter.java
index 9cc412175a..f2c2a5a37d 100644
---
a/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ControlFilter.java
+++
b/framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ControlFilter.java
@@ -176,7 +176,7 @@ public class ControlFilter extends HttpFilter {
if (queryString != null) {
queryString = URLDecoder.decode(queryString, "UTF-8");
if (UtilValidate.isUrlInString(queryString)
- || !SecuredUpload.isValidText(queryString,
SecuredUpload.getallowedTokens(), true)
+ ||
!SecuredUpload.isValidText(queryString.toLowerCase(),
SecuredUpload.getallowedTokens(), true)
&& isSolrTest()) {
Debug.logError("For security reason this URL is not
accepted", MODULE);
throw new RuntimeException("For security reason this URL
is not accepted");
