From 27eccfa0d4ae51658a1c8d91c0cd73d24dbcb0ec Mon Sep 17 00:00:00 2001 From: Nicolas Malin Date: Wed, 22 Jan 2025 16:58:17 +0100 Subject: [PATCH 1/5] Improved: [SECURITY] (CVE-2024-36104) Path traversal leading to RCE (OFBIZ-13092) Improved the denied token resolution through regexp pattern. We define each potential token and we generate the following regexp for each **** .*(%.{2,5}|[^\\w])" + token + "[^\\w].*" **** We also improved the allowed token with analysed it form security.properties and web.xml directly by plain text of with sha signature to manage each special case --- framework/security/config/security.properties | 11 +- .../apache/ofbiz/security/SecuredUpload.java | 105 ++++++++++-------- .../ofbiz/security/SecurityUtilTest.java | 44 ++++++++ .../ofbiz/webapp/control/ControlFilter.java | 3 +- 4 files changed, 112 insertions(+), 51 deletions(-) diff --git a/framework/security/config/security.properties b/framework/security/config/security.properties index 544dc16421..caba407681 100644 --- a/framework/security/config/security.properties +++ b/framework/security/config/security.properties @@ -277,13 +277,12 @@ csvformat=CSVFormat.DEFAULT #-- #-- If you cross issues while loading an image file because of a token found there, you may try to surround the string by spaces, as " tr " below. #-- Actually most of the tokens should but it's now a bit late for me. I mean to test all of them... -deniedWebShellTokens=java.,beans,freemarker, DENIEDFILEEXTENSIONS = getDeniedFileExtensions(); - private static final List DENIEDWEBSHELLTOKENS = getDeniedWebShellTokens(); + private static final Pattern DENIEDWEBSHELLTOKENS = computeDeniedWebShellTokens(); + private static final List ALLOWEDWEBSHELLTOKENS = computeAllowedWebShellTokens(); private static final Integer MAXLINELENGTH = UtilProperties.getPropertyAsInteger("security", "maxLineLength", 0); private static final Boolean ALLOWSTRINGCONCATENATIONINUPLOADEDFILES = UtilProperties.getPropertyAsBoolean("security", "allowStringConcatenationInUploadedFiles", false); @@ -125,28 +128,31 @@ public class SecuredUpload { // 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 allowed) throws IOException { - return isValidText(content, allowed, false); - } - - public static boolean isValidText(String content, List 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 queryParameters = StringUtil.split(content, "&"); - return DENIEDWEBSHELLTOKENS.stream().allMatch(token -> isValid(queryParameters, token.toLowerCase(), allowed)); + String contentWithoutSpaces = StringUtil.removeSpaces(content); + 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 - return DENIEDWEBSHELLTOKENS.stream().allMatch(token -> isValid(content.toLowerCase(), token.toLowerCase(), allowed)); + return containsDeniedWebShellToken(List.of(content), allowed, getDeniedWebShellTokens()); + } + + public static boolean isValidQuery(String content, List allowed) throws IOException { + if (content == null) { + return false; + } + // Check the query string is safe, notably no reverse shell + return !containsDeniedWebShellToken(StringUtil.split(content, "&"), allowed, getDeniedWebShellTokens()); + } + + public static boolean containsDeniedWebShellToken(List contents, List allowed, Pattern deniedWebShellTokens) { + return contents.stream().anyMatch(token -> deniedWebShellTokens.matcher(token.toLowerCase()).find() + && !isAllowed(token.toLowerCase(), allowed)); } public static boolean isValidFileName(String fileToCheck, Delegator delegator) throws IOException { @@ -245,7 +251,7 @@ public static boolean isValidFileName(String fileToCheck, Delegator delegator) t */ public static boolean isValidFile(String fileToCheck, String fileType, Delegator delegator) throws IOException, ImageReadException { // Allow all - if (("true".equalsIgnoreCase(EntityUtilProperties.getPropertyValue("security", "allowAllUploads", delegator)))) { + if ("true".equalsIgnoreCase(EntityUtilProperties.getPropertyValue("security", "allowAllUploads", delegator))) { return true; } @@ -850,32 +856,25 @@ private static boolean isValidTextFile(String fileName, Boolean encodedContent) return isValidText(content, allowed); } - // Check there is no web shell - private static boolean isValid(String content, String string, List allowed) { - boolean isOK = !content.contains(string) || allowed.contains(string); - if (!isOK) { - Debug.logInfo("The uploaded file contains the string '" + string + "'. It can't be uploaded for security reason", MODULE); + private static boolean isAllowed(String content, List allowed) { + List allowedContents; + if (allowed != null) { + allowedContents = new ArrayList<>(allowed); + allowedContents.addAll(ALLOWEDWEBSHELLTOKENS); + } else { + allowedContents = ALLOWEDWEBSHELLTOKENS; } - return isOK; - } - - // Check there is no reverse shell in query string - private static boolean isValid(List queryParameters, String string, List 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 (UtilValidate.isEmpty(allowedContents)) { + return false; } - if (!isOK) { - Debug.logInfo("The HTTP query string contains the string '" + string + "'. It can't be uploaded for security reason", MODULE); + for (String allowContent : allowedContents) { + if ((allowContent.startsWith("$SHA") + && allowContent.equals(HashCrypt.cryptBytes("SHA", "OFBiz", content.toLowerCase().getBytes(StandardCharsets.UTF_8)))) + || content.trim().toLowerCase().contains(allowContent)) { + return true; + } } - return isOK; + return false; } private static void deleteBadFile(String fileToCheck) { @@ -891,9 +890,27 @@ private static List getDeniedFileExtensions() { return UtilValidate.isNotEmpty(deniedExtensions) ? StringUtil.split(deniedExtensions, ",") : new ArrayList<>(); } - private static List getDeniedWebShellTokens() { - String deniedTokens = UtilProperties.getPropertyValue("security", "deniedWebShellTokens"); - return UtilValidate.isNotEmpty(deniedTokens) ? StringUtil.split(deniedTokens, ",") : new ArrayList<>(); + public static Pattern getDeniedWebShellTokens() { + return DENIEDWEBSHELLTOKENS; + } + + private static Pattern computeDeniedWebShellTokens() { + return computeDeniedWebShellTokensPattern( + StringUtil.split(UtilProperties.getPropertyValue("security", "deniedWebShellTokens").toLowerCase(), ",")); + } + public static Pattern computeDeniedWebShellTokensPattern(List tokens) { + return Pattern.compile(tokens.stream() + .map(token -> ".*(%.{2,5}|[^\\w])" + token + "[^\\w].*") + .collect(Collectors.joining("|"))); + } + + private static List getAllowedTokens() { + return ALLOWEDWEBSHELLTOKENS; + } + + private static List computeAllowedWebShellTokens() { + String allowedTokens = UtilProperties.getPropertyValue("security", "allowedTokens"); + return UtilValidate.isNotEmpty(allowedTokens) ? StringUtil.split(allowedTokens, ",") : List.of(); } private static boolean checkMaxLinesLength(String fileToCheck) { diff --git a/framework/security/src/test/java/org/apache/ofbiz/security/SecurityUtilTest.java b/framework/security/src/test/java/org/apache/ofbiz/security/SecurityUtilTest.java index 2dae4f7b88..b838ca364f 100644 --- a/framework/security/src/test/java/org/apache/ofbiz/security/SecurityUtilTest.java +++ b/framework/security/src/test/java/org/apache/ofbiz/security/SecurityUtilTest.java @@ -18,6 +18,8 @@ */ package org.apache.ofbiz.security; +import java.util.regex.Pattern; +import org.apache.ofbiz.base.util.UtilCodec; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; @@ -27,6 +29,7 @@ import org.junit.Test; public class SecurityUtilTest { + @Test public void basicAdminPermissionTesting() { List adminPermissions = Arrays.asList("PARTYMGR", "EXAMPLE", "ACCTG_PREF"); @@ -53,4 +56,45 @@ public void multiLevelBadHierarchyPermissionTesting() { assertFalse(SecurityUtil.checkMultiLevelAdminPermissionValidity(adminPermissions, "HOTDEP_PARTYMGR_ADMIN")); } + @Test + public void isDeniedWebShell() { + Pattern testDeniedWebShellTokens = SecuredUpload.computeDeniedWebShellTokensPattern(List.of("suspecttoken", "allowedtoken")); + List.of(" suspecttoken.", + " suspectToken.", + " SuspectToken.", + " SUSPECTTOKEN.", + " suspectToken.", + " { + String encodedToken = UtilCodec.getEncoder("url").encode(token); + assertTrue("failed to stop '" + token + "' encoded as '" + encodedToken + " with " + testDeniedWebShellTokens, + SecuredUpload.containsDeniedWebShellToken(List.of(encodedToken), List.of(), testDeniedWebShellTokens)); + }); + } + + @Test + public void isAllowedWebShell() { + Pattern testDeniedWebShellTokens = SecuredUpload.computeDeniedWebShellTokensPattern(List.of("suspecttoken", "allowedtoken")); + List.of(" suspecttokena ", + "suspectToken", + " allowedToken(", + " allowedtoken(") + .forEach(token -> { + String encodedToken = UtilCodec.getEncoder("url").encode(token); + assertFalse("failed to allow '" + token + "' encoded as '" + encodedToken + " with " + testDeniedWebShellTokens, + SecuredUpload.containsDeniedWebShellToken(List.of(encodedToken), List.of("allowedtoken"), testDeniedWebShellTokens)); + }); + } } 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 4ccd968f54..9adbf67543 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 @@ -202,7 +202,8 @@ public void doFilter(HttpServletRequest req, HttpServletResponse resp, FilterCha if (queryString != null) { queryString = URLDecoder.decode(queryString, "UTF-8"); if (UtilValidate.isUrl(queryString) - || !SecuredUpload.isValidText(queryString.toLowerCase(), ALLOWEDTOKENS, true) + || !SecuredUpload.isValidQuery(queryString, + StringUtil.split(req.getServletContext().getInitParameter("allowedQueryTokens"), ",")) && isSolrTest()) { Debug.logError("For security reason this URL is not accepted", MODULE); throw new RuntimeException("For security reason this URL is not accepted"); From 6a72a1b419962e416857515ff8907f383d2a11af Mon Sep 17 00:00:00 2001 From: Gil Portenseigne Date: Fri, 31 Jan 2025 15:15:23 +0100 Subject: [PATCH 2/5] Renaming to avoid confusion --- .../main/java/org/apache/ofbiz/security/SecuredUpload.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 63b01715cc..7841cb0685 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 @@ -151,8 +151,8 @@ public static boolean isValidQuery(String content, List allowed) throws } public static boolean containsDeniedWebShellToken(List contents, List allowed, Pattern deniedWebShellTokens) { - return contents.stream().anyMatch(token -> deniedWebShellTokens.matcher(token.toLowerCase()).find() - && !isAllowed(token.toLowerCase(), allowed)); + return contents.stream().anyMatch(content -> deniedWebShellTokens.matcher(content.toLowerCase()).find() + && !isAllowed(content.toLowerCase(), allowed)); } public static boolean isValidFileName(String fileToCheck, Delegator delegator) throws IOException { From 3d5e3b75764af7de85d4a5674ceae454c5024c75 Mon Sep 17 00:00:00 2001 From: Gil Portenseigne Date: Fri, 31 Jan 2025 15:16:30 +0100 Subject: [PATCH 3/5] Add javadoc and simplify regex to detect token patterns --- .../java/org/apache/ofbiz/security/SecuredUpload.java | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) 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 7841cb0685..e275bad6e8 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 @@ -898,9 +898,18 @@ private static Pattern computeDeniedWebShellTokens() { return computeDeniedWebShellTokensPattern( StringUtil.split(UtilProperties.getPropertyValue("security", "deniedWebShellTokens").toLowerCase(), ",")); } + + /** + * Build Pattern from list of token, based on regexp that complies to + * 1 - any non word suffix : ^[a-zA-Z0-9_] + * 2 - any non word prefix : ^[a-zA-Z0-9_] + * OR any prefix with '%' followed by 2 to 5 chars. (for percent-encoding char in query, ie. %21 for !) + * @param tokens to support detection rule + * @return built pattern + */ public static Pattern computeDeniedWebShellTokensPattern(List tokens) { return Pattern.compile(tokens.stream() - .map(token -> ".*(%.{2,5}|[^\\w])" + token + "[^\\w].*") + .map(token -> "(%.{2,5}|[^\\w])" + token + "[^\\w]") .collect(Collectors.joining("|"))); } From 77657fb86ac74bae37dc18cfc8b1131c1d016ae5 Mon Sep 17 00:00:00 2001 From: Gil Portenseigne Date: Fri, 31 Jan 2025 15:36:18 +0100 Subject: [PATCH 4/5] Ease readability --- .../org/apache/ofbiz/security/SecuredUpload.java | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) 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 e275bad6e8..aa7e7e2901 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 @@ -857,20 +857,14 @@ private static boolean isValidTextFile(String fileName, Boolean encodedContent) } private static boolean isAllowed(String content, List allowed) { - List allowedContents; + List allowedContents = new ArrayList<>(ALLOWEDWEBSHELLTOKENS); if (allowed != null) { - allowedContents = new ArrayList<>(allowed); - allowedContents.addAll(ALLOWEDWEBSHELLTOKENS); - } else { - allowedContents = ALLOWEDWEBSHELLTOKENS; - } - if (UtilValidate.isEmpty(allowedContents)) { - return false; + allowedContents.addAll(allowed); } for (String allowContent : allowedContents) { - if ((allowContent.startsWith("$SHA") - && allowContent.equals(HashCrypt.cryptBytes("SHA", "OFBiz", content.toLowerCase().getBytes(StandardCharsets.UTF_8)))) - || content.trim().toLowerCase().contains(allowContent)) { + boolean allowedHashedTokenIdentified = allowContent.startsWith("$SHA") + && allowContent.equals(HashCrypt.cryptBytes("SHA", "OFBiz", content.toLowerCase().getBytes(StandardCharsets.UTF_8))); + if (allowedHashedTokenIdentified || content.trim().toLowerCase().contains(allowContent)) { return true; } } From 00e8efe7757f795d87562e72330038657c4aa2b3 Mon Sep 17 00:00:00 2001 From: Gil Portenseigne Date: Fri, 31 Jan 2025 17:26:38 +0100 Subject: [PATCH 5/5] allowed query tokens seems uneeded anymore => better delete for now to release. --- .../java/org/apache/ofbiz/security/SecuredUpload.java | 2 +- .../apache/ofbiz/webapp/control/ControlFilter.java | 11 ++--------- 2 files changed, 3 insertions(+), 10 deletions(-) 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 aa7e7e2901..459777e3d5 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 @@ -907,7 +907,7 @@ public static Pattern computeDeniedWebShellTokensPattern(List tokens) { .collect(Collectors.joining("|"))); } - private static List getAllowedTokens() { + public static List getAllowedTokens() { return ALLOWEDWEBSHELLTOKENS; } 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 9adbf67543..763f37fd68 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 @@ -90,8 +90,6 @@ public class ControlFilter extends HttpFilter { private int errorCode; /** The list of all path prefixes that are allowed. */ private Set allowedPaths; - private static final List ALLOWEDTOKENS = getAllowedTokens(); - @Override public void init(FilterConfig conf) throws ServletException { @@ -151,11 +149,6 @@ private void redirect(HttpServletResponse resp, String contextPath) throws IOExc resp.sendRedirect(redirectPathIsUrl ? redirectPath : (contextPath + redirectPath)); } - private static List getAllowedTokens() { - String allowedTokens = UtilProperties.getPropertyValue("security", "allowedTokens"); - return UtilValidate.isNotEmpty(allowedTokens) ? StringUtil.split(allowedTokens, ",") : new ArrayList<>(); - } - /** * Makes allowed paths pass through while redirecting the others to a fix location. * Reject wrong URLs @@ -202,8 +195,8 @@ public void doFilter(HttpServletRequest req, HttpServletResponse resp, FilterCha if (queryString != null) { queryString = URLDecoder.decode(queryString, "UTF-8"); if (UtilValidate.isUrl(queryString) - || !SecuredUpload.isValidQuery(queryString, - StringUtil.split(req.getServletContext().getInitParameter("allowedQueryTokens"), ",")) + //TODO : Solr dependency in framework, how to avoid, through webapp def ? + || !SecuredUpload.isValidQuery(queryString, List.of()) && isSolrTest()) { Debug.logError("For security reason this URL is not accepted", MODULE); throw new RuntimeException("For security reason this URL is not accepted");