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
commit f95ab015f353ca820b3718747575898e87dd9084
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.
---
framework/security/config/security.properties | 2 +-
.../org/apache/ofbiz/security/SecuredUpload.java | 6 ---
.../apache/ofbiz/webapp/control/ControlFilter.java | 62 ++++++++++++++++++----
3 files changed, 52 insertions(+), 18 deletions(-)
diff --git a/framework/security/config/security.properties
b/framework/security/config/security.properties
index 6fbc9b9ea8..141ec7e899 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
+allowedTokens=$SHA$OFBiz$2naHrANKTniFcgLJk4oXr3IRQ48,$SHA$OFBiz$evAu1vcT5d1tjVXFTeVXU-6aNz8,$SHA$OFBiz$-MaMN-Dui294v86UT1T8BkG3v8k
#-- 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/security/src/main/java/org/apache/ofbiz/security/SecuredUpload.java
b/framework/security/src/main/java/org/apache/ofbiz/security/SecuredUpload.java
index 8c88d46278..cfe6e1f9ee 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
@@ -910,12 +910,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 ca8d2cca69..2c97b0c9f2 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,8 +22,11 @@ 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;
+import java.util.List;
import java.util.Set;
import java.util.stream.Collectors;
@@ -38,7 +41,10 @@ import javax.servlet.http.HttpSession;
import org.apache.commons.lang.BooleanUtils;
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;
import org.apache.ofbiz.base.util.UtilValidate;
import org.apache.ofbiz.entity.GenericValue;
import org.apache.ofbiz.security.SecuredFreemarker;
@@ -85,6 +91,8 @@ public class ControlFilter extends HttpFilter {
private int errorCode;
/** The list of all path prefixes that are allowed. */
private Set<String> allowedPaths;
+ private static final List<String> ALLOWEDTOKENS = getAllowedTokens();
+
@Override
public void init(FilterConfig conf) throws ServletException {
@@ -139,8 +147,7 @@ public class ControlFilter extends HttpFilter {
* Makes allowed paths pass through while redirecting the others to a fix
location.
*/
@Override
- public void doFilter(HttpServletRequest req, HttpServletResponse resp,
FilterChain chain)
- throws IOException, ServletException {
+ public void doFilter(HttpServletRequest req, HttpServletResponse resp,
FilterChain chain) throws IOException, ServletException {
String context = req.getContextPath();
HttpSession session = req.getSession();
@@ -158,7 +165,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 = req.getRequestURI();
+ String uriWithContext = URLDecoder.decode(req.getRequestURI(),
"UTF-8");
String uri = uriWithContext.substring(context.length());
GenericValue userLogin = (GenericValue)
session.getAttribute("userLogin");
@@ -169,25 +176,35 @@ public class ControlFilter extends HttpFilter {
}
// Reject wrong URLs
- String queryString = req.getQueryString();
+ String queryString = null;
+ try {
+ queryString = new URI(uriWithContext).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.isUrlInString(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");
}
}
-
- String initialURI = req.getRequestURI();
- if (initialURI != null) { // Allow tests with Mockito.
ControlFilterTests send null
+ boolean bypass = true;
+ if (queryString != null) {
+ bypass =
isAnyAllowedToken(StringUtil.split(queryString.toLowerCase(), "Y&"),
ALLOWEDTOKENS);
+ }
+ if (uriWithContext != null && !bypass) { // "null" allows tests
with Mockito. ControlFilterTests sends null.
try {
- String uRIFiltered = new URI(initialURI)
+ String uRIFiltered = new URI(uriWithContext)
.normalize().toString()
.replaceAll(";", "")
.replaceAll("(?i)%2e", "");
- if (!initialURI.equals(uRIFiltered)) {
+ if (!uriWithContext.equals(uRIFiltered)) {
Debug.logError("For security reason this URL is not
accepted", MODULE);
throw new RuntimeException("For security reason this
URL is not accepted");
}
@@ -230,4 +247,27 @@ public class ControlFilter extends HttpFilter {
private void redirect(HttpServletResponse resp, String contextPath) throws
IOException {
resp.sendRedirect(redirectPathIsUrl ? redirectPath : (contextPath +
redirectPath));
}
+
+ 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;
+ }
}
