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 52bf8b30a1 Fixed: [SECURITY] (CVE-2024-36104) Path traversal leading
to RCE (OFBIZ-13092)
52bf8b30a1 is described below
commit 52bf8b30a1ee8d78b457fe6b89cc79c7c56815ff
Author: Jacques Le Roux <jacques.le.r...@les7arts.com>
AuthorDate: Thu Jan 16 11:28:00 2025 +0100
Fixed: [SECURITY] (CVE-2024-36104) Path traversal leading to RCE
(OFBIZ-13092)
I've got a strange issue that seems caused by one of this Jira modification:
On demo environment, when clicking on several sort field links, a
'For security reason this URL is not accepted' error is thrown.
For instance, if you perform a search on find party screen and try to sort
by
partyId, createdDate or else, you will get this message.
I tried to reproduce it in a local trunk environment without success : the
uRIFiltered and the initialURI are the same.
jleroux: but indeed when the same is done on a domain site, for instance
official OFBiz demos, it's reproducible in all demo instances.
This fixes it, with some more: refactoring, formatting and renaming
Thanks: Leïla for report and details.
Conflicts handled by hand (a lot) in ControlFilter.java
---
framework/security/config/security.properties | 2 +-
.../org/apache/ofbiz/security/SecuredUpload.java | 6 --
.../apache/ofbiz/webapp/control/ControlFilter.java | 69 ++++++++++++++++++----
3 files changed, 57 insertions(+), 20 deletions(-)
diff --git a/framework/security/config/security.properties
b/framework/security/config/security.properties
index 2658815a78..1204f9bd05 100644
--- a/framework/security/config/security.properties
+++ b/framework/security/config/security.properties
@@ -250,7 +250,7 @@
deniedWebShellTokens=java.,beans,freemarker,<script,javascript,<body,body ,<form
#-- 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$2naHrANKTniFcgLJk4oXr3IRQ48
+allowedTokens=$SHA$OFBiz$2naHrANKTniFcgLJk4oXr3IRQ48,$SHA$OFBiz$evAu1vcT5d1tjVXFTeVXU-6aNz8,$SHA$OFBiz$-MaMN-Dui294v86UT1T8BkG3v8k
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 5d557ed80a..059ca043e7 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
@@ -846,12 +846,6 @@ 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) {
if (MAXLINELENGTH == 0) {
return true;
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 9776e64e5d..19d0697806 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
@@ -22,7 +22,10 @@ import java.io.IOException;
import java.net.URI;
import java.net.URISyntaxException;
import java.net.URLDecoder;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
import java.util.HashSet;
+import java.util.List;
import java.util.Set;
import javax.servlet.Filter;
@@ -34,7 +37,10 @@ import javax.servlet.ServletResponse;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
+import org.apache.ofbiz.base.crypto.HashCrypt;
import org.apache.ofbiz.base.util.Debug;
+import org.apache.ofbiz.base.util.StringUtil;
+import org.apache.ofbiz.base.util.UtilProperties;
import org.apache.ofbiz.base.util.UtilValidate;
import org.apache.ofbiz.entity.GenericValue;
import org.apache.ofbiz.security.SecuredUpload;
@@ -76,6 +82,7 @@ public class ControlFilter implements Filter {
private String redirectPath;
protected int errorCode;
private Set<String> allowedPaths = new HashSet<>();
+ private static final List<String> ALLOWEDTOKENS = getAllowedTokens();
@Override
public void init(FilterConfig filterConfig) throws ServletException {
@@ -140,29 +147,45 @@ public class ControlFilter implements Filter {
// get the request URI without the webapp mount point
String requestUri =
URLDecoder.decode(httpRequest.getRequestURI().substring(httpRequest.getContextPath().length()),
"UTF-8");
+
// Reject wrong URLs
- String queryString = httpRequest.getQueryString();
+ String queryString = null;
+ try {
+ queryString = new URI(requestUri).getQuery();
+
+ } catch (URISyntaxException e) {
+ Debug.logError("Weird URI: " + e, module);
+ throw new RuntimeException(e);
+ }
+
if (queryString != null) {
queryString = URLDecoder.decode(queryString, "UTF-8");
if (UtilValidate.isUrl(queryString)
- ||
!SecuredUpload.isValidText(queryString.toLowerCase(),
SecuredUpload.getallowedTokens(), true)
- && isSolrTest()) {
+ ||
!SecuredUpload.isValidText(queryString.toLowerCase(), ALLOWEDTOKENS, 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())
- .normalize().toString()
- .replaceAll(";", "")
- .replaceAll("(?i)%2e", "");
- if (!((HttpServletRequest)
request).getRequestURL().toString().equals(url)) {
- Debug.logError("For security reason this URL is not
accepted", module);
- throw new RuntimeException("For security reason this
URL is not accepted");
+ boolean bypass = true;
+ if (queryString != null) {
+ bypass =
isAnyAllowedToken(StringUtil.split(queryString.toLowerCase(), "Y&"),
ALLOWEDTOKENS);
+ }
+ if (requestUri != null && !bypass) { // "null" allows tests
with Mockito. ControlFilterTests sends null.
+ try {
+ String url = new URI(((HttpServletRequest)
request).getRequestURL().toString())
+ .normalize().toString()
+ .replaceAll(";", "")
+ .replaceAll("(?i)%2e", "");
+ if (!((HttpServletRequest)
request).getRequestURL().toString().equals(url)) {
+ Debug.logError("For security reason this URL is
not accepted", module);
+ throw new RuntimeException("For security reason
this URL is not accepted");
+ }
+ } catch (URISyntaxException e) {
+ throw new RuntimeException(e);
}
- } catch (URISyntaxException e) {
- throw new RuntimeException(e);
}
}
@@ -209,4 +232,24 @@ public class ControlFilter implements Filter {
public void destroy() {
}
+
+ private static List<String> getAllowedTokens() {
+ String allowedTokens = UtilProperties.getPropertyValue("security",
"allowedTokens");
+ return UtilValidate.isNotEmpty(allowedTokens) ?
StringUtil.split(allowedTokens, ",") : new ArrayList<>();
+ }
+
+ // Check there is any allowedToken in URL
+ private static boolean isAnyAllowedToken(List<String> queryParameters,
List<String> allowed) {
+ boolean isOK = false;
+ for (String parameter : queryParameters) {
+ parameter = parameter.substring(0, parameter.indexOf("=") + 1);
+ if (allowed.contains(HashCrypt.cryptBytes("SHA", "OFBiz",
parameter.getBytes(StandardCharsets.UTF_8)))) {
+ isOK = true;
+ break;
+ } else {
+ continue;
+ }
+ }
+ return isOK;
+ }
}
