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
commit 8385c8e75550c779bb30c93f1075499d524578be
Author: Jacques Le Roux <jacques.le.r...@les7arts.com>
AuthorDate: Tue Nov 26 07:56:44 2024 +0100
Improved: Prevent URL parameters manipulation (OFBIZ-13147)
Add ROT13, and improves few short deniedWebShellTokens by surrounding them
by
spaces. It's to avoid collisions while loading image files
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.
Adds 6 new deniedWebShellTokens in security.properties
This follows Leïla comment. It simplifies the
SecuredUpload::isValidEncodedText
content and renames it isNotValidEncodedText
Also 3 new deniedWebShellTokens are added, some more to come
Conflicts handled by hand in ControlFilter.java and SecuredUpload.java
---
framework/security/config/security.properties | 13 +++++-
.../org/apache/ofbiz/security/SecuredUpload.java | 49 ++++++++++------------
.../apache/ofbiz/webapp/control/ControlFilter.java | 2 +-
.../widget/renderer/macro/MacroMenuRenderer.java | 7 +---
4 files changed, 35 insertions(+), 36 deletions(-)
diff --git a/framework/security/config/security.properties
b/framework/security/config/security.properties
index 65ad284b38..e018769b76 100644
--- a/framework/security/config/security.properties
+++ b/framework/security/config/security.properties
@@ -236,12 +236,21 @@ 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
[...]
+#--
+#-- If you cross issues while loading an image file because of a token found
there, you may try to surround the string by spaces, as " tr " below.
+#-- Actually most of the tokens should but it's now a bit late for me. I mean
to test all of them...
+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,
tr , 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 9e9a5bd45d..1663a0b2de 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
@@ -40,7 +40,6 @@ 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;
import java.util.Iterator;
import java.util.List;
@@ -52,7 +51,7 @@ import javax.imageio.ImageIO;
import javax.imageio.ImageReader;
import javax.imageio.stream.ImageInputStream;
-import org.apache.batik.dom.svg.SAXSVGDocumentFactory;
+import org.apache.batik.anim.dom.SAXSVGDocumentFactory;
import org.apache.batik.util.XMLResourceDescriptor;
import org.apache.commons.csv.CSVFormat;
import org.apache.commons.csv.CSVParser;
@@ -98,7 +97,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
@@ -120,46 +119,41 @@ public class SecuredUpload {
* @return
* @throws IOException
*/
- public static boolean isValidEncodedText(String content, List<String>
allowed) throws IOException {
+ public static boolean isNotValidEncodedText(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);
+ 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 true;
+ 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
+ // 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 && !isValidEncodedText(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 {
@@ -451,7 +445,7 @@ public class SecuredUpload {
default:
throw new IOException("Format of the original image "
+ fileName + " is not supported for write operation !");
}
- imageParser.writeImage(sanitizedImage, fos, new
HashMap<>());
+ imageParser.writeImage(sanitizedImage, fos, null);
}
// Set state flag
safeState = true;
@@ -505,7 +499,7 @@ public class SecuredUpload {
return false;
}
// Load stream in PDF parser
- PdfReader reader = new PdfReader(file.getAbsolutePath());
+ new PdfReader(file.getAbsolutePath());
return true;
} catch (Exception e) {
return false;
@@ -802,21 +796,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 126a6be85d..4e9a35efb4 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,7 +145,7 @@ public class ControlFilter implements Filter {
if (queryString != null) {
queryString = URLDecoder.decode(queryString, "UTF-8");
if (UtilValidate.isUrl(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");
diff --git
a/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/macro/MacroMenuRenderer.java
b/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/macro/MacroMenuRenderer.java
index 17998b01ce..81cbd3b6b9 100644
---
a/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/macro/MacroMenuRenderer.java
+++
b/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/macro/MacroMenuRenderer.java
@@ -257,12 +257,7 @@ public class MacroMenuRenderer implements
MenuStringRenderer {
targetParameters.append(parameter.getKey());
targetParameters.append("'");
targetParameters.append(",'value':'");
- UtilCodec.SimpleEncoder simpleEncoder =
(UtilCodec.SimpleEncoder) context.get("simpleEncoder");
- if (simpleEncoder != null) {
-
targetParameters.append(simpleEncoder.encode(parameter.getValue()));
- } else {
- targetParameters.append(parameter.getValue());
- }
+ targetParameters.append(parameter.getValue());
targetParameters.append("'}");
}
targetParameters.append("]");
