This is an automated email from the ASF dual-hosted git repository. mridulpathak pushed a commit to branch trunk in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git
commit 5343967656d71f767142fd69e7e978bfacf22708 Author: Mridul Pathak <[email protected]> AuthorDate: Sun Oct 18 22:39:10 2026 +0530 Improved: Remove the parsing of query strings encoded in the request path Now that sort links no longer put their query string in the request path, UtilHttp.getParameterMap no longer parses a query string out of an encoded request path, and ControlFilter no longer checks such a query string against deniedWebShellTokens. The allowedTokens property and the query string mode of SecuredUpload.isValidText, which were only used by that check, are removed. --- .../java/org/apache/ofbiz/base/util/UtilHttp.java | 20 ----------- framework/security/config/security.properties | 5 --- .../org/apache/ofbiz/security/SecuredUpload.java | 41 +++------------------- .../apache/ofbiz/webapp/control/ControlFilter.java | 28 --------------- 4 files changed, 5 insertions(+), 89 deletions(-) diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/util/UtilHttp.java b/framework/base/src/main/java/org/apache/ofbiz/base/util/UtilHttp.java index 9c05f3248a..565122cb20 100644 --- a/framework/base/src/main/java/org/apache/ofbiz/base/util/UtilHttp.java +++ b/framework/base/src/main/java/org/apache/ofbiz/base/util/UtilHttp.java @@ -33,10 +33,7 @@ import java.io.InputStream; import java.io.OutputStream; import java.io.UnsupportedEncodingException; import java.net.FileNameMap; -import java.net.URI; -import java.net.URISyntaxException; import java.net.URLConnection; -import java.net.URLDecoder; import java.net.URLEncoder; import java.nio.ByteBuffer; import java.nio.charset.Charset; @@ -69,8 +66,6 @@ import org.apache.commons.fileupload2.core.DiskFileItemFactory; import org.apache.commons.fileupload2.core.FileItem; import org.apache.commons.fileupload2.core.FileUploadException; import org.apache.commons.fileupload2.jakarta.JakartaServletFileUpload; -import org.apache.http.NameValuePair; -import org.apache.http.client.utils.URLEncodedUtils; import org.apache.ofbiz.entity.Delegator; import org.apache.ofbiz.entity.util.EntityUtilProperties; import org.apache.ofbiz.webapp.control.ConfigXMLReader; @@ -171,21 +166,6 @@ public final class UtilHttp { if (Debug.verboseOn()) { Debug.logVerbose("Made Request Parameter Map with [" + params.size() + "] Entries", MODULE); } - - // Handles encoded queryStrings - String requestURI = req.getRequestURI(); - if (params.isEmpty() && null != requestURI) { - try { - List<NameValuePair> nameValuePairs = URLEncodedUtils.parse( - new URI(UtilHttp.encodeBlanks(URLDecoder.decode(requestURI, "UTF-8"))), - Charset.forName("UTF-8")); - for (NameValuePair element : nameValuePairs) { - params.put(element.getName(), element.getValue()); - } - } catch (UnsupportedEncodingException | URISyntaxException e) { - Debug.logError("Can't handle encoded queryString " + requestURI, MODULE); - } - } return canonicalizeParameterMap(params); } diff --git a/framework/security/config/security.properties b/framework/security/config/security.properties index 65b1e4ff4a..675667b9c6 100644 --- a/framework/security/config/security.properties +++ b/framework/security/config/security.properties @@ -372,11 +372,6 @@ deniedWebShellTokens=java.,beans,freemarker,<script,javascript,<body,body ,<form execute,println,calc,touch,curl,base64, tcp ,4444,base32, tr , xxd ,bash, od ,|od ,printf,echo -#-- 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 - #-- 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 #-- properties to accept it on the next execution. 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 91112ad74c..9d9009cc22 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 @@ -78,7 +78,6 @@ import org.apache.commons.csv.CSVParser; import org.apache.commons.io.FileUtils; import org.apache.commons.io.FilenameUtils; import org.apache.commons.lang.StringUtils; -import org.apache.ofbiz.base.crypto.HashCrypt; import org.apache.ofbiz.base.util.Debug; import org.apache.ofbiz.base.util.FileUtil; import org.apache.ofbiz.base.util.StringUtil; @@ -286,28 +285,17 @@ public class SecuredUpload { return true; } - // Cover method of the same name below. Historically used with 84 references when below was created // 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 isQuery) throws IOException { if (content == null) { return false; } - 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; - } - } 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.toLowerCase(), allowed)); + 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; } // Check there is no web shell in an uploaded file @@ -1594,25 +1582,6 @@ public class SecuredUpload { return isOK; } - // 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 (!parameter.contains(string) - || allowed.contains(HashCrypt.cryptBytes("SHA", "OFBiz", parameter.toLowerCase().getBytes(StandardCharsets.UTF_8)))) { - continue; - } else { - isOK = false; - break; - } - } - if (!isOK) { - Debug.logInfo("The HTTP query string contains the string '" + string + "'. It can't be uploaded for security reason", MODULE); - } - return isOK; - } - private static void deleteBadFile(String fileToCheck) { Debug.logError("File : " + fileToCheck + ", can't be uploaded for security reason", MODULE); File badFile = new File(fileToCheck); 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 acc7d3a704..3694e7f701 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.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; @@ -36,15 +35,11 @@ import org.apache.commons.text.StringEscapeUtils; import org.apache.commons.validator.routines.UrlValidator; import org.apache.logging.log4j.ThreadContext; import org.apache.ofbiz.base.util.Debug; -import org.apache.ofbiz.base.util.StringUtil; import org.apache.ofbiz.base.util.UtilGenerics; import org.apache.ofbiz.base.util.UtilHttp; -import org.apache.ofbiz.base.util.UtilProperties; -import org.apache.ofbiz.base.util.UtilValidate; import org.apache.ofbiz.common.UrlServletHelper; import org.apache.ofbiz.entity.GenericValue; import org.apache.ofbiz.security.SecuredFreemarker; -import org.apache.ofbiz.security.SecuredUpload; import jakarta.servlet.FilterChain; import jakarta.servlet.FilterConfig; @@ -95,7 +90,6 @@ 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 @@ -151,11 +145,6 @@ public class ControlFilter extends HttpFilter { 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<>(); - } - private static boolean isControlFilterTests() { return null != System.getProperty("ControlFilterTests"); } @@ -230,23 +219,6 @@ public class ControlFilter extends HttpFilter { } } - // Reject insecure URLs - 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(), ALLOWEDTOKENS, true)) { - Debug.logError("For security reason this URL is not accepted", MODULE); - throw new RuntimeException("For security reason this URL is not accepted"); - } - } if (uriWithContext != null) { // "null" allows tests with Mockito because ControlFilterTests sends null. try { String uRIFiltered = new URI(uriWithContext)
