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 721e2e8cd5 Improved: Prevent URL parameters manipulation (OFBIZ-13147)
721e2e8cd5 is described below
commit 721e2e8cd52a809b78e524cf09976c9410834720
Author: Jacques Le Roux <[email protected]>
AuthorDate: Thu Oct 24 08:29:21 2024 +0200
Improved: Prevent URL parameters manipulation (OFBIZ-13147)
Improves SecuredUpload.java in order to not accept Base64 encoded query
strings
even if "base64" is already in deniedWebShellTokens, better guarantee.
Thanks to Nicolas.
Just an indentation in ControlFilter.
Add tcp and 4444 (for udp) in deniedWebShellTokens to stop reverse shells.
Conflicts handled by hand
framework/security/config/security.properties
framework/security/src/main/java/org/apache/ofbiz/security/SecuredUpload.java
framework/webapp/src/main/java/org/apache/ofbiz/webapp/control/ControlFilter.java
---
framework/security/config/security.properties | 4 ++-
.../org/apache/ofbiz/security/SecuredUpload.java | 39 +++++++++++++++++++++-
.../apache/ofbiz/webapp/control/ControlFilter.java | 4 +--
3 files changed, 43 insertions(+), 4 deletions(-)
diff --git a/framework/security/config/security.properties
b/framework/security/config/security.properties
index cbc3d54672..eb030d7dc2 100644
--- a/framework/security/config/security.properties
+++ b/framework/security/config/security.properties
@@ -240,7 +240,9 @@
deniedWebShellTokens=java.,beans,freemarker,<script,javascript,<body,body ,<form
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,calculate,curl,base64
+ execute,println,calc,touch,curl,base64,tcp,4444
+
+allowStringConcatenationInUploadedFiles=false
#-- Max line length for uploaded files, by default 10000
maxLineLength=
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 edf7f189e1..496efccc91 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
@@ -37,6 +37,7 @@ 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.HashMap;
import java.util.HashSet;
@@ -107,9 +108,45 @@ public class SecuredUpload {
private static final List<String> DENIEDFILEEXTENSIONS =
getDeniedFileExtensions();
private static final List<String> DENIEDWEBSHELLTOKENS =
getDeniedWebShellTokens();
private static final Integer MAXLINELENGTH =
UtilProperties.getPropertyAsInteger("security", "maxLineLength", 10000);
+ private static final Boolean ALLOWSTRINGCONCATENATIONINUPLOADEDFILES =
+ UtilProperties.getPropertyAsBoolean("security",
"allowStringConcatenationInUploadedFiles", false);
+
+ /**
+ * For the content analyze if it's a base64 string with potential code
injection
+ * @param content
+ * @param allowed
+ * @return
+ * @throws IOException
+ */
+ public static boolean isValidEncodedText(String content, List<String>
allowed) throws IOException {
+ try {
+ return
!isValidText(Base64.getDecoder().decode(content).toString(), allowed, false)
+ ||
!isValidText(Base64.getMimeDecoder().decode(content).toString(), allowed, false)
+ ||
!isValidText(Base64.getUrlDecoder().decode(content).toString(), allowed, false);
+ } catch (IllegalArgumentException e) {
+ // the encoded text isn't a Base64, allow it because there is no
security risk
+ return true;
+ }
+ }
public static boolean isValidText(String content, List<String> allowed)
throws IOException {
- return content != null ? DENIEDWEBSHELLTOKENS.stream().allMatch(token
-> isValid(content, token.toLowerCase(), allowed)) : false;
+ return isValidText(content, allowed, true);
+ }
+
+ 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 && !isValidEncodedText(content, allowed)) {
+ return false;
+ }
+ return DENIEDWEBSHELLTOKENS.stream().allMatch(token ->
isValid(content, token.toLowerCase(), allowed));
}
public static boolean isValidFileName(String fileToCheck, Delegator
delegator) throws IOException {
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 8d56123775..2a03dbe314 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
@@ -145,8 +145,8 @@ public class ControlFilter implements Filter {
// wt=javabin allows Solr tests, see
https://cwiki.apache.org/confluence/display/solr/javabin
if (UtilValidate.isUrl(queryString)
|| !SecuredUpload.isValidText(queryString,
Collections.emptyList())
- &&
!(queryString.contains("JavaScriptEnabled=Y")
- ||
queryString.contains("wt=javabin"))) {
+ && !(queryString.contains("JavaScriptEnabled=Y")
+ || queryString.contains("wt=javabin"))) {
Debug.logError("For security reason this URL is not
accepted", module);
throw new RuntimeException("For security reason this URL
is not accepted");
}
