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 3eb3fc9b15 Improved: Prevent URL parameters manipulation (OFBIZ-13147)
3eb3fc9b15 is described below
commit 3eb3fc9b156f4f27d4270ceadd808d87748d9095
Author: Jacques Le Roux <jacques.le.r...@les7arts.com>
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.
---
framework/security/config/security.properties | 2 +-
.../org/apache/ofbiz/security/SecuredUpload.java | 33 ++++++++++++++++++++--
.../apache/ofbiz/security/SecurityUtilTest.java | 4 ++-
.../apache/ofbiz/webapp/control/ControlFilter.java | 4 +--
4 files changed, 37 insertions(+), 6 deletions(-)
diff --git a/framework/security/config/security.properties
b/framework/security/config/security.properties
index 2c54c8a454..c7effe8d3f 100644
--- a/framework/security/config/security.properties
+++ b/framework/security/config/security.properties
@@ -278,7 +278,7 @@
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,curl,base64
+ execute,println,calc,touch,curl,base64,tcp,4444
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 8e83b452bb..0b80473b75 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
@@ -38,6 +38,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.HashSet;
import java.util.Iterator;
@@ -119,14 +120,42 @@ 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 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 {
- String contentWithoutSpaces = content.replace(" ", "");
+ 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;
}
- return content != null ? DENIEDWEBSHELLTOKENS.stream().allMatch(token
-> isValid(content, token.toLowerCase(), allowed)) : 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/security/src/test/java/org/apache/ofbiz/security/SecurityUtilTest.java
b/framework/security/src/test/java/org/apache/ofbiz/security/SecurityUtilTest.java
index 93bcbf8a12..2354a60b0c 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
@@ -64,7 +64,7 @@ public class SecurityUtilTest {
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
+ execute,println,calc,touch,curl,base64,tcp
*/
try {
List<String> allowed = new ArrayList<>();
@@ -153,6 +153,8 @@ public class SecurityUtilTest {
assertFalse(SecuredUpload.isValidText("touch", allowed));
assertFalse(SecuredUpload.isValidText("curl", allowed));
assertFalse(SecuredUpload.isValidText("base64", allowed));
+ assertFalse(SecuredUpload.isValidText("tcp", allowed));
+ assertFalse(SecuredUpload.isValidText("4444", allowed));
} catch (IOException e) {
fail(String.format("IOException occured : %s", e.getMessage()));
}
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 a4122fdcf2..b696cd6367 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
@@ -173,8 +173,8 @@ public class ControlFilter extends HttpFilter {
// 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");
}
