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 df406ad816 Improved: [SECURITY] (CVE-2024-36104) Path traversal
leading to RCE (OFBIZ-13092)
df406ad816 is described below
commit df406ad81684268a5785ccd2d9588680c7c2f26d
Author: Jacques Le Roux <[email protected]>
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.
---
framework/security/config/security.properties | 2 +-
.../apache/ofbiz/webapp/control/ControlFilter.java | 27 +++-------------------
2 files changed, 4 insertions(+), 25 deletions(-)
diff --git a/framework/security/config/security.properties
b/framework/security/config/security.properties
index 141ec7e899..6fbc9b9ea8 100644
--- a/framework/security/config/security.properties
+++ b/framework/security/config/security.properties
@@ -288,7 +288,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$-MaMN-Dui294v86UT1T8BkG3v8k
+allowedTokens=$SHA$OFBiz$2naHrANKTniFcgLJk4oXr3IRQ48
#-- RegExp to secure groovy script execution. If the regExp match a script, it
would be disabled and OFBiz run nothing.
#-- In this case, you will have on log the original script with it hash. The
hash can be added on allowedScriptletHashes
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 ae215c83bc..6b0cef5f12 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.Arrays;
import java.util.Collections;
@@ -39,9 +38,9 @@ import javax.servlet.http.HttpServletResponse;
import javax.servlet.http.HttpSession;
import org.apache.commons.lang.BooleanUtils;
+import org.apache.commons.text.StringEscapeUtils;
import org.apache.commons.validator.routines.UrlValidator;
import org.apache.logging.log4j.ThreadContext;
-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;
@@ -160,21 +159,6 @@ public class ControlFilter extends HttpFilter {
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;
- }
-
/**
* Makes allowed paths pass through while redirecting the others to a fix
location.
* Reject wrong URLs
@@ -198,7 +182,7 @@ public class ControlFilter extends HttpFilter {
} else if (req.getAttribute(FORWARDED_FROM_SERVLET) == null
&& !allowedPaths.isEmpty()) {
// Get the request URI without the webapp mount point.
- String uriWithContext = URLDecoder.decode(req.getRequestURI(),
"UTF-8");
+ String uriWithContext =
StringEscapeUtils.unescapeHtml4(URLDecoder.decode(req.getRequestURI(),
"UTF-8"));
String uri = uriWithContext.substring(context.length());
GenericValue userLogin = (GenericValue)
session.getAttribute("userLogin");
@@ -227,12 +211,7 @@ public class ControlFilter extends HttpFilter {
throw new RuntimeException("For security reason this URL
is not accepted");
}
}
- boolean bypass = true;
- if (queryString != null) {
- List<String> queryStringList =
StringUtil.splitWithStringSeparator(queryString.toLowerCase(), "&");
- bypass = isAnyAllowedToken(queryStringList, ALLOWEDTOKENS);
- }
- if (uriWithContext != null && !bypass) { // "null" allows tests
with Mockito. ControlFilterTests sends null.
+ if (uriWithContext != null) { // "null" allows tests with Mockito
because ControlFilterTests sends null.
try {
String uRIFiltered = new URI(uriWithContext)
.normalize().toString()
