This is an automated email from the ASF dual-hosted git repository.
jleroux pushed a commit to branch release24.09
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
The following commit(s) were added to refs/heads/release24.09 by this push:
new 05d0de94ba Improved: Prevent URL parameters manipulation (OFBIZ-13147)
05d0de94ba is described below
commit 05d0de94bab85e96de30bf42b20e89ea2b8ffae2
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 1a276dc6b0..23a44f0a6f 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");
}
