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 6c00877a14 Improved: [SECURITY] (CVE-2024-36104) Path traversal
leading to RCE (OFBIZ-13092)
6c00877a14 is described below
commit 6c00877a14fa4c0c85cad6b12c34a4762c61ead0
Author: Jacques Le Roux <jacques.le.r...@les7arts.com>
AuthorDate: Sat Jan 18 10:11:14 2025 +0100
Improved: [SECURITY] (CVE-2024-36104) Path traversal leading to RCE
(OFBIZ-13092)
Using allowedTokens was a bad idea (when all you have is a (new!) hammer,
everything looks like a nail).
It came to my mind that I already used StringEscapeUtils::unescapeHtml4
in several places and in this case it's the tool of choice.
Actually I thought about it when beginning, but was unsure it would be
enough.
Now it's clear in my mind and this replaces allowedTokens by simply
StringEscapeUtils::unescapeHtml4
So this also removes the recently added allowedTokens in
security.properties.
They are now useless.
Also it's quite better because it handles all cases known or unknown.
Better keep allowedTokens list as short as possible.
Conflicts handled by hand in both files
---
framework/security/config/security.properties | 2 +-
.../apache/ofbiz/webapp/control/ControlFilter.java | 41 ++++++----------------
2 files changed, 11 insertions(+), 32 deletions(-)
diff --git a/framework/security/config/security.properties
b/framework/security/config/security.properties
index 73bf5a910f..2658815a78 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,$SHA$OFBiz$evAu1vcT5d1tjVXFTeVXU-6aNz8,$SHA$OFBiz$ORZaKvS7a0ee4gZb9P5hHuHnEyE,$SHA$OFBiz$T5DBu6tPuZzDCfYNci_23SrUa3Q,$SHA$OFBiz$BXhGVix7t3kfHrhNB0z9I0H9_rQ
+allowedTokens=$SHA$OFBiz$2naHrANKTniFcgLJk4oXr3IRQ48
allowStringConcatenationInUploadedFiles=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 4b690d1a4e..badda5d9fc 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,6 @@ 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;
@@ -37,7 +36,7 @@ import javax.servlet.ServletResponse;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
-import org.apache.ofbiz.base.crypto.HashCrypt;
+import org.apache.commons.text.StringEscapeUtils;
import org.apache.ofbiz.base.util.Debug;
import org.apache.ofbiz.base.util.StringUtil;
import org.apache.ofbiz.base.util.UtilProperties;
@@ -118,6 +117,12 @@ public class ControlFilter implements Filter {
return
!GenericValue.getStackTraceAsString().contains("ControlFilterTests")
&& null == System.getProperty("SolrDispatchFilter");
}
+
+ private static List<String> getAllowedTokens() {
+ String allowedTokens = UtilProperties.getPropertyValue("security",
"allowedTokens");
+ return UtilValidate.isNotEmpty(allowedTokens) ?
StringUtil.split(allowedTokens, ",") : new ArrayList<>();
+ }
+
@Override
public void doFilter(ServletRequest request, ServletResponse response,
FilterChain chain) throws IOException, ServletException {
HttpServletRequest httpRequest = (HttpServletRequest) request;
@@ -145,8 +150,7 @@ public class ControlFilter implements Filter {
if (httpRequest.getAttribute(FORWARDED_FROM_SERVLET) == null &&
!allowedPaths.isEmpty()) {
// check to make sure the requested url is allowed
// get the request URI without the webapp mount point
- String requestUri =
URLDecoder.decode(httpRequest.getRequestURI().substring(httpRequest.getContextPath().length()),
"UTF-8");
-
+ String requestUri =
StringEscapeUtils.unescapeHtml4(URLDecoder.decode(httpRequest.getRequestURI().substring(httpRequest.getContextPath().length()),
"UTF-8"));
// Reject wrong URLs
String queryString = null;
@@ -169,18 +173,13 @@ public class ControlFilter implements Filter {
}
if
(!requestUri.matches("/control/logout;jsessionid=[A-Z0-9]{32}\\.jvm1")) {
- boolean bypass = true;
- if (queryString != null) {
- List<String> queryStringList =
StringUtil.splitWithStringSeparator(queryString.toLowerCase(), "&");
- bypass = isAnyAllowedToken(queryStringList, ALLOWEDTOKENS);
- }
- if (requestUri != null && !bypass) { // "null" allows tests
with Mockito. ControlFilterTests sends null.
+ if (requestUri != null) { // "null" allows tests with Mockito
because ControlFilterTests sends null.
try {
String url = new URI(requestUri)
.normalize().toString()
.replaceAll(";", "")
.replaceAll("(?i)%2e", "");
- if (!((HttpServletRequest)
request).getRequestURL().toString().equals(url)) {
+ if (!requestUri.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");
}
@@ -233,24 +232,4 @@ 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;
- }
}
