This is an automated email from the ASF dual-hosted git repository.
jleroux pushed a commit to branch release18.12
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
The following commit(s) were added to refs/heads/release18.12 by this push:
new b15ffa06fc Improved: Prevent URL parameters manipulation (OFBIZ-13147)
b15ffa06fc is described below
commit b15ffa06fc348c085ae178a77a4e4e83f610f4f8
Author: Jacques Le Roux <jacques.le.r...@les7arts.com>
AuthorDate: Fri Nov 8 16:34:17 2024 +0100
Improved: Prevent URL parameters manipulation (OFBIZ-13147)
I have refactored SecuredUpload a bit by clearly separating what is used for
web shell in uploaded files and reverse shell in query strings.
The idea is also to keep as much as possible securing code in SecuredUpload.
Even if now more than upload is concerned.
Conflicts handled by hand in ControlFilter.java
---
framework/security/config/security.properties | 13 +++---
.../org/apache/ofbiz/security/SecuredUpload.java | 50 +++++++++++++++++++---
.../apache/ofbiz/webapp/control/ControlFilter.java | 13 ++++++
3 files changed, 64 insertions(+), 12 deletions(-)
diff --git a/framework/security/config/security.properties
b/framework/security/config/security.properties
index eb030d7dc2..504d8013bf 100644
--- a/framework/security/config/security.properties
+++ b/framework/security/config/security.properties
@@ -235,12 +235,13 @@ csvformat=CSVFormat.DEFAULT
#-- Else "template.utility.Execute" is a good replacement but not as much
catching, who knows...
#--
#-- If you are sure you are safe for a token you can remove it, etc.
-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
+#-- 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
[...]
+
+#-- 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$EP-l2t4A_60cRYYnEqEaSiDjfrs,$SHA$OFBiz$JG1RWjLnFzQOpNRUqllybbbfyOE
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 496efccc91..9e9a5bd45d 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
@@ -32,6 +32,7 @@ import java.io.StringReader;
import java.nio.ByteBuffer;
import java.nio.charset.CharacterCodingException;
import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
@@ -69,6 +70,7 @@ import
org.apache.commons.imaging.formats.tiff.TiffImageParser;
import org.apache.commons.io.FileUtils;
import org.apache.commons.io.FilenameUtils;
import org.apache.commons.lang.StringUtils;
+import org.apache.ofbiz.base.crypto.HashCrypt;
import org.apache.ofbiz.base.util.Debug;
import org.apache.ofbiz.base.util.FileUtil;
import org.apache.ofbiz.base.util.StringUtil;
@@ -129,23 +131,34 @@ public class SecuredUpload {
}
}
+ // 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
public static boolean isValidText(String content, List<String> allowed)
throws IOException {
- return isValidText(content, allowed, true);
+ return isValidText(content, allowed, false);
}
public static boolean isValidText(String content, List<String> allowed,
boolean testEncodeContent) throws IOException {
if (content == null) {
return false;
}
- String contentWithoutSpaces = content.replaceAll("\\s", "");
- 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;
+ if (!testEncodeContent) {
+ 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 && !isValidEncodedText(content, allowed)) {
return false;
+ } else if (testEncodeContent) {
+ // e.g. split parameters of an at all non encoded HTTP query
string
+ List<String> queryParameters = StringUtil.split(content, "&");
+ return DENIEDWEBSHELLTOKENS.stream().allMatch(token ->
isValid(queryParameters, token, 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));
}
@@ -789,6 +802,7 @@ public class SecuredUpload {
return isValidText(content, allowed);
}
+ // This is used for checking 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);
if (!isOK) {
@@ -797,6 +811,24 @@ public class SecuredUpload {
return isOK;
}
+ // This is used for checking there is no reverse shell
+ 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)))) {
+ continue;
+ } else {
+ isOK = false;
+ break;
+ }
+ }
+ if (!isOK) {
+ Debug.logInfo("The HTTP query string contains the string '" +
string + "'. It can't be uploaded for security reason", MODULE);
+ }
+ return isOK;
+ }
+
private static void deleteBadFile(String fileToCheck) {
Debug.logError("File : " + fileToCheck + ", can't be uploaded for
security reason", MODULE);
File badFile = new File(fileToCheck);
@@ -815,6 +847,12 @@ public class SecuredUpload {
return UtilValidate.isNotEmpty(deniedTokens) ?
StringUtil.split(deniedTokens, ",") : new ArrayList<>();
}
+ public static List<String> getallowedTokens() {
+ String allowedTokens = UtilProperties.getPropertyValue("security",
"allowedTokens");
+ return UtilValidate.isNotEmpty(allowedTokens) ?
StringUtil.split(allowedTokens, ",") : new ArrayList<>();
+ }
+
+
private static boolean checkMaxLinesLength(String fileToCheck) {
try {
File file = new File(fileToCheck);
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 8f0a984a4e..126a6be85d 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
@@ -21,6 +21,7 @@ package org.apache.ofbiz.webapp.control;
import java.io.IOException;
import java.net.URI;
import java.net.URISyntaxException;
+import java.net.URLDecoder;
import java.util.HashSet;
import java.util.Set;
@@ -34,7 +35,9 @@ import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import org.apache.ofbiz.base.util.Debug;
+import org.apache.ofbiz.base.util.UtilValidate;
import org.apache.ofbiz.entity.GenericValue;
+import org.apache.ofbiz.security.SecuredUpload;
import org.apache.ofbiz.security.SecurityUtil;
/*
@@ -138,6 +141,16 @@ public class ControlFilter implements Filter {
String requestUri =
httpRequest.getRequestURI().substring(httpRequest.getContextPath().length());
// Reject wrong URLs
+ String queryString = httpRequest.getQueryString();
+ if (queryString != null) {
+ queryString = URLDecoder.decode(queryString, "UTF-8");
+ if (UtilValidate.isUrl(queryString)
+ || !SecuredUpload.isValidText(queryString,
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");
+ }
+ }
if
(!requestUri.matches("/control/logout;jsessionid=[A-Z0-9]{32}\\.jvm1")) {
try {
String url = new URI(((HttpServletRequest)
request).getRequestURL().toString())
